Skip to content

🌱 Enable optionalorrequired linter #11909

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion .golangci-kal.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ linters-settings:
- "maxlength" # Ensure all strings and arrays have maximum lengths/maximum items.
- "nobools" # Bools do not evolve over time, should use enums instead.
- "nofloats" # Ensure floats are not used.
- "optionalorrequired" # Every field should be marked as `+optional` or `+required`.
- "requiredfields" # Required fields should not be pointers, and should not have `omitempty`.
- "statussubresource" # All root objects that have a `status` field should have a status subresource.

Expand All @@ -32,7 +33,6 @@ linters-settings:
# - "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.
# - "optionalorrequired" # Every field should be marked as `+optional` or `+required`.
disable:
- "*" # We will manually enable new linters after understanding the impact. Disable all by default.
lintersConfig:
Expand Down Expand Up @@ -102,3 +102,16 @@ issues:
text: "field (XPreserveUnknownFields|XPreserveUnknownFields|XValidations|XMetadata|XIntOrString) json tag does not match pattern"
linters:
- kal
# The following rules are disabled until we migrate to the new API.
- path: "bootstrap/kubeadm/api/v1beta1/kubeadm_types.go"
text: "field Token is marked as required, should not be a pointer"
linters:
- kal
- path: "api/v1beta1/clusterclass_types.go"
text: "field Ref is marked as required, should not be a pointer"
linters:
- kal
- path: "api/v1alpha1/*|api/v1beta1/*|api/v1alpha3/*"
text: "field Items must be marked as optional or required"
linters:
- kal
15 changes: 15 additions & 0 deletions api/v1beta1/cluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ type ClusterAvailabilityGate struct {
// Topology encapsulates the information of the managed resources.
type Topology struct {
// class is the name of the ClusterClass object to create the topology.
// +required
Class string `json:"class"`

// classNamespace is the namespace of the ClusterClass object to create the topology.
Expand All @@ -553,6 +554,7 @@ type Topology struct {
ClassNamespace string `json:"classNamespace,omitempty"`

// version is the Kubernetes version of the cluster.
// +required
Version string `json:"version"`

// rolloutAfter performs a rollout of the entire cluster one component at a time,
Expand Down Expand Up @@ -669,12 +671,14 @@ type MachineDeploymentTopology struct {
// class is the name of the MachineDeploymentClass used to create the set of worker nodes.
// This should match one of the deployment classes defined in the ClusterClass object
// mentioned in the `Cluster.Spec.Class` field.
// +required
Class string `json:"class"`

// name is the unique identifier for this MachineDeploymentTopology.
// The value is used with other unique identifiers to create a MachineDeployment's Name
// (e.g. cluster's name, etc). In case the name is greater than the allowed maximum length,
// the values are hashed together.
// +required
Name string `json:"name"`

// failureDomain is the failure domain the machines will be created in.
Expand Down Expand Up @@ -772,12 +776,14 @@ type MachinePoolTopology struct {
// class is the name of the MachinePoolClass used to create the pool of worker nodes.
// This should match one of the deployment classes defined in the ClusterClass object
// mentioned in the `Cluster.Spec.Class` field.
// +required
Class string `json:"class"`

// name is the unique identifier for this MachinePoolTopology.
// The value is used with other unique identifiers to create a MachinePool's Name
// (e.g. cluster's name, etc). In case the name is greater than the allowed maximum length,
// the values are hashed together.
// +required
Name string `json:"name"`

// failureDomains is the list of failure domains the machine pool will be created in.
Expand Down Expand Up @@ -826,6 +832,7 @@ type MachinePoolTopology struct {
// Variable definition in the ClusterClass `status` variables.
type ClusterVariable struct {
// name of the variable.
// +required
Name string `json:"name"`

// definitionFrom specifies where the definition of this Variable is from.
Expand All @@ -842,6 +849,7 @@ type ClusterVariable struct {
// hard-coded schema for apiextensionsv1.JSON which cannot be produced by another type via controller-tools,
// i.e. it is not possible to have no type field.
// Ref: https://github.com/kubernetes-sigs/controller-tools/blob/d0e03a142d0ecdd5491593e941ee1d6b5d91dba6/pkg/crd/known_types.go#L106-L111
// +required
Value apiextensionsv1.JSON `json:"value"`
}

Expand Down Expand Up @@ -908,6 +916,7 @@ type ClusterNetwork struct {
type NetworkRanges struct {
// cidrBlocks is a list of CIDR blocks.
// +kubebuilder:validation:MaxItems=100
// +required
CIDRBlocks []string `json:"cidrBlocks"`
}

Expand Down Expand Up @@ -1073,9 +1082,11 @@ func (c *ClusterStatus) GetTypedPhase() ClusterPhase {
// APIEndpoint represents a reachable Kubernetes API endpoint.
type APIEndpoint struct {
// host is the hostname on which the API server is serving.
// +required
Host string `json:"host"`

// port is the port on which the API server is serving.
// +required
Port int32 `json:"port"`
}

Expand Down Expand Up @@ -1110,11 +1121,14 @@ type Cluster struct {
metav1.TypeMeta `json:",inline"`
// metadata is the standard object's metadata.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`

// spec is the desired state of Cluster.
// +optional
Spec ClusterSpec `json:"spec,omitempty"`
// status is the observed state of Cluster.
// +optional
Status ClusterStatus `json:"status,omitempty"`
}

Expand Down Expand Up @@ -1248,6 +1262,7 @@ type ClusterList struct {
metav1.TypeMeta `json:",inline"`
// metadata is the standard list's metadata.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#lists-and-simple-kinds
// +optional
metav1.ListMeta `json:"metadata,omitempty"`
// items is the list of Clusters.
Items []Cluster `json:"items"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we making this optional? Shouldn't this be required? (see also the delta in zz_generated.openapi.go)

(cc @JoelSpeed)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean with exactly? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edited comments were not reflected.
required is correct here, ideally zz_generated_api.go should not be changed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the List version of a resource doesn't matter too much, it doesn't effect the generated CRD or behaviour of the cluster, but does affect the openapi as you've seen.

I'm intending to check with API machinery folks about their expectations here, whether this should be optional or required and from there will set up the linters to ensure consistency.

Copy link
Member

@sbueringer sbueringer Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If getting that feedback from apimachinery takes too long I would suggest to drop all the markers for the Items field for now. Then we can get at least everything else done for the code freeze

(I don't want to block everything else on this PR just for a marker that don't have any impact anywhere (not in CRDs and the generated OpenAPI code is as far as I know only used to generate some specs for Runtime Extensions that don't even use lists)

Copy link
Member

@sbueringer sbueringer Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a bit about it. @sivchari Can we directly remove the markers from the Items fields on this PR, add an exclude and follow-up? I have multiple other PRs with potential merge conflicts that I have to coordinate with this PR

(adding the Items markers later is trivial)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbueringer
Make sense to me, thanks.
I fixed them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

Expand Down
36 changes: 36 additions & 0 deletions api/v1beta1/clusterclass_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,14 @@ type ClusterClass struct {
metav1.TypeMeta `json:",inline"`
// metadata is the standard object's metadata.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`

// spec is the desired state of ClusterClass.
// +optional
Spec ClusterClassSpec `json:"spec,omitempty"`
// status is the observed state of ClusterClass.
// +optional
Status ClusterClassStatus `json:"status,omitempty"`
}

Expand Down Expand Up @@ -255,10 +258,12 @@ type MachineDeploymentClass struct {
// class denotes a type of worker node present in the cluster,
// this name MUST be unique within a ClusterClass and can be referenced
// in the Cluster to create a managed MachineDeployment.
// +required
Class string `json:"class"`

// template is a local struct containing a collection of templates for creation of
// MachineDeployment objects representing a set of worker nodes.
// +required
Template MachineDeploymentClassTemplate `json:"template"`

// machineHealthCheck defines a MachineHealthCheck for this MachineDeploymentClass.
Expand Down Expand Up @@ -300,6 +305,7 @@ type MachineDeploymentClass struct {
// Defaults to 0 (machine will be considered available as soon as it
// is ready)
// NOTE: This value can be overridden while defining a Cluster.Topology using this MachineDeploymentClass.
// +optional
MinReadySeconds *int32 `json:"minReadySeconds,omitempty"`

// readinessGates specifies additional conditions to include when evaluating Machine Ready condition.
Expand All @@ -319,6 +325,7 @@ type MachineDeploymentClass struct {
// strategy is the deployment strategy to use to replace existing machines with
// new ones.
// NOTE: This value can be overridden while defining a Cluster.Topology using this MachineDeploymentClass.
// +optional
Strategy *MachineDeploymentStrategy `json:"strategy,omitempty"`
}

Expand All @@ -332,10 +339,12 @@ type MachineDeploymentClassTemplate struct {

// bootstrap contains the bootstrap template reference to be used
// for the creation of worker Machines.
// +required
Bootstrap LocalObjectTemplate `json:"bootstrap"`

// infrastructure contains the infrastructure template reference to be used
// for the creation of worker Machines.
// +required
Infrastructure LocalObjectTemplate `json:"infrastructure"`
}

Expand Down Expand Up @@ -410,10 +419,12 @@ type MachinePoolClass struct {
// class denotes a type of machine pool present in the cluster,
// this name MUST be unique within a ClusterClass and can be referenced
// in the Cluster to create a managed MachinePool.
// +required
Class string `json:"class"`

// template is a local struct containing a collection of templates for creation of
// MachinePools objects representing a pool of worker nodes.
// +required
Template MachinePoolClassTemplate `json:"template"`

// failureDomains is the list of failure domains the MachinePool should be attached to.
Expand Down Expand Up @@ -452,6 +463,7 @@ type MachinePoolClass struct {
// Defaults to 0 (machine will be considered available as soon as it
// is ready)
// NOTE: This value can be overridden while defining a Cluster.Topology using this MachinePoolClass.
// +optional
MinReadySeconds *int32 `json:"minReadySeconds,omitempty"`
}

Expand All @@ -465,10 +477,12 @@ type MachinePoolClassTemplate struct {

// bootstrap contains the bootstrap template reference to be used
// for the creation of the Machines in the MachinePool.
// +required
Bootstrap LocalObjectTemplate `json:"bootstrap"`

// infrastructure contains the infrastructure template reference to be used
// for the creation of the MachinePool.
// +required
Infrastructure LocalObjectTemplate `json:"infrastructure"`
}

Expand All @@ -495,12 +509,14 @@ func (m MachineHealthCheckClass) IsZero() bool {
// be configured in the Cluster topology and used in patches.
type ClusterClassVariable struct {
// name of the variable.
// +required
Name string `json:"name"`

// required specifies if the variable is required.
// Note: this applies to the variable as a whole and thus the
// top-level object defined in the schema. If nested fields are
// required, this will be specified inside the schema.
// +required
Required bool `json:"required"`

// metadata is the metadata of a variable.
Expand All @@ -513,6 +529,7 @@ type ClusterClassVariable struct {
Metadata ClusterClassVariableMetadata `json:"metadata,omitempty"`

// schema defines the schema of the variable.
// +required
Schema VariableSchema `json:"schema"`
}

Expand All @@ -539,6 +556,7 @@ type VariableSchema struct {
// openAPIV3Schema defines the schema of a variable via OpenAPI v3
// schema. The schema is a subset of the schema used in
// Kubernetes CRDs.
// +required
OpenAPIV3Schema JSONSchemaProps `json:"openAPIV3Schema"`
}

Expand All @@ -549,9 +567,11 @@ type VariableSchema struct {
// which are not supported in CAPI have been removed.
type JSONSchemaProps struct {
// description is a human-readable description of this variable.
// +optional
Description string `json:"description,omitempty"`

// example is an example for this variable.
// +optional
Example *apiextensionsv1.JSON `json:"example,omitempty"`

// type is the type of the variable.
Expand Down Expand Up @@ -871,9 +891,11 @@ const (
// ClusterClassPatch defines a patch which is applied to customize the referenced templates.
type ClusterClassPatch struct {
// name of the patch.
// +required
Name string `json:"name"`

// description is a human-readable description of this patch.
// +optional
Description string `json:"description,omitempty"`

// enabledIf is a Go template to be used to calculate if a patch should be enabled.
Expand All @@ -900,12 +922,14 @@ type ClusterClassPatch struct {
// PatchDefinition defines a patch which is applied to customize the referenced templates.
type PatchDefinition struct {
// selector defines on which templates the patch should be applied.
// +required
Selector PatchSelector `json:"selector"`

// jsonPatches defines the patches which should be applied on the templates
// matching the selector.
// Note: Patches will be applied in the order of the array.
// +kubebuilder:validation:MaxItems=100
// +required
JSONPatches []JSONPatch `json:"jsonPatches"`
}

Expand All @@ -916,12 +940,15 @@ type PatchDefinition struct {
// Note: The results of selection based on the individual fields are ANDed.
type PatchSelector struct {
// apiVersion filters templates by apiVersion.
// +required
APIVersion string `json:"apiVersion"`

// kind filters templates by kind.
// +required
Kind string `json:"kind"`

// matchResources selects templates based on where they are referenced.
// +required
MatchResources PatchSelectorMatch `json:"matchResources"`
}

Expand Down Expand Up @@ -972,13 +999,15 @@ type PatchSelectorMatchMachinePoolClass struct {
type JSONPatch struct {
// op defines the operation of the patch.
// Note: Only `add`, `replace` and `remove` are supported.
// +required
Op string `json:"op"`

// path defines the path of the patch.
// Note: Only the spec of a template can be patched, thus the path has to start with /spec/.
// Note: For now the only allowed array modifications are `append` and `prepend`, i.e.:
// * for op: `add`: only index 0 (prepend) and - (append) are allowed
// * for op: `replace` or `remove`: no indexes are allowed
// +required
Path string `json:"path"`

// value defines the value of the patch.
Expand Down Expand Up @@ -1039,6 +1068,7 @@ type ExternalPatchDefinition struct {
type LocalObjectTemplate struct {
// ref is a required reference to a custom resource
// offered by a provider.
// +required
Ref *corev1.ObjectReference `json:"ref"`
}

Expand Down Expand Up @@ -1079,6 +1109,7 @@ type ClusterClassV1Beta2Status struct {
// ClusterClassStatusVariable defines a variable which appears in the status of a ClusterClass.
type ClusterClassStatusVariable struct {
// name is the name of the variable.
// +required
Name string `json:"name"`

// definitionsConflict specifies whether or not there are conflicting definitions for a single variable name.
Expand All @@ -1087,6 +1118,7 @@ type ClusterClassStatusVariable struct {

// definitions is a list of definitions for a variable.
// +kubebuilder:validation:MaxItems=100
// +required
Definitions []ClusterClassStatusVariableDefinition `json:"definitions"`
}

Expand All @@ -1095,12 +1127,14 @@ type ClusterClassStatusVariableDefinition struct {
// from specifies the origin of the variable definition.
// This will be `inline` for variables defined in the ClusterClass or the name of a patch defined in the ClusterClass
// for variables discovered from a DiscoverVariables runtime extensions.
// +required
From string `json:"from"`

// required specifies if the variable is required.
// Note: this applies to the variable as a whole and thus the
// top-level object defined in the schema. If nested fields are
// required, this will be specified inside the schema.
// +required
Required bool `json:"required"`

// metadata is the metadata of a variable.
Expand All @@ -1113,6 +1147,7 @@ type ClusterClassStatusVariableDefinition struct {
Metadata ClusterClassVariableMetadata `json:"metadata,omitempty"`

// schema defines the schema of the variable.
// +required
Schema VariableSchema `json:"schema"`
}

Expand Down Expand Up @@ -1151,6 +1186,7 @@ type ClusterClassList struct {
metav1.TypeMeta `json:",inline"`
// metadata is the standard list's metadata.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#lists-and-simple-kinds
// +optional
metav1.ListMeta `json:"metadata,omitempty"`
// items is the list of ClusterClasses.
Items []ClusterClass `json:"items"`
Expand Down
2 changes: 2 additions & 0 deletions api/v1beta1/common_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,11 @@ const (
// MachineAddress contains information for the node's address.
type MachineAddress struct {
// type is the machine address type, one of Hostname, ExternalIP, InternalIP, ExternalDNS or InternalDNS.
// +required
Type MachineAddressType `json:"type"`

// address is the machine address.
// +required
Address string `json:"address"`
}

Expand Down
Loading