-
Notifications
You must be signed in to change notification settings - Fork 24
Add Kubernetes API Linter to kueue-operator #1282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| version: "2" | ||
| run: | ||
| allow-parallel-runners: true | ||
| linters: | ||
| default: none | ||
| enable: | ||
| - kubeapilinter # linter for Kube API conventions | ||
| settings: | ||
| custom: | ||
| kubeapilinter: | ||
| type: module | ||
| description: KAL is the Kube-API-Linter and lints Kube like APIs based on API conventions and best practices. | ||
| settings: | ||
| linters: | ||
| enable: | ||
| - "commentstart" # Ensure comments start with the serialized version of the field name. | ||
| - "conditions" # Ensure conditions have the correct json tags and markers. | ||
| - "conflictingmarkers" | ||
| - "duplicatemarkers" # Ensure there are no exact duplicate markers. for types and fields. | ||
| - "integers" # Ensure only int32 and int64 are used for integers. | ||
| - "jsontags" # Ensure every field has a json tag. | ||
| - "maxlength" # Ensure all strings and arrays have maximum lengths/maximum items. | ||
| - "nobools" # Bools do not evolve over time, should use enums instead. | ||
| - "nodurations" # Prevents usage of `Duration` types. | ||
| - "nofloats" # Ensure floats are not used. | ||
| - "nomaps" # Ensure maps are not used. | ||
| - "nonullable" # Ensure that types and fields do not have the nullable marker. | ||
| - "notimestamp" # Prevents usage of 'Timestamp' fields | ||
| - "optionalfields" # Ensure that all fields marked as optional adhere to being pointers and | ||
| # having the `omitempty` value in their `json` tag where appropriate. | ||
| - "optionalorrequired" # Every field should be marked as `+optional` or `+required`. | ||
| - "requiredfields" # Required fields should not be pointers, and should not have `omitempty`. | ||
| - "ssatags" # Ensure array fields have the appropriate listType markers | ||
| - "statusoptional" # Ensure all first children within status should be optional. | ||
| - "statussubresource" # All root objects that have a `status` field should have a status subresource. | ||
| - "uniquemarkers" # Ensure that types and fields do not contain more than a single definition of a marker that should only be present once. | ||
| - "nophase" # Phase fields are discouraged by the Kube API conventions, use conditions instead. | ||
|
|
||
| # Linters below this line are disabled, pending conversation on how and when to enable them. | ||
| disable: | ||
| - "*" # We will manually enable new linters after understanding the impact. Disable all by default. | ||
| lintersConfig: | ||
| conflictingmarkers: | ||
| conflicts: | ||
| - name: "default_vs_required" | ||
| sets: | ||
| - ["default", "kubebuilder:default"] | ||
| - ["required", "kubebuilder:validation:Required", "k8s:required"] | ||
| description: "A field with a default value cannot be required" | ||
| conditions: | ||
| isFirstField: Warn # Require conditions to be the first field in the status struct. | ||
| usePatchStrategy: Ignore # Ignore patchStrategy markers on the Conditions field. | ||
| useProtobuf: Forbid # We don't use protobuf, so protobuf tags are not required. | ||
| optionalfields: | ||
| pointers: | ||
| preference: WhenRequired # Always | WhenRequired # Whether to always require pointers, or only when required. Defaults to `Always`. | ||
| policy: SuggestFix # SuggestFix | Warn # The policy for pointers in optional fields. Defaults to `SuggestFix`. | ||
| omitempty: | ||
| policy: SuggestFix # SuggestFix | Warn | Ignore # The policy for omitempty in optional fields. Defaults to `SuggestFix`. | ||
|
|
||
| exclusions: | ||
| generated: strict | ||
| rules: | ||
| ## KAL should only run on API folders. | ||
| - path-except: "pkg/apis" | ||
| linters: | ||
| - kubeapilinter | ||
| ## Breaking changes: These fields would need to be pointers but changing them is a breaking API change. | ||
| ## KueueConfiguration optional fields should be pointers | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "optionalfields: field KueueConfiguration.WorkloadManagement" | ||
| linters: | ||
| - kubeapilinter | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "optionalfields: field KueueConfiguration.GangScheduling" | ||
| linters: | ||
| - kubeapilinter | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "optionalfields: field KueueConfiguration.Preemption" | ||
| linters: | ||
| - kubeapilinter | ||
| ## Required fields with valid zero values should be pointers | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "requiredfields: field GangScheduling.Policy" | ||
| linters: | ||
| - kubeapilinter | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "requiredfields: field ByWorkload.Admission" | ||
| linters: | ||
| - kubeapilinter | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "requiredfields: field WorkloadManagement.LabelPolicy" | ||
| linters: | ||
| - kubeapilinter | ||
| - path: "pkg/apis/kueueoperator/v1/types.go" | ||
| text: "requiredfields: field Preemption.PreemptionPolicy" | ||
| linters: | ||
| - kubeapilinter | ||
|
|
||
| issues: | ||
| max-same-issues: 0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| version: v2.7.2 | ||
| name: golangci-lint-kube-api-linter | ||
| destination: ./bin | ||
| plugins: | ||
| - module: 'sigs.k8s.io/kube-api-linter' | ||
| version: v0.0.0-20251208100930-d3015c953951 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,19 +24,19 @@ type Kueue struct { | |
|
|
||
| // spec holds user settable values for configuration | ||
| // +required | ||
| Spec KueueOperandSpec `json:"spec"` | ||
| Spec KueueOperandSpec `json:"spec,omitzero"` | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any concerns on breaking changes for omitzero or omitempty? I don't have any CRD changes but I guess its worth asking if there could be any UX issues with adding these tags. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is going to be safe. Looking through, I can see that the empty marshalled spec object would have been rejected by the fact you have at least 1 required field that doesn't permit its empty value in the spec. So previously a user who set nothing, would have marshalled the empty object, and got a rejection. Now, they wouldn't marshal anything, and would still get a rejection
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Joel! |
||
| // status holds observed values from the cluster. | ||
| // They may not be overridden. | ||
| // +optional | ||
| Status KueueStatus `json:"status,omitempty"` | ||
| Status KueueStatus `json:"status,omitzero,omitempty"` | ||
| } | ||
|
|
||
| type KueueOperandSpec struct { | ||
| operatorv1.OperatorSpec `json:",inline"` | ||
| // config is the desired configuration | ||
| // for the Kueue operator. | ||
| // +required | ||
| Config KueueConfiguration `json:"config"` | ||
| Config KueueConfiguration `json:"config,omitzero"` | ||
| } | ||
|
|
||
| type KueueConfiguration struct { | ||
|
|
@@ -45,7 +45,7 @@ type KueueConfiguration struct { | |
| // known as external frameworks. | ||
| // Kueue will only manage workloads that correspond to the specified integrations. | ||
| // +required | ||
| Integrations Integrations `json:"integrations"` | ||
| Integrations Integrations `json:"integrations,omitzero"` | ||
| // workloadManagement controls how Kueue manages workloads. | ||
| // By default Kueue will manage workloads that have a queue-name label. | ||
| // Workloads that are missing the queue-name will be ignored by Kueue. | ||
|
|
@@ -133,7 +133,7 @@ type ExternalFramework struct { | |
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')" | ||
| // +required | ||
| Group string `json:"group"` | ||
| Group string `json:"group,omitempty"` | ||
| // resource is the Resource type of the external framework. | ||
| // Resource types are lowercase and plural (e.g. pods, deployments). | ||
| // Must be a valid DNS 1123 label consisting of a lower-case alphanumeric string | ||
|
|
@@ -144,7 +144,7 @@ type ExternalFramework struct { | |
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?$')" | ||
| // +required | ||
| Resource string `json:"resource"` | ||
| Resource string `json:"resource,omitempty"` | ||
| // version is the version of the api (e.g. v1alpha1, v1beta1, v1). | ||
| // Must be a valid DNS 1035 label consisting of a lower-case alphanumeric string | ||
| // and hyphens of at most 63 characters in length. | ||
|
|
@@ -154,7 +154,7 @@ type ExternalFramework struct { | |
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?$')" | ||
| // +required | ||
| Version string `json:"version"` | ||
| Version string `json:"version,omitempty"` | ||
| } | ||
|
|
||
| // This is the integrations for Kueue. | ||
|
|
@@ -171,7 +171,7 @@ type Integrations struct { | |
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="each item in frameworks must be unique" | ||
| // +listType=set | ||
| // +required | ||
| Frameworks []KueueIntegration `json:"frameworks"` | ||
| Frameworks []KueueIntegration `json:"frameworks,omitempty"` | ||
| // externalFrameworks are a list of GroupVersionResources | ||
| // that are managed for Kueue by external controllers. | ||
| // externalFrameworks are optional and should only be used if you have an external controller | ||
|
|
@@ -181,7 +181,7 @@ type Integrations struct { | |
| // +listMapKey=group | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| // +optional | ||
| ExternalFrameworks []ExternalFramework `json:"externalFrameworks"` | ||
| ExternalFrameworks []ExternalFramework `json:"externalFrameworks,omitempty"` | ||
| // labelKeysToCopy are a list of label keys that are copied once a workload is created. | ||
| // These keys are persisted to the internal Kueue workload object. | ||
| // If not specified, only the Kueue labels will be copied. | ||
|
|
@@ -190,7 +190,7 @@ type Integrations struct { | |
| // +listType=map | ||
| // +listMapKey=key | ||
| // +optional | ||
| LabelKeysToCopy []LabelKeys `json:"labelKeysToCopy"` | ||
| LabelKeysToCopy []LabelKeys `json:"labelKeysToCopy,omitempty"` | ||
| } | ||
|
|
||
| type LabelKeys struct { | ||
|
|
@@ -208,7 +208,7 @@ type LabelKeys struct { | |
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(r'^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?([a-z0-9]([-a-z0-9]*[a-z0-9])?)$')" | ||
| // +required | ||
| Key string `json:"key"` | ||
| Key string `json:"key,omitempty"` | ||
| } | ||
|
|
||
| // +kubebuilder:validation:Enum=ByWorkload;None;"" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a couple of differences from upstream, is this expected?
https://github.com/kubernetes-sigs/kueue/blob/main/.golangci-kal.yml#L50-L61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed up a change for conditions so it matches.