🐛 (for all generators): fix being silently ignored when placed directly above var/const/func declarations without a blank line. Both with and without blank line styles now work - #1387
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86 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 |
db6eb15 to
63f65f3
Compare
| // +testing:typelvl="here on type" | ||
| // normal godoc | ||
| type Foo struct { | ||
| // +testing:pkglvl="not here in struct" |
There was a problem hiding this comment.
This implies to me that at some point there was a deliberate choice to not support what this bug fix is now supporting. What kind of impact might an end user expect from this change?
There was a problem hiding this comment.
Good catch. That assertion verifies a separate behavior (removed by mistake). I've added it back to show the two are unrelated.
|
This feels like a relatively risky change, since it has the potential to cause huge diffs in the generation for folks who haven't noticed that their markers aren't being observed. What about a warning when the markers are being ignored instead of suddenly observing them? |
63f65f3 to
98f5f49
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86 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 |
…ration) being silently ignored when placed directly above var/const/func declarations without a blank line. Both with and without blank line styles now work
98f5f49 to
382f605
Compare
|
Hi @JoelSpeed, I'm not sure I follow the concern. If someone explicitly adds a marker, I'd expect it to take effect. Silently ignoring it feels more surprising. POV, the current behavior is the bug. This PR fixes that by making the marker behave as documented, rather than introducing a new behavior. If the concern is unexpected diffs, those diffs would only happen because users had already added the marker but weren't getting the expected result. So, why someone would add the marker if does not want the behaviour of the mark at all? Also, this is limited to package-level markers (such as RBAC and webhook markers). Type and field markers already work without requiring a blank line. For example, on // +kubebuilder:default={}
DefaultedEmptyMap map[string]string `json:"defaultedEmptyMap"`
// +kubebuilder:validation:Required
ExplicitlyRequiredKubebuilder string `json:"...,omitempty"`The inconsistency is that package-level markers placed the same way are silently ignored. To me, that's confusing. This PR just makes package-level markers behave consistently with the rest of the marker system. This also matches #436, where this behavior was reported as unexpected rather than intentional. |
Package-level markers like +kubebuilder:rbac, +kubebuilder:webhook, +groupName, etc. were silently ignored when placed directly above
Both styles now work (backward compatible):
Closes #551
Assisted-by: Claude