✨ Add option to omit controller-gen version in generated output - #1437
✨ Add option to omit controller-gen version in generated output#1437pedjak wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pedjak The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @pedjak. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
0350788 to
20bbcde
Compare
|
/assign @sbueringer @alvaroaleman @camilamacedo86 |
| // (controller-gen.kubebuilder.io/version) should be added to the CRD. | ||
| // | ||
| // Left unspecified, the default is true. | ||
| AddToolVersionAnnotation *bool `marker:",optional"` |
There was a problem hiding this comment.
The current behaviour is to have that by default so should not be skip something like?
I do not think we would like to change the default behaviour.
There was a problem hiding this comment.
@camilamacedo86 Agreed, the default behavior stays unchanged — the annotation is always added unless you explicitly opt out.
Would you prefer a naming like SkipToolVersionAnnotation (default false, set to true to opt out)? Or do you have another suggestion in mind?
There was a problem hiding this comment.
Done — renamed to SkipToolVersionAnnotation (default false, set to true to opt out). Also renamed the applyconfiguration field to SkipToolVersion for consistency.
20bbcde to
f79d4d5
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pedjak The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| // (controller-gen.kubebuilder.io/version) should be skipped when generating the CRD. | ||
| // | ||
| // Left unspecified, the default is false. | ||
| SkipToolVersionAnnotation *bool `marker:",optional"` |
There was a problem hiding this comment.
Why not keep the same name for both places ?
Why not SkipToolVersion here as well?
WDYT?
There was a problem hiding this comment.
Done — renamed to OmitToolVersion in both generators for consistency.
| ExternalApplyConfigurations []string `marker:",optional"` | ||
|
|
||
| // SkipToolVersion specifies if the tool version should be excluded from the | ||
| // "Code generated by" comment header. When true, the generator name is |
There was a problem hiding this comment.
helpgen only uses the first physical line of a field comment as the summary. Since the first sentence wraps onto a second line, the generated summary gets cut off:
Summary: "specifies if the tool version should be excluded from the"That is what controller-gen applyconfiguration -w shows, so the help text looks truncated.
Could we make the first line a complete sentence and move the extra explanation into the details in both one? For example:
For CRD
// OmitToolVersion omits the controller-gen version annotation from the generated CRD.
//
// When set, the controller-gen.kubebuilder.io/version annotation is not written
// to the CRD's metadata. This only affects that annotation; the generated schema
// is unchanged. Left unspecified, the default is false.And For Here:
// OmitToolVersion omits the tool version from the "Code generated by" file header.
//
// When set, the generator name in the header is normalized to "controller-gen"
// regardless of the binary name, instead of any version-bearing name. This only
// affects the header comment; no annotations are involved. Left unspecified, the
// default is false.Also, it makes clear what it does.
There was a problem hiding this comment.
Good catch — restructured the godoc so the first line is a complete sentence in both generators. The generated help summaries now read cleanly.
|
|
||
| Expect(string(content)).To(HavePrefix("// Code generated by controller-gen. DO NOT EDIT.")) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
I think this test should also cover the default case.
Right now it only checks that the generated header is:
// Code generated by controller-gen. DO NOT EDIT.That passes because the marker changes the generator name to controller-gen. But the test does not prove that the marker caused the change.
Could we add a second case that runs without skipToolVersion=true and checks that the header is not controller-gen?
It("should leave the generator name unchanged when SkipToolVersion is unset", func() {
// run without skipToolVersion=true
Expect(string(content)).NotTo(HavePrefix(
"// Code generated by controller-gen. DO NOT EDIT.",
))
})Then the existing test can keep checking the marker case:
It("should normalize the generator name when SkipToolVersion is true", func() {
// run with skipToolVersion=true
Expect(string(content)).To(HavePrefix(
"// Code generated by controller-gen. DO NOT EDIT.",
))
})Together, these prove that SkipToolVersion is what changes the generated header, instead of the test passing because of the test binary name.
There was a problem hiding this comment.
Added a default-case test that verifies the header preserves the original generator name when OmitToolVersion is unset. Also added os.RemoveAll(tmpDir) to AfterEach to clean up the temp directory.
| // When SkipToolVersion is true, override os.Args[0] so that gengo's | ||
| // GoBoilerplate (which reads filepath.Base(os.Args[0])) emits a | ||
| // version-free "Code generated by controller-gen" header. | ||
| if d.SkipToolVersion != nil && *d.SkipToolVersion { |
There was a problem hiding this comment.
WDYT about OmitToolVersion?
Omit seems to be a bette verb, wdyt?
There was a problem hiding this comment.
Agreed — renamed to OmitToolVersion everywhere. Much clearer.
f79d4d5 to
1a7e9ad
Compare
|
|
||
| By("searching for version annotation") | ||
| Expect(out.buf.String()).NotTo(ContainSubstring("controller-gen.kubebuilder.io/version")) | ||
| }) |
There was a problem hiding this comment.
Could we have the other way around here?
Should add the version annotation by default when omittoolversion is not informed at all.
|
Hi @pedjak,
The current error message is not clear. The actual fix is to remove the existing year from the header of applyconfiguration_integration_test.go. This failure is caused by the new Python check introduced by @sbueringer and @alvaroaleman here: @sbueringer @alvaroaleman, honestly, the Python implementation seems more complex than necessary for this requirement, and it does not appear to correctly ignore files that existed before the check was introduced. The most straightforward fix would be to remove the year from all existing license headers. Otherwise, we will continue to see inconsistent failures in files that were not changed. The check should also clearly explain that the year must be removed. The current error message does not identify the actual problem. |
No, we should not modify existing copyright headers to remove the year, see:
The issue here is not that the boilerplate verification somehow didn't check this file before. It always verifies all files. The reason why this check fails now is because we intentionally have different rules for generated and not generated files:
The problem here in this PR is that we are introducing the "DO NOT EDIT" string into this file. Because of that the file is now detected as a generated file and accordingly we are now getting a hint that we should remove the year. I see two options on how to resolve this:
|
Add per-generator options to control whether controller-gen version information is included in generated files: - crd:omitToolVersion — omits the controller-gen.kubebuilder.io/version annotation from CRD manifests - applyconfiguration:omitToolVersion — omits the tool version from the "Code generated by" comment header Both default to false, preserving current behavior.
1a7e9ad to
90a5751
Compare
|
@camilamacedo86 @sbueringer — rebased onto main and went with option 2. |
What does this do, and why do we need it?
When
controller-genruns, it embeds its version into generated files:controller-gen.kubebuilder.io/versionannotationCode generated bycomment headerWhen a team upgrades controller-gen, every generated file shows a diff even though the actual schema, RBAC rules, or deepcopy logic is unchanged. This creates large, noisy pull requests that obscure real changes and complicate code review.
This PR adds per-generator options to opt out of including version information:
crd:omitToolVersion=true— omits thecontroller-gen.kubebuilder.io/versionannotation from CRD manifestsapplyconfiguration:omitToolVersion=true— normalizes the generator name tocontroller-genin theCode generated bycomment, preventing version-suffixed binary names from leaking into outputBoth options default to
false, preserving current behavior.Fixes #1436