Added enums linter #180

krishagarwal278 · 2025-11-05T06:08:33Z

Description

This PR introduces a new enums linter to enforce best practices for enumerated fields in Kubernetes APIs.

The new linter, enums, achieves the following:

Documents Enum Fields: Enforces use of type aliases with +enum markers instead of plain strings. Ensures enum values are explicitly documented in OpenAPI schemas with clear justifications for their purpose.
Prevents Regressions: Validates that enum fields match documented patterns. Prevents undocumented or accidental changes to enum naming conventions and type definitions.
Improves API Quality: Enforces PascalCase naming for enum values (e.g., "PhasePending"). Ensures thoughtful consideration for enum field design, improving schema validation and generated client type safety.
The linter is disabled by default and supports an allowlist for special cases like command-line tool names.

Fixes #17

krishagarwal278 · 2025-11-07T14:57:11Z

/label tide/merge-method-squash

JoelSpeed

Please review the kubebuilder validations documentation for how the kubebuilder version of the enum marker works, this PR is not handling it correctly at the moment

JoelSpeed · 2025-11-07T15:39:49Z

docs/linters.md

+
+This provides better API evolution, self-documentation, and validation compared to plain strings.
+
+By default, `enums` is not enabled.


It probably should be

JoelSpeed · 2025-11-07T15:40:11Z

docs/linters.md

 The `duplicatemarkers` linter can automatically fix all markers that are exact match to another markers.
 If there are duplicates across fields and their underlying type, the marker on the type will be preferred and the marker on the field will be removed.

+## Enums


We are likely to need configuration on this linter for accepted exceptions to the rules here

JoelSpeed · 2025-11-07T15:41:54Z

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

+package a
+
+// Valid enum type with proper marker
+// +kubebuilder:validation:Enum


This isn't actually equivalent to +enum. +enum will pick up constants for the type, this marker requires them to be specified explicitly

+kubebuilder:validation:Enum=Pending;Running;Succeeded;Failed

JoelSpeed · 2025-11-07T15:42:45Z

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

+)
+
+// Alternative marker format
+// +k8s:enum


Does this exist already? I wasn't sure if DV was at this point

yes it does: https://kubernetes.io/docs/reference/using-api/declarative-validation/#tag-enum

JoelSpeed · 2025-11-07T15:43:16Z

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

+)
+
+// Invalid: Missing +enum marker
+// This type doesn't have +enum marker, so it will be flagged when used in fields


What triggers this to be flag? A string type alias with some constants assigned?

JoelSpeed · 2025-11-07T16:31:23Z

pkg/analysis/enums/analyzer.go

+}
+
+// isPascalCase checks if a string follows PascalCase naming convention.
+func isPascalCase(s string) bool {


I suspect checking pascal case is quite hard. What is the logic here trying to check? First character is capital and that not all characters are upper? All characters upper may actually be valid (e.g. HTTP or HTTPS would be valid)

JoelSpeed · 2025-11-07T16:33:46Z

pkg/analysis/enums/doc.go

+1. Fields must use type aliases, not plain strings: String fields that represent enums should
+use a type alias with an +enum marker rather than a raw string type.


This is and isn't correct. If you see +kubebuilder:validation:Enum on a field, and it's using a plain string, recommending to use a type alias is valid.

For a generic string, this is not true

JoelSpeed · 2025-11-07T16:34:28Z

pkg/analysis/enums/doc.go

+2. Type aliases must have +enum marker: Type aliases used for enumerated values must be
+annotated with either // +kubebuilder:validation:Enum or // +k8s:enum.


Only if we think they should be enums, which means they have a number of constants associated. There may be use cases for free-form strings

JoelSpeed · 2025-11-07T16:34:55Z

pkg/analysis/enums/doc.go

+2. Type aliases must have +enum marker: Type aliases used for enumerated values must be
+annotated with either // +kubebuilder:validation:Enum or // +k8s:enum.
+3. Enum values must be PascalCase: Constant values for enums should follow PascalCase naming
+(e.g., "PhasePending", "StateRunning") rather than lowercase, snake_case, or SCREAMING_SNAKE_CASE.


TIL that SCREAMING_SNAKE_CASE is a thing 😂

😂😂😂

JoelSpeed · 2025-11-07T16:35:45Z

pkg/analysis/enums/doc.go

+Note: This linter is disabled by default as enum usage is recommended but not strictly
+required for all Kubernetes APIs. Enable it in your configuration to enforce these conventions.


If we get this linter right, it should be on by default IMO

linter now enabled by default so updated/fixed failing integration tests 👍🏻

modified: docs/linters.md new file: pkg/analysis/enums/analyzer.go new file: pkg/analysis/enums/analyzer_test.go new file: pkg/analysis/enums/config.go new file: pkg/analysis/enums/doc.go new file: pkg/analysis/enums/initializer.go new file: pkg/analysis/enums/testdata/src/a/a.go new file: pkg/analysis/enums/testdata/src/b/b.go modified: pkg/registration/doc.go

… the file

…nd other reviews

k8s-ci-robot · 2025-11-11T19:52:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: krishagarwal278
Once this PR has been reviewed and has the lgtm label, please assign everettraven 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

JoelSpeed · 2025-11-11T22:43:45Z

docs/linters.md

+- **Declarative Validation Marker** (`+k8s:enum`): Used in Kubernetes core API types for declarative validation.
+
+**Enum Marker Modes:**
+- **Auto-discovery** (`+k8s:enum` or `+kubebuilder:validation:Enum`): Validates that constants follow PascalCase


The kubebuilder version doesn't work like this, it must have explicit values

JoelSpeed · 2025-11-11T22:44:08Z

docs/linters.md

+
+**Enum Marker Types:**
+- **CRD Validation Marker** (`+kubebuilder:validation:Enum`): Used for CRD validation in projects with CustomResourceDefinitions. Processed by controller-gen to generate OpenAPI schema validation.
+- **Declarative Validation Marker** (`+k8s:enum`): Used in Kubernetes core API types for declarative validation.


What about plan +enum?

JoelSpeed · 2025-11-11T22:45:32Z

docs/linters.md

+    allowlist:
+      - kubectl
+      - docker
+    # When true, flags plain string fields (default: false)


Probably should default to true, I think this is preferred in Kube API conventions over strings with other validation

JoelSpeed · 2025-11-12T15:59:33Z

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

@@ -0,0 +1,141 @@
+package a
+
+// Valid: +kubebuilder:validation:Enum without explicit values (auto-discovers constants)


This comment is not accurate, as you have constants?

JoelSpeed · 2025-11-12T16:00:30Z

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

+)
+
+// Valid: +kubebuilder:validation:Enum with explicit values (doesn't check constants)
+// +kubebuilder:validation:Enum=Pending;helper


helper should be flagged here as it isn't pascal case

JoelSpeed · 2025-11-12T16:12:56Z

pkg/analysis/enums/doc.go

+
+Good:
+
+	// +kubebuilder:validation:Enum


Suggested change

// +kubebuilder:validation:Enum

// +enum

JoelSpeed · 2025-11-12T16:15:39Z

pkg/analysis/enums/initializer.go

+
+// validateConfig implements validation of the enums linter config.
+func validateConfig(cfg *Config, fldPath *field.Path) field.ErrorList {
+	// Config is optional, allowlist can be empty


We probably want to validate that the allowlist does not contain duplicates

JoelSpeed · 2025-11-12T16:16:43Z

pkg/analysis/enums/config.go

+	// RequireTypeAliasForEnums when true, enforces that string fields representing enums
+	// must use type aliases instead of plain string types.
+	// Default: false (plain strings are allowed)
+	RequireTypeAliasForEnums bool `yaml:"requireTypeAliasForEnums" json:"requireTypeAliasForEnums"`


We try to avoid bools in favour of descriptive enum values, so that we can evolve in the future if we need to.

Maybe this needs to be KubebuilderEnumPolicy: RequireTypeAlias | AllowPlainString?

JoelSpeed · 2025-11-12T16:17:23Z

tests/integration/testdata/default_configurations/affinity.go

I don't think this is an accurate set of changes. I think these should be reverted.

JoelSpeed · 2025-11-12T16:18:21Z

tests/integration/testdata/default_configurations/container.go

The purpose of the integration tests is not to change the content of the files, but add extra want statements when there are new issues. Please revert the changes you've made here and instead add the newly wanted issues

JoelSpeed · 2025-11-12T16:26:10Z

pkg/analysis/enums/analyzer.go

+//
+// Returns false for:
+//   - +kubebuilder:validation:Enum=value1;value2 (CRD validation marker with explicit values)
+func usesAutoDiscovery(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool {


We provide markerAccess to get markers directly, you don't need to re-parse them here

JoelSpeed · 2025-11-12T16:27:08Z

pkg/analysis/enums/analyzer.go

+// isPascalCase checks if a string follows PascalCase naming convention.
+// Allows: PascalCase (Running), all-uppercase acronyms (HTTP, API), single letters (A).
+// Rejects: lowercase start (running), snake_case (phase_pending), kebab-case (phase-pending).
+func isPascalCase(s string) bool {


Probably best to use a regex here rather than manually parsing

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2025

k8s-ci-robot requested review from jpbetz and sivchari November 5, 2025 06:08

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 5, 2025

krishagarwal278 force-pushed the enums branch 2 times, most recently from fa477bf to a76ac80 Compare November 6, 2025 21:17

krishagarwal278 changed the title ~~[WIP]: Added enums linter~~ Added enums linter Nov 7, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2025

JoelSpeed reviewed Nov 7, 2025

View reviewed changes

krishagarwal278 added 4 commits November 10, 2025 14:09

Simplified loops, switches, and conditionals. Cleaned comments

a6d6e1d

Updated lint failures

6f6da92

Added/removed extra comments, moved helper functions at the bottom of…

4822af7

… the file

krishagarwal278 force-pushed the enums branch from 10b6e09 to 4822af7 Compare November 10, 2025 19:09

Declarative and CRD validation markers, default=true, updated docs, a…

f3cc283

…nd other reviews

JoelSpeed reviewed Nov 12, 2025

View reviewed changes


		This provides better API evolution, self-documentation, and validation compared to plain strings.

		By default, `enums` is not enabled.

		1. Fields must use type aliases, not plain strings: String fields that represent enums should
		use a type alias with an +enum marker rather than a raw string type.

		2. Type aliases must have +enum marker: Type aliases used for enumerated values must be
		annotated with either // +kubebuilder:validation:Enum or // +k8s:enum.

		Note: This linter is disabled by default as enum usage is recommended but not strictly
		required for all Kubernetes APIs. Enable it in your configuration to enforce these conventions.

		@@ -0,0 +1,141 @@
		package a

		// Valid: +kubebuilder:validation:Enum without explicit values (auto-discovers constants)


		Good:

		// +kubebuilder:validation:Enum

Added enums linter #180

Are you sure you want to change the base?

Added enums linter #180

Uh oh!

Conversation

krishagarwal278 commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

krishagarwal278 commented Nov 7, 2025

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!

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!

k8s-ci-robot commented Nov 11, 2025

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!

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!

3 participants

krishagarwal278 commented Nov 5, 2025 •

edited

Loading