-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Update vendor to Kubernetes 1.24.0 #4871
Update vendor to Kubernetes 1.24.0 #4871
Conversation
This follows go update in kubernetes: kubernetes/kubernetes#109461
package provider // import "sigs.k8s.io/cloud-provider-azure/pkg/provider" | ||
package provider // import "sigs.k8s.io/cloud-provider-azure/pkg/provider |
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.
Seems the test script is choking on this change, I wonder if it was manually fixed on the PR introducing it.
Looks like it was fixed in v1.24.0: kubernetes-sigs/cloud-provider-azure#1158
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 opened #4875 about this. The obvious solution is to carry the suspected fix in vendor, but I'd rather not do that if we can avoid it. Among other problems it may break anyone importing CA code (transitive dependency would be taken from original repo and not the fixed version in CA vendor in that case).
Let's see if Azure maintainers can help us with this one.
629e6d3
to
159dae0
Compare
Manually removed the update to vendor/sigs.k8s.io/cloud-provider-azure/pkg/provider/doc.go as per discussion in kubernetes#4875.
159dae0
to
3b361b6
Compare
/hold |
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.
One query for my understanding, but other than that (and the Azure issue):
/lgtm
/approve
@@ -151,6 +151,6 @@ func TestDebugInfo(t *testing.T) { | |||
|
|||
predicateErr := predicateChecker.CheckPredicates(clusterSnapshot, p1, "n1") | |||
assert.NotNil(t, predicateErr) | |||
assert.Equal(t, "node(s) had taint {SomeTaint: WhyNot?}, that the pod didn't tolerate", predicateErr.Message()) | |||
assert.Equal(t, "node(s) had untolerated taint {SomeTaint: WhyNot?}", predicateErr.Message()) |
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 assume this is a manually applied change to make the test match the upstream error message?
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.
Correct, the test relies on the wording of upstream error message. The message has changed upstream and so I had to update the test.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gjtempleton, MaciekPytel 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 |
/hold cancel |
Which component this PR applies to?
cluster-autoscaler
What type of PR is this?
Update vendor for CA 1.24 release