Add references linter #163
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: krishagarwal278 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @krishagarwal278! |
k8s-ci-robot
added
cncf-cla: no
krishagarwal278
force-pushed
the
KAL-25
branch
from
a2cce44 to
4e54c0c
Compare
docs/linters.md
Outdated
| ```yaml | ||
| lintersConfig: | ||
| references: | ||
| allowRefAndRefs: false | true # Allow Ref/Refs suffixes for OpenShift compatibility. Defaults to `false`. |
There was a problem hiding this comment.
We generally prefer enums over bools, so maybe lets change this to be more like policy config like we have in other places. Perhaps the values could be AllowRefAndRefs and ForbidRefAndRefs?
There was a problem hiding this comment.
Nit: Let's also drop the OpenShift mention. This is a vendor-neutral project.
There was a problem hiding this comment.
yes i agree. Having policy config in an enum would make a lot more sense. Incorporated in latest commit 👍🏻
docs/linters.md
Outdated
| **OpenShift compatibility (allowRefAndRefs=true):** | ||
| - Reports errors for fields ending with 'Reference' or 'References' | ||
| - Allows fields ending with 'Ref' or 'Refs' without reporting errors |
There was a problem hiding this comment.
In this mode we want to also report Ref and Refs and drop all of these
| @@ -0,0 +1,22 @@ | |||
| package a | |||
|
|
|||
| type TestSpec struct { | |||
There was a problem hiding this comment.
Can you try some field names where they contain the string ref/refs or reference/references but not at the end? Eg Prefix or Prefereneces perhaps could be in field names?
There was a problem hiding this comment.
across both 'a' and 'b' test files i incorporated tests for prefix, suffix, middle of the field and edge cases 👍🏻
pkg/analysis/references/analyzer.go
Outdated
| func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo) { | ||
| if field == nil || len(field.Names) == 0 { | ||
| return | ||
| } | ||
|
|
||
| fieldName := utils.FieldName(field) | ||
|
|
||
| if strings.HasSuffix(fieldName, "Reference") { | ||
| suggestedName := strings.TrimSuffix(fieldName, "Reference") + "Ref" | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: field.Pos(), | ||
| Message: fmt.Sprintf("field %s should use 'Ref' instead of 'Reference'", fieldName), | ||
| SuggestedFixes: []analysis.SuggestedFix{ | ||
| { | ||
| Message: "replace 'Reference' with 'Ref'", | ||
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: field.Names[0].Pos(), | ||
| NewText: []byte(suggestedName), | ||
| End: field.Names[0].End(), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if strings.HasSuffix(fieldName, "References") { | ||
| suggestedName := strings.TrimSuffix(fieldName, "References") + "Refs" | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: field.Pos(), | ||
| Message: fmt.Sprintf("field %s should use 'Refs' instead of 'References'", fieldName), | ||
| SuggestedFixes: []analysis.SuggestedFix{ | ||
| { | ||
| Message: "replace 'References' with 'Refs'", | ||
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: field.Names[0].Pos(), | ||
| NewText: []byte(suggestedName), | ||
| End: field.Names[0].End(), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| // If allowRefAndRefs is false, report errors for standalone Ref/Refs fields | ||
| // If allowRefAndRefs is true (OpenShift), don't report errors for Ref/Refs fields | ||
| if !a.allowRefAndRefs { | ||
| // Check for fields ending with Ref or Refs (excluding those already handled above) | ||
| if fieldName != "Ref" && strings.HasSuffix(fieldName, "Ref") && !strings.HasSuffix(fieldName, "eRef") { | ||
| pass.Reportf(field.Pos(), "field %s should not use 'Ref' suffix", fieldName) | ||
| } | ||
|
|
||
| if fieldName != "Refs" && strings.HasSuffix(fieldName, "Refs") && !strings.HasSuffix(fieldName, "eRefs") { | ||
| pass.Reportf(field.Pos(), "field %s should not use 'Refs' suffix", fieldName) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is all very well written, but, take a look at the namingconventions linter. I believe we can implement this linter as a wrapper around that linter just by providing it with fixed configuration based on the config rules for this particular linter.
| lintersConfig: | ||
| references: {} | ||
There was a problem hiding this comment.
NIt, probably best to remove the blank lines in these code blocks
|
|
||
| // validateConfig validates the configuration for the references linter. | ||
| func validateConfig(cfg *Config, fldPath *field.Path) field.ErrorList { | ||
| // No validation needed for this simple config |
There was a problem hiding this comment.
If we switch to an enum, I'd expect some validation here FWIW
There was a problem hiding this comment.
policy validation incorporated 👍🏻
.golangci.yaml
Outdated
| - govet | ||
| - importas | ||
| - ineffassign | ||
| - kubeapilinter |
There was a problem hiding this comment.
Reverted that change 👍🏻 can't have kubeapilinter as a linter itself for this project 😆
Was testing out/exploring different files in the linter and so might've committed this as a change too
There was a problem hiding this comment.
I want more test cases about references linter. And you mention OpenShift, but I think kube-api-linter should provide better practices for all developer who design Kubernetes API. Though original issue says OpenShift, I think it's better to publish this linter more general.
docs/linters.md
Outdated
| - Reports errors for fields ending with 'Reference' or 'References' | ||
| - Also reports errors for fields ending with 'Ref' or 'Refs' (unless they were corrected from Reference/References) | ||
|
|
||
| **OpenShift compatibility (allowRefAndRefs=true):** |
There was a problem hiding this comment.
OpenShift compatibility
Is this unique problem with OpenShift ? Other projects might use this option, so I suggest to rename this to more general.
There was a problem hiding this comment.
Not unique to Openshift. Removed mention 👍🏻
pkg/analysis/references/analyzer.go
Outdated
| if fieldName != "Ref" && strings.HasSuffix(fieldName, "Ref") && !strings.HasSuffix(fieldName, "eRef") { | ||
| pass.Reportf(field.Pos(), "field %s should not use 'Ref' suffix", fieldName) | ||
| } | ||
|
|
||
| if fieldName != "Refs" && strings.HasSuffix(fieldName, "Refs") && !strings.HasSuffix(fieldName, "eRefs") { | ||
| pass.Reportf(field.Pos(), "field %s should not use 'Refs' suffix", fieldName) | ||
| } |
There was a problem hiding this comment.
What is eRef and eRefs ? why do we check the field with it ? Isn't it enough to only check Ref or Refs ?
I want some examples for test.
There was a problem hiding this comment.
replaced the initially added condition to check for letters/patterns before or after. Incorporated edge cases in test files 👍🏻
docs/linters.md
Outdated
|
|
||
| **Default behavior (allowRefAndRefs=false):** | ||
| - Reports errors for fields ending with 'Reference' or 'References' | ||
| - Also reports errors for fields ending with 'Ref' or 'Refs' (unless they were corrected from Reference/References) |
There was a problem hiding this comment.
It says “suffix,” but does that mean it doesn't allow fields named Ref or Refs?
There was a problem hiding this comment.
removed "suffix" as we are replacing instances of 'Reference' and 'References' anywhere across the field. beginning(prefix), middle, end
k8s-ci-robot
added
size/XL
docs/linters.md
Outdated
|
|
||
| The `references` linter ensures that field names use 'Ref'/'Refs' instead of 'Reference'/'References'. | ||
|
|
||
| By default, `references` is enabled and operates in strict mode, prohibiting the use of 'Reference', 'References', 'Ref', or 'Refs' anywhere in field names |
There was a problem hiding this comment.
Default should be to allow Ref/Refs, but not Reference/References
docs/linters.md
Outdated
| ``` | ||
|
|
||
| **Default behavior (policy: ForbidRefAndRefs):** | ||
| - Reports errors for fields containing 'Reference' or 'References' anywhere and suggests replacing with 'Ref' or 'Refs' |
There was a problem hiding this comment.
Why would it suggest to replace with something that it will then report is bad?
There was a problem hiding this comment.
Yikes, i guess I misunderstood the default policy to be forbidRefAndRefs. It's now allowRefAndRefs which makes sense because we want to allow Ref and Refs but not Reference and References as the default configuration option
| // NewAnalyzer creates a new analysis.Analyzer for the namingconventions | ||
| // linter based on the provided config. | ||
| func newAnalyzer(cfg *Config) *analysis.Analyzer { | ||
| // This function is exported to allow other linters to wrap namingconventions | ||
| // with fixed configuration. | ||
| func NewAnalyzer(cfg *Config) *analysis.Analyzer { |
There was a problem hiding this comment.
Are we certain this needs to be changed to exported? I thought we avoided that on the other PR
There was a problem hiding this comment.
Ahh nice catch. Guess we can avoid it here too!
pkg/analysis/references/analyzer.go
Outdated
| // Match "Refs" but not when part of "References" | ||
| namingconventions.Convention{ | ||
| Name: "forbid-refs", | ||
| ViolationMatcher: "Refs([^a-z]|$)", |
There was a problem hiding this comment.
Refs would not be part of references? I don't think the extra capture is needed here is it?
There was a problem hiding this comment.
this was part of forbidRefAndRefs policy
pkg/analysis/references/analyzer.go
Outdated
|
|
||
| // If policy is ForbidRefAndRefs, add conventions to forbid Ref/Refs anywhere | ||
| // Exclude patterns already handled by Reference/References above | ||
| if policy == PolicyForbidRefAndRefs { |
There was a problem hiding this comment.
I think it might make sense rather to append, to look into replace the list in this case (probably switch on the valid values?) and then look at how you could regex Reference Ref References and Refs` in a single regex for the drop
There was a problem hiding this comment.
Appending into a single regex for this makes so much sense!
There was a problem hiding this comment.
Agreed, a single regex makes sense here.
| var errs field.ErrorList | ||
|
|
||
| // Validate Policy enum if provided | ||
| if cfg.Policy != "" && cfg.Policy != PolicyAllowRefAndRefs && cfg.Policy != PolicyForbidRefAndRefs { |
There was a problem hiding this comment.
Better to use a switch here and have the default case error
| lintersConfig: | ||
| references: {} | ||
pkg/analysis/references/config.go
Outdated
|
|
||
| // Config represents the configuration for the references linter. | ||
| type Config struct { | ||
| // Policy controls whether Ref/Refs are allowed or forbidden in field names. |
There was a problem hiding this comment.
For consistency with other rules
| // Policy controls whether Ref/Refs are allowed or forbidden in field names. | |
| // policy controls whether Ref/Refs are allowed or forbidden in field names. |
| ConfigRefs []string `json:"configRefs"` // want `naming convention "references-to-refs": field ConfigReferences: field names should use 'Refs' instead of 'References'` | ||
|
|
||
| // Fields with Reference at beginning should be flagged | ||
| RefCount int `json:"referenceCount"` // want `naming convention "reference-to-ref": field ReferenceCount: field names should use 'Ref' instead of 'Reference'` |
There was a problem hiding this comment.
Should it not also have fixed the json?
There was a problem hiding this comment.
When testing locally, it's also fixing json. Latest commit has the new version of the file 👍🏻
pkg/analysis/references/analyzer.go
Outdated
| namingconventions.Convention{ | ||
| Name: "forbid-refs", | ||
| ViolationMatcher: "Refs([^a-z]|$)", | ||
| Operation: namingconventions.OperationInform, |
There was a problem hiding this comment.
Should this be a Drop rather than inform?
There was a problem hiding this comment.
I think we should just inform here because user wants to flag / forbidRefAndRefs they may not necessarily want to drop that substring itself.
For eg: We might not want NodeRef --> Node
dropping/swapping out ref(s) could be in scope for another story 👍🏻
There was a problem hiding this comment.
Ack, happy to have this as inform for now, will see if we get any feedback about it
There was a problem hiding this comment.
Not sure I agree here. I think intention behind forbidRefAndRefs is actually to cause something like NodeRef -> Node.
If we think there is a reasonable use case to inform instead of suggest fixes, maybe we follow configuration precedence of other linters to allow a Warn and SuggestFixes configuration for this linter and we default to SuggestFixes?
…der to make it case sensitive
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
docs/linters.md
Outdated
| - **Allows** fields containing 'Ref' or 'Refs' without reporting errors | ||
|
|
||
| **Strict mode (policy: ForbidRefAndRefs):** | ||
| - Reports errors for fields containing 'Reference' or 'References' anywhere and suggests replacing with 'Ref' or 'Refs' |
There was a problem hiding this comment.
This behaviour feels wrong still, suggesting to replace something that will then flag again?
There was a problem hiding this comment.
+1, this should be dropping of the offending text.
There was a problem hiding this comment.
@JoelSpeed @everettraven if y'all agree to dropping Ref/s too as offending text in ForbidRefAndRefs, can certainly make that change 👍🏻
docs/linters.md
Outdated
| - `NodeReferences` → `NodeRefs` | ||
|
|
||
| Note: In the default `ForbidRefAndRefs` mode, these fixes are intermediate - the linter will still report errors on the resulting 'Ref'/'Refs' patterns, indicating they should be removed entirely | ||
| Note: In the `ForbidRefAndRefs` mode, the fixes are intermediate. As, the linter will still report errors on the resulting 'Ref'/'Refs' patterns, indicating they should be removed entirely |
There was a problem hiding this comment.
I think we can avoid this
pkg/analysis/references/analyzer.go
Outdated
| conventions := []namingconventions.Convention{ | ||
| { | ||
| Name: "reference-to-ref", | ||
| ViolationMatcher: "(?i)reference(s?)", |
There was a problem hiding this comment.
Why do we make this case insensitive? Isn't that causing things like Preference to become Prefs, I don't think that's actually desired? I think we are better not having the case insensitivity aren't we?
There was a problem hiding this comment.
Hmm. That's a good question - I can't remember if removing the case sensitivity would mean we don't update the serialized form of the name correctly or not.
Would be good to double check that. If it still handles serialized name updates correctly, I agree we can put this back to being case sensitive.
Doing that means we do miss cases where the field name casing is incorrect though like Podreferences.
I wonder if adding a linter specifically for field name being PascalCase would make sense and consider something like Podreferences to be out of scope here?
There was a problem hiding this comment.
+1 @JoelSpeed i agree on making it case sensitive. Currently we are getting false positives as its converting Preference to Prefs which is documented as a known limitation.
@everettraven Circling back to our last discussion on PascalCase i still do feel that adding another linter for field name being PascalCase would make most sense.
We can possibly flag instances likePodreferences, referenceCount here to say field has invalid casing.
There was a problem hiding this comment.
@krishagarwal278 Could you verify that removing the case insensitivity results in successfully matching the json tags that look like reference* and *Reference?
pkg/analysis/references/analyzer.go
Outdated
| // If policy is ForbidRefAndRefs, also flag Ref/Refs as problematic (no fix provided) | ||
| // This creates a two-step hint: fix Reference→Ref, but know that Ref also needs changing | ||
| if policy == PolicyForbidRefAndRefs { | ||
| conventions = append(conventions, |
There was a problem hiding this comment.
I think this wants to replace, not append. That way we can avoid the intermediate steps.
Should be able to just have a single regex here I think
pkg/analysis/references/analyzer.go
Outdated
| namingconventions.Convention{ | ||
| Name: "forbid-refs", | ||
| ViolationMatcher: "Refs([^a-z]|$)", | ||
| Operation: namingconventions.OperationInform, |
There was a problem hiding this comment.
Ack, happy to have this as inform for now, will see if we get any feedback about it
| } | ||
|
|
||
| // buildConventions creates the naming conventions based on the policy. | ||
| func buildConventions(policy Policy) []namingconventions.Convention { |
There was a problem hiding this comment.
Try making this a switch on the policy, and returning a single convention from each branch of the switch, I think that will make the code more logical and the UX better
docs/linters.md
Outdated
| ```yaml | ||
| lintersConfig: | ||
| references: | ||
| policy: AllowRefAndRefs | ForbidRefAndRefs # Defaults to `AllowRefAndRefs`. |
There was a problem hiding this comment.
Probably more of a nitpick, but I wonder if something like PreferAbbreviatedReference and NoReference might be a bit more intuitive naming from the end-user perspective?
There was a problem hiding this comment.
Naming suggestion works for me
There was a problem hiding this comment.
Nit: I think @JoelSpeed i agreed on something like PreferAbbreviatedReference and NoReferences
docs/linters.md
Outdated
| - **Allows** fields containing 'Ref' or 'Refs' without reporting errors | ||
|
|
||
| **Strict mode (policy: ForbidRefAndRefs):** | ||
| - Reports errors for fields containing 'Reference' or 'References' anywhere and suggests replacing with 'Ref' or 'Refs' |
There was a problem hiding this comment.
+1, this should be dropping of the offending text.
pkg/analysis/references/analyzer.go
Outdated
| conventions := []namingconventions.Convention{ | ||
| { | ||
| Name: "reference-to-ref", | ||
| ViolationMatcher: "(?i)reference(s?)", |
There was a problem hiding this comment.
Hmm. That's a good question - I can't remember if removing the case sensitivity would mean we don't update the serialized form of the name correctly or not.
Would be good to double check that. If it still handles serialized name updates correctly, I agree we can put this back to being case sensitive.
Doing that means we do miss cases where the field name casing is incorrect though like Podreferences.
I wonder if adding a linter specifically for field name being PascalCase would make sense and consider something like Podreferences to be out of scope here?
pkg/analysis/references/analyzer.go
Outdated
|
|
||
| // If policy is ForbidRefAndRefs, add conventions to forbid Ref/Refs anywhere | ||
| // Exclude patterns already handled by Reference/References above | ||
| if policy == PolicyForbidRefAndRefs { |
There was a problem hiding this comment.
Agreed, a single regex makes sense here.
pkg/analysis/references/analyzer.go
Outdated
| namingconventions.Convention{ | ||
| Name: "forbid-refs", | ||
| ViolationMatcher: "Refs([^a-z]|$)", | ||
| Operation: namingconventions.OperationInform, |
There was a problem hiding this comment.
Not sure I agree here. I think intention behind forbidRefAndRefs is actually to cause something like NodeRef -> Node.
If we think there is a reasonable use case to inform instead of suggest fixes, maybe we follow configuration precedence of other linters to allow a Warn and SuggestFixes configuration for this linter and we default to SuggestFixes?
|
|
||
| ## References | ||
|
|
||
| The `references` linter ensures that field names use 'Ref'/'Refs' instead of 'Reference'/'References'. |
There was a problem hiding this comment.
It looks like we've established a pattern that for linters associated with a "don't do this" semantic we prefix with no.
I think staying consistent would be good, so I wonder if this should instead be noreference?
@JoelSpeed what are your thoughts here?
There was a problem hiding this comment.
Yeah I think that makes sense, noreference or noreferences
There was a problem hiding this comment.
The only pushback I can think of on that, is that the others are forbidding the usage of certain types. Which means this one forbidding just the naming might be a bit different? Does it imply that you aren't allowed reference types vs fields with references in the name?
There was a problem hiding this comment.
We have nophase and notimestamp which are naming conventions
5 participants
Added a new references linter to enforce that API field names use Ref/Refs suffixes instead of Reference/References.
What it does:
Fixes #25