-
Notifications
You must be signed in to change notification settings - Fork 632
Update CRD generator to handle errors encountered during generation. #4158
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 CRD generator to handle errors encountered during generation. #4158
Conversation
In addition to the generator fix, adjustments have been made to existing CRDs to correct errors which were not handled before, and `./hack/update-codegen.sh` was executed. The listType marker was added to two non-list fields in 78496d8 The MaxLength validation marker was used on an array field instead of MaxItems in 110bcaf
|
Welcome @joshlreese! |
|
Hi @joshlreese. 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-sigs/prow repository. |
|
Thanks @joshlreese! This is an important fix. Please update the release note to make it clear that this represents multiple breaking changes + highlight the new validation that is enforced (specifically the new max number of items). /ok-to-test |
|
@robscott - I've updated the release note to be a bit more clear - I tried to find other PRs with breaking changes as references but didn't have much luck other than seeing the PR titles mention a breaking change (seen in #3937). The listType change appears to only have influenced the generated OpenAPI content - is that a breaking change as well? |
This looks great, thanks!
Not in my opinion. I think we really only want to focus on changes to the k8s API, so in this case that means changes to the CRDs. This change LGTM, but will wait for someone else to sign off since it is technically a breaking change. |
|
Good catch, thanks! The atomic ones were introduced here: #3964 and I totally missed it! thanks! For the CRD changes, those are made on experimental APIs (ext_auth), so while I agree we need to be sure that this is visible, I think we are fine breaking experimental APIs :) @robscott a detail: our CI actions are not running for new contributors, so I need someone with write access here to approve the run (and change on the project config to always run!) |
rikatz
left a comment
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
/hold
Let's just trigger the pipeline actions first
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joshlreese, rikatz, robscott 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 |
|
/cherry-pick release-1.4 |
|
@rikatz: #4158 failed to apply on top of branch "release-1.4": 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-sigs/prow repository. |
…ubernetes-sigs#4158) In addition to the generator fix, adjustments have been made to existing CRDs to correct errors which were not handled before, and `./hack/update-codegen.sh` was executed. The listType marker was added to two non-list fields in 78496d8 The MaxLength validation marker was used on an array field instead of MaxItems in 110bcaf
|
@rikatz |
…ubernetes-sigs#4158) In addition to the generator fix, adjustments have been made to existing CRDs to correct errors which were not handled before, and `./hack/update-codegen.sh` was executed. The listType marker was added to two non-list fields in 78496d8 The MaxLength validation marker was used on an array field instead of MaxItems in 110bcaf
|
As I see, the fix didn't make it into v1.4.1 due to the failed cherry-picking automation. Not sure whom to ping, could any of you take a look at this, please? :) @danwinship @robscott @shaneutt @rikatz |
What type of PR is this?
/kind bug
What this PR does / why we need it:
There are a handful of invalid markers on CRDs today as a result of the CRD generator not handling errors encountered during generation. This leads to either unnecessary metadata in CRD manifests, or missing validation (in the case of MaxLength being used instead of MaxItems).
Prior to correcting the markers on the existing CRDs, the generator produced the following errors:
The use of
loader.PrintErrorswas taken from https://github.com/kubernetes-sigs/controller-tools/blob/5c2d62552c97a0139c1dfdc6276c69f2a5f6120d/pkg/genall/genall.go#L282.Which issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: