-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
⚠️ (go/v3) upgrade controller-runtime from 0.10.0 to 0.11.0, k8s from 1.22 to 1.23 and controller-gen from v0.0.7 to v0.8.0 #2471
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @ankitm123! |
Seems like for make install and make generate to work, it needs go1.17.
I can try to track down the errors, probably need to pin some packages, if upgrade to go1.17 is undesired. Basically structfield.IsExported was added in go 1.17, and hence requires go 1.17. EDIT: I went ahead and pinned the version of apimachinary to v0.22.2, and that seems to pass install and unit tests locally for me. |
Hi @ankitm123. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
7ae4803
to
7ccdb85
Compare
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.
Thank you for your contribution 🥇
Just a few nits:
a) To update the controller-runtime into the scaffolds is required to do the change in:
->
ControllerRuntimeVersion = "v0.10.0" |
- Then, run
make generate
to update all scaffolds.
b) Since the goal is the scaffolds began to use the new version we must use the mojo ':warning:' in the title. Also, since it is used to create our release notes WDYT about : :warning: (go/v3) upgrade controller-runtime from 0.10 to 0.11
Is it required to upgrade the go version as well? If yes, that we need to clarify that in the title, an update in the scaffolds as in the docs regards the go version required. E.g ( :warning: (go/v3) upgrade controller-runtime from 0.10 to 0.11, k8s to , and golang version to 1.17
/ok-to-test |
testdata/project-v3-multigroup/controllers/ship/frigate_controller.go
Outdated
Show resolved
Hide resolved
sgtm, will update. |
7ccdb85
to
34434ea
Compare
@camilamacedo86 So I went forward and upgraded the go version in this PR along with the commit message. I will check why the PRs are failing, and update this PR. |
34434ea
to
e8ea885
Compare
This file needs to be updated with go:17: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/kubebuilder/kubebuilder-presubmits.yaml |
2676b9d
to
58d8ecf
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.
Shows that we are also missing updated: https://github.com/kubernetes-sigs/kubebuilder/blob/master/build/.goreleaser.yml#L46
…rom 1.22 to 1.23 Signed-off-by: ankitm123 <ankitmohapatra123@gmail.com>
58d8ecf
to
c8b0546
Compare
All tests (except api-diff) passed finally! 🎉 |
/hold cancel |
/lgtm @varshaprasad96 wdyt can we move forward here? |
/override APIDiff |
@camilamacedo86: /override requires a failed status context or a job name to operate on.
Only the following contexts were expected:
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. |
/override ci/APIDiff |
@camilamacedo86: /override requires a failed status context or a job name to operate on.
Only the following contexts were expected:
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. |
/skip APIDiff |
It's a github check, and not prow, so override wont work 🤔 , needs to be done in gh(?). |
Hi @ankitm123
Yep. I was trying to see if we let it merge alone. I can force the merge anyway. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankitm123, camilamacedo86, varshaprasad96 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 |
Description
also, envtest setup from 1.22 to 1.23
NOTE: This pr changes the scaffolds produced by KB CLI for go/v3 plugin. The go/v2 plugin cannot be upgraded and has been kept as a legacy.
Signed-off-by: ankitm123 ankitmohapatra123@gmail.com
fixes #2448