-
Notifications
You must be signed in to change notification settings - Fork 84
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
Bump controller-runtime to 0.17 / kube to 1.29 #380
Bump controller-runtime to 0.17 / kube to 1.29 #380
Conversation
justinsb
commented
Feb 26, 2024
- chore: Update to controller-runtime 0.17 / kube 1.29
- deprecation: Use NewDynamicRESTMapper instead of NewDiscoveryRESTMapper
We need kubectl 1.29.1 due to a regression in OpenAPIGetter in kubectl 1.29.0, fixed in 1.29.1
NewDiscoveryRESTMapper has been removed from the latest controller-runtime. NewDynamicRESTMapper is preferred, and is available in older versions of controller-runtime also, so prefer that in all controller-runtime versions.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb 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 |
Let me know if I can help speed this along somehow! |
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 know this PR is WIP, but I left just one comment.
go 1.20 | ||
go 1.21 | ||
|
||
toolchain go1.22.0 |
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.
Maybe, we should update the files for GitHub Actions?
- https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/blob/master/.github/workflows/main.yml
This file uses the go v1.20 setup at 2 place.
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.
You're right - I just pushed that and tests are looking much happier!
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.
And I just saw that @tomasaschan proposed a better version of the change in justinsb#1 , so using that one
@justinsb |
52d455f
to
7a185d0
Compare
Removed WIP, and I also think we should consider expanding ownership of this repo, perhaps starting with @tomasaschan . Sometimes I get pulled into different things and things fall off my plate - sorry for the delays here! |
/lgtm (but I suspect it won't matter yet...) |
@tomasaschan: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
I agree with expanding ownership. 👍 |