-
Notifications
You must be signed in to change notification settings - Fork 38.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix custom metrics conversion functions #94481
Conversation
7356f97
to
d376f86
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
d376f86
to
615f903
Compare
/retest |
@liggitt - PTAL when you will have a chance |
@@ -124,8 +124,7 @@ func Convert_custom_metrics_MetricListOptions_To_v1beta2_MetricListOptions(in *c | |||
} | |||
|
|||
func autoConvert_v1beta2_MetricValue_To_custom_metrics_MetricValue(in *MetricValue, out *custommetrics.MetricValue, s conversion.Scope) error { | |||
// TODO: Inefficient conversion - can we improve it? | |||
if err := s.Convert(&in.DescribedObject, &out.DescribedObject, 0); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this a bug? would this call actually work in 1.19?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would when I was changing that, but you're right - it was a bug.
scope.Convert calls converter:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/conversion/converter.go#L183
which in turn were only looking into registered conversions:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/conversion/converter.go#L289
which doesn't exist (until this PR).
So yes - it seemed to be a 1.19 bug (it also means we have 0 tests for custom metrics...)
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ref #87006
/kind cleanup
/priority important-longterm