Add statusoptional linter #73

yongruilin · 2025-04-29T21:12:04Z

This PR introduces a new linter, statusoptional, which checks that all first-level children fields within a status struct are marked as optional. It adds functionality to automatically suggest fixes for fields that are missing the appropriate markers.

Fix #31

JoelSpeed

In the optionalorrequired linter, we give the user a choice of which optional tag they prefer, we probably want to do the same here so that the suggested fix uses their preferred marker, right?

pkg/analysis/statusoptional/doc.go

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

pkg/analysis/statusoptional/testdata/src/a/a.go.golden

pkg/analysis/statusoptional/analyzer.go

JoelSpeed · 2025-05-01T09:59:21Z

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

+	// +kubebuilder:validation:Optional
+	Reason string `json:"reason"`
+
+	State `json:",inline"` // want "status field \"State\" must be marked as optional"


This isn't correct. You can't mark an embedded field as optional, it would have no effect on the openapi schema.

The child of this embedded field however should be marked as optional, which I think you have already

A go embed withjson:,inline cannot be optional, but a non-inlined go embed can (embed must be a pointer). Today we embed ObjectMeta but don't inline it and tag it with json:metadata,omitempty. ObjectMeta is not a great example because it cannot be empty, but a pointer embed struct can.

if you want full test coverage it's quite a few cases: Embed inlined, embed non-inlined (omit empty is allowed but doesn't do anything), pointer embed non-inlined, pointer embed non-inlined omitempty. On and we've got omitzero support coming so: embed non-inlined omitzero and pointer embed non-inlined omitzero too..

Added more test cases.

JoelSpeed · 2025-05-01T09:59:50Z

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

+}
+
+// MyResourceStatus is a sample status with a mix of optional and non-optional fields.
+type MyResourceStatus struct {


Please add an optional struct field to this type that then has required members within it. The linter shouldn't report anything for the members of the new struct as they would be second level

Added a test case for it.

JoelSpeed · 2025-05-01T10:00:52Z

@yongruilin just wanted to check if you saw #73 (review)

pkg/analysis/statusoptional/analyzer.go

sivchari · 2025-05-02T11:51:37Z

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

+}
+
+// MyResourceStatus is a sample status with a mix of optional and non-optional fields.
+type MyResourceStatus struct {


When the following field is added to struct

// +required RequiredField string `json:"requiredField"`

The diagnostic works fine, but the suggesting results in followng.

//+required //+optional RequiredField string `json"requiredField"`

Fixed code can compile, but the schema doesn't make sense. So we should handle it if the filed has already required field.

Good catch, I updated to handle this case.

yongruilin · 2025-05-06T22:19:16Z

@yongruilin just wanted to check if you saw #73 (review)

Apologize, I was a bit tied up last week.
I should've address the comments. PTAL.

sivchari · 2025-05-08T04:45:10Z

pkg/analysis/statusoptional/analyzer.go

+	// OptionalMarker is the marker that indicates that a field is optional.
+	OptionalMarker = "optional"
+
+	// KubebuilderOptionalMarker is the marker that indicates that a field is optional in kubebuilder.
+	KubebuilderOptionalMarker = "kubebuilder:validation:Optional"
+
+	// RequiredMarker is the marker that indicates that a field is required.
+	RequiredMarker = "required"
+
+	// KubebuilderRequiredMarker is the marker that indicates that a field is required in kubebuilder.
+	KubebuilderRequiredMarker = "kubebuilder:validation:Required"


Suggested change

// OptionalMarker is the marker that indicates that a field is optional.

OptionalMarker = "optional"

// KubebuilderOptionalMarker is the marker that indicates that a field is optional in kubebuilder.

KubebuilderOptionalMarker = "kubebuilder:validation:Optional"

// RequiredMarker is the marker that indicates that a field is required.

RequiredMarker = "required"

// KubebuilderRequiredMarker is the marker that indicates that a field is required in kubebuilder.

KubebuilderRequiredMarker = "kubebuilder:validation:Required"

You can delete these entirely, then suggest to use markers constant instead.

https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/markers/markers.go

Updated, is it recently added?

debug.test89409974

JoelSpeed · 2025-05-08T11:36:29Z

pkg/analysis/statusoptional/analyzer_test.go

+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.RunWithSuggestedFixes(t, testdata, newAnalyzer("optional"), "a")


Should add a test case for the alternative optional marker too, should be fairly simple to copy the existing cases across

Added the test cases for kubebuilder optional and k8s:optional.

pkg/analysis/statusoptional/testdata/src/a/a.go.golden

pkg/config/linters_config.go

pkg/analysis/statusoptional/analyzer.go

JoelSpeed · 2025-05-08T11:46:17Z

pkg/analysis/statusoptional/analyzer.go

+		// Handle standard required markers
+		if fieldMarkers.Has(RequiredMarker) {
+			for _, marker := range fieldMarkers[RequiredMarker] {
+				textEdits = append(textEdits, analysis.TextEdit{
+					Pos: marker.Pos,
+					End: marker.End,
+					// Remove the marker completely (will add preferred one separately)
+					NewText: []byte(""),
+				})
+			}
+		}


In other packages we have helpers for removing the markers, perhaps we can copy that pattern and extract this to a helper to reduce the duplication between these lines and the block below

Left a TODO on it. I prefer to have a separate PR to consolidate the usage.

JoelSpeed · 2025-05-09T06:54:05Z

pkg/config/linters_config.go

+type StatusOptionalConfig struct {
+	// preferredOptionalMarker is the preferred marker to use for optional fields.
+	// If this field is not set, the default value is "optional".
+	// Valid values are "optional" and "kubebuilder:validation:Optional".


And the K8s version now right? Or do you not want to include that in the documentation until the declarative validation project is further along?

JoelSpeed · 2025-05-09T06:54:20Z

pkg/validation/linters_config.go

+	fieldErrors := field.ErrorList{}
+
+	switch soc.PreferredOptionalMarker {
+	case "", markers.OptionalMarker, markers.KubebuilderOptionalMarker:


As above, should this include the K8s version?

JoelSpeed · 2025-05-09T16:03:34Z

pkg/validation/linters_config.go

+	switch soc.PreferredOptionalMarker {
+	case "", markers.OptionalMarker, markers.KubebuilderOptionalMarker, markers.K8sOptionalMarker:
+	default:
+		fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("preferredOptionalMarker"), soc.PreferredOptionalMarker, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", markers.OptionalMarker, markers.KubebuilderOptionalMarker)))


This line needs updating to include the K8s version of the optional

This commit introduces a new linter, `statusoptional`, which checks that all first-level children fields within a status struct are marked as optional. It adds functionality to automatically suggest fixes for fields that are missing the appropriate markers.

JoelSpeed · 2025-05-09T16:16:54Z

I think this is looking all good and correct now, @yongruilin are you happy with it?

yongruilin · 2025-05-09T16:45:11Z

Yea, it looks good to me as well now.

yongruilin · 2025-05-14T15:54:33Z

@JoelSpeed anything else in your mind for this PR?

JoelSpeed · 2025-05-14T15:59:32Z

/lgtm
/approve

k8s-ci-robot · 2025-05-14T15:59:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, yongruilin

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:

~~OWNERS~~ [JoelSpeed]

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

k8s-ci-robot requested review from JoelSpeed and jpbetz April 29, 2025 21:12

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 Apr 29, 2025

yongruilin force-pushed the statusoptional branch 3 times, most recently from 357ccbf to 22348c8 Compare April 29, 2025 22:48

JoelSpeed reviewed Apr 30, 2025

View reviewed changes

yongruilin force-pushed the statusoptional branch 4 times, most recently from 2f69191 to 10f15c4 Compare April 30, 2025 22:42

JoelSpeed reviewed May 1, 2025

View reviewed changes

sivchari reviewed May 2, 2025

View reviewed changes

yongruilin force-pushed the statusoptional branch 2 times, most recently from 14cf805 to 025fc57 Compare May 6, 2025 22:10

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 May 6, 2025

yongruilin force-pushed the statusoptional branch 2 times, most recently from 3db7fa8 to 511c165 Compare May 6, 2025 22:16

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

sivchari reviewed May 8, 2025

View reviewed changes

JoelSpeed reviewed May 8, 2025

View reviewed changes

yongruilin force-pushed the statusoptional branch from 511c165 to c19a8cc Compare May 8, 2025 21:48

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

JoelSpeed reviewed May 9, 2025

View reviewed changes

yongruilin force-pushed the statusoptional branch from c19a8cc to f930ac2 Compare May 9, 2025 15:55

JoelSpeed reviewed May 9, 2025

View reviewed changes

yongruilin force-pushed the statusoptional branch from f930ac2 to 50f7599 Compare May 9, 2025 16:07

Add statusoptional linter

60896da

This commit introduces a new linter, `statusoptional`, which checks that all first-level children fields within a status struct are marked as optional. It adds functionality to automatically suggest fixes for fields that are missing the appropriate markers.

yongruilin force-pushed the statusoptional branch from 50f7599 to 60896da Compare May 9, 2025 16:08

k8s-ci-robot assigned JoelSpeed May 14, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2025

k8s-ci-robot merged commit 25b7231 into kubernetes-sigs:main May 14, 2025
4 checks passed

Add statusoptional linter #73

Add statusoptional linter #73

Uh oh!

Conversation

yongruilin commented Apr 29, 2025

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpbetz May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JoelSpeed commented May 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongruilin commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

JoelSpeed commented May 9, 2025

Uh oh!

yongruilin commented May 9, 2025

Uh oh!

yongruilin commented May 14, 2025

Uh oh!

JoelSpeed commented May 14, 2025

Uh oh!

k8s-ci-robot commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

jpbetz May 1, 2025 •

edited

Loading