New Linter: Dependent tag #168

yongruilin · 2025-10-28T22:57:52Z

This PR introduces a new dependenttags linter to enforce dependencies between markers, preventing API inconsistencies where one marker requires another.

The linter is configurable with rules and supports requiring either all (default) or any of the specified dependent markers.

Fixes #146

k8s-ci-robot · 2025-10-28T22:57:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yongruilin
Once this PR has been reviewed and has the lgtm label, please assign jpbetz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

docs/linters.md

pkg/analysis/dependenttags/analyzer.go

docs/linters.md

JoelSpeed · 2025-10-29T10:23:02Z

docs/linters.md

+      - main: "listType"
+        dependents:
+          - "k8s:listType"


When there are dependencies, how do values work? Can this linter fix dependencies? If I want the same value between these two, is that achievable? If I wanted different values, is that achievable?

Values are ignored: The linter only checks if a marker like +listType exists; it does not care about its value (e.g., =map).

No automatic fixes: The linter will report that a dependent marker is missing, but it cannot automatically add it for you.

Value matching is not supported: Enforcing that two markers have the same or different values is outside the scope of this linter.

I have updated the docs/linters.md file with a "Behavior" section to make this clear.

docs/linters.md

pkg/analysis/dependenttags/analyzer.go

JoelSpeed · 2025-10-29T10:34:05Z

pkg/analysis/dependenttags/config.go

+// defaultConfig is the default configuration for the dependenttags linter.
+var defaultConfig = Config{
+	Rules: []Rule{},
+}


Since this is an empty config, do we really need it?

pkg/analysis/dependenttags/initializer.go

JoelSpeed · 2025-10-29T10:35:01Z

pkg/analysis/dependenttags/initializer.go

+	}
+
+	rulesPath := fldPath.Child("rules")
+	for i, rule := range cfg.Rules {


Should be an error to have no rules

pkg/analysis/dependenttags/initializer.go

Introduces a new configurable linter named `dependenttags` to enforce dependencies between markers. This linter ensures that if a main marker is present on a field, a set of dependent markers are also present, preventing API inconsistencies. The linter supports two dependency types: - `All`: Requires all dependent markers to be present. - `Any`: Requires at least one of the dependent markers to be present. This addresses issue kubernetes-sigs#146.

Adds documentation for the new `dependenttags` linter to `docs/linters.md`. This includes a description of the linter, its configuration options, and examples of how to use it.

JoelSpeed

Please ensure we consistently updated all/any to All/Any

I think it would be useful (especially for the listType case) to be able to say "this identifier with this value relies on this identifier"

What if we changed the config to

- identifier: ...
  payload: ... # optional
  dependents:
  - identifier: ...
    payload: ... # optional

And then you would be able to say that if it has a specified payload, then the dependents are either requiring just the identifier, or the identifier with a particular payload

This could become a bit more complex with DV parsing though, not sure how arguments will fit into this CC @everettraven

JoelSpeed · 2025-10-30T16:46:25Z

docs/linters.md

+
+This linter only checks for the presence or absence of markers; it does not inspect or enforce specific values within those markers. Therefore:
+
+- **Values:** The linter does not care about the values of the `identifier` or `dependent` markers. It only verifies if the markers themselves are present.


+listType=map needs +listMapKey=..., that cannot be represented here then? Feels like that could be something that is useful

It can, you can directly put identifier as +listType=map

Please make sure there are test cases for this

JoelSpeed · 2025-10-30T16:47:45Z

pkg/analysis/dependenttags/testdata/src/a/a.go

+
+type Union struct {
+	// +k8s:unionMember
+	InvalidMember string // want "field with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"


We should include the field name, and ideally struct name in the error message

Suggested change

InvalidMember string // want "field with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"

InvalidMember string // want "field Union.InvalidMember with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"

JoelSpeed · 2025-10-30T16:50:14Z

pkg/analysis/dependenttags/config.go

+
+const (
+	// DependencyTypeAll indicates that all dependent markers are required.
+	DependencyTypeAll DependencyType = "all"


Suggested change

DependencyTypeAll DependencyType = "all"

DependencyTypeAll DependencyType = "All"

pkg/analysis/dependenttags/config.go

docs/linters.md

JoelSpeed · 2025-10-30T16:52:54Z

pkg/analysis/dependenttags/initializer_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			initializer := dependenttags.Initializer()
+			errs := initializer.ValidateConfig(tc.cfg, field.NewPath(""))
+
+			if tc.expectErr && len(errs) == 0 {
+				t.Errorf("expected validation errors, but got none")
+			}
+
+			if !tc.expectErr && len(errs) > 0 {
+				t.Errorf("unexpected validation errors: %v", errs)
+			}
+
+			if !tc.expectErr {
+				if _, err := initializer.Init(tc.cfg); err != nil {
+					t.Errorf("unexpected error initializing analyzer: %v", err)
+				}
+			}
+		})
+	}


The rest of our tests use gomega for assertions, worth doing the same here?

JoelSpeed · 2025-10-30T16:54:03Z

pkg/analysis/dependenttags/initializer.go

+		}
+
+		if rule.Type == "" {
+			errs = append(errs, field.Required(rulesPath.Index(i).Child("type"), "type must be explicitly set to 'all' or 'any'"))


Use a format string with the actual constant values please

JoelSpeed · 2025-10-30T16:54:35Z

pkg/analysis/dependenttags/initializer.go

+			case DependencyTypeAll, DependencyTypeAny:
+				// valid
+			default:
+				errs = append(errs, field.Invalid(rulesPath.Index(i).Child("type"), rule.Type, "type must be 'all' or 'any'"))


Doesn't invalid take a list of expected valid values?

It seems not. I've updated to use format string in error detail.

…standardize type values

k8s-ci-robot requested review from JoelSpeed and everettraven October 28, 2025 22:57

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2025

sivchari reviewed Oct 29, 2025

View reviewed changes

docs/linters.md Show resolved Hide resolved

pkg/analysis/dependenttags/analyzer.go Outdated Show resolved Hide resolved

pkg/analysis/dependenttags/analyzer.go Outdated Show resolved Hide resolved

pkg/analysis/dependenttags/analyzer.go Outdated Show resolved Hide resolved

JoelSpeed reviewed Oct 29, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2025

yongruilin force-pushed the dependent-tag branch from c549928 to 34fe862 Compare October 29, 2025 21:00

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2025

yongruilin force-pushed the dependent-tag branch 2 times, most recently from 547d0ff to c1dcbd1 Compare October 29, 2025 21:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2025

yongruilin added 2 commits October 29, 2025 21:23

docs(linter): add documentation for dependenttags

a5a9a02

Adds documentation for the new `dependenttags` linter to `docs/linters.md`. This includes a description of the linter, its configuration options, and examples of how to use it.

yongruilin force-pushed the dependent-tag branch from c1dcbd1 to a5a9a02 Compare October 29, 2025 21:23

JoelSpeed reviewed Oct 30, 2025

View reviewed changes

Address comment: enhance error messages with struct and field names; …

81de26c

…standardize type values


		This linter only checks for the presence or absence of markers; it does not inspect or enforce specific values within those markers. Therefore:

		- Values: The linter does not care about the values of the `identifier` or `dependent` markers. It only verifies if the markers themselves are present.

	InvalidMember string // want "field with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"
	InvalidMember string // want "field Union.InvalidMember with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"

	DependencyTypeAll DependencyType = "all"
	DependencyTypeAll DependencyType = "All"

New Linter: Dependent tag #168

Are you sure you want to change the base?

New Linter: Dependent tag #168

Conversation

yongruilin commented Oct 28, 2025

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants