-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Fix informer-gen to honor nonNamespaced tag #80458
Fix informer-gen to honor nonNamespaced tag #80458
Conversation
Hi @tatsuhiro-t. Thanks for your PR. I'm waiting for a kubernetes 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. |
/cc @yliaog |
please add a test case for this fix |
Where is the existing test case? |
it looks there is not one currently. but still it is good to add one. |
I have no time to add test case from scratch. I think it is much better and beneficial to everyone to merge this simple fix rather than leaving code-generator in miserable, unusable state. |
please see my comment in #80457, i think the current informer gen is working as intended. |
I disagree. The other generators can handle this without extra blank lines. Actually, informer-gen partially handles this except for the broken portion which #80458 tries to fix. |
ok, i agree with you. could you maybe modify the test in the following directories to make sure it won't regress. staging/src/k8s.io/code-generator/hack/verify-codegen.sh |
904dc3e62e0628fa49249779687cdf730c9ac4c0 modifies test to exercise nonNamespaced tag |
staging/src/k8s.io/code-generator/_examples/apiserver/apis/example2/types.go
Show resolved
Hide resolved
/lgtm |
/lgtm |
what else is required for this PR to be merged? Maybe I can help? |
it needs approval. |
Is there any blocking issue to approve this PR? |
super tiny nit: can you squash the last two commits together? /assign @sttts |
/priority important-soon |
/milestone v1.17 |
51038db
to
937158b
Compare
@nikhita Squashed 2 last commits into one and force-pushed. Thank you. |
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
node-e2e is currently broken: #83086 |
/retest |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts, tatsuhiro-t 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 |
/retest Review the full test history for this PR. Silence the bot with an |
/retest |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes #80457 so that informer-gen produces correct code for a type with nonNamespaced tag.
Which issue(s) this PR fixes:
Fixes #80457
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: