-
Notifications
You must be signed in to change notification settings - Fork 1.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 kubernetes-sigs/controller-tools to v0.5.0 #2200
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. |
I wasn't sure if an API diff was considered a breaking change since it is minor bump. It does change the output of Additionally, hopefully the CLA will sort itself out. |
Looks good to me. Seems like the only changes to test data are line wrapping whitespace for the descriptions and the annotations with the controller-tools version. |
@@ -41,7 +41,7 @@ Projects scaffolded with Kubebuilder v3 will use the `go.kubebuilder.io/v3` plug | |||
* A new option to create the projects using ComponentConfig is introduced. For more info see its [enhancement proposal][enhancement proposal] and the [Component config tutorial][component-config-tutorial] | |||
* Manager manifests now use `SecurityContext` to address security concerns. More info: [#1637][issue-1637] | |||
- Misc: | |||
* Support for [controller-tools][controller-tools] `v0.4.1` (for `go/v2` it is `v0.3.0` and previously it was `v0.2.5`) | |||
* Support for [controller-tools][controller-tools] `v0.5.0` (for `go/v2` it is `v0.3.0` and previously it was `v0.2.5`) |
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.
is this version bump going to be back ported ?
If not, can we directly bump controller-tools to 0.6.0 and controller-runtime to 0.9.0. That would bring in k8s 1.21 support for projects scaffolded with v3 plugin.
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 can definitely add bumps - controller-tools v0.6.0 had yet to be published at the time of PR. Is the preference for including it in this PR or should I start a separate one to include those bumps?
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.
Two PRs is fine with me.
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'll create a second PR today at some point today to cover this (assuming following caveat is resolved). I did an initial pass updating controller-tools to v0.6.0 and controller-runtime to v0.9.0. The controller-runtime update seemed to cause some issues, but I haven't had a chance to investigate yet.
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.
#2208 will handle this.
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.
@estroz sg!
I believe the white space differences may be a result of |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Birdrock, estroz 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 |
/override "APIDiff / Verify API differences" |
@estroz: /override requires a failed status context 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 APIDiff |
@estroz: /override requires a failed status context 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. |
Merging this myself since I cannot override a GH action. |
@estroz it's annoying isn't it? |
Description
kubebuilder has been updated to use github.com/gobuffalo/flect v0.2.2; the v3/scaffolds/init.go specifies kubernetes-sigs/controller-tools v0.4.1, which has github.com/gobuffalo/flect v0.2.0 as a dependency. kubernetes-sigs/controller-tools v0.5.0 has github.com/gobuffalo/flect v0.2.2 as a dependency.
Motivation
The two versions of github.com/gobuffalo/flect have slightly difference casing mechanics, so pluralizing doesn't exactly match. #1993 points this out, but was closed. While
kubebuilder create api
pluralizes properly,make manifests
creates different pluralization in some cases (e.g.: Builder -> Buildren). Reconciling these two versions coordinates the casing mechanics.Regardless of the underlying problem, it seems like a good idea to keep these dependencies synchronized. This could be considered a fix to part of #1933.