WIP: Fix API k8s API conventions and Kube API linter check #2394
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| [APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-merge-robot added the needs-rebase 
camilamacedo86 changed the title 
openshift-ci bot added the do-not-merge/work-in-progress 
…ncorrect kubebuilder marker usage for ClusterCatalog Spec Priority.
…arker is being used
camilamacedo86 force-pushed the linter-check branch from 2a22350 to c565f79 Compare openshift-merge-robot removed the needs-rebase There was a problem hiding this comment.
Pull request overview
This PR introduces a new kube-api-linter plugin to the golangci-lint toolchain to enforce Kubernetes API conventions in the codebase. The changes standardize API marker annotations across Go type definitions and update generated CRD manifests and documentation accordingly.
Key changes:
- Adds kube-api-linter plugin configuration and custom golangci-lint build setup
- Replaces
+kubebuilder:validation:Requiredmarkers with the shorter+requiredmarker - Adds
+optionalmarkers for optional fields to improve API clarity - Fixes validation marker syntax (e.g.,
minimum→Minimum,maximum→Maximum) - Removes
metadatafrom CRD required field lists (metadata is implicitly required) - Fixes a typo:
+kubebuilder:validation.Required→+required
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.golangci.yaml | Adds kubeapilinter to enabled linters with optionalfields configuration preferring pointers only when required |
.custom-gcl.yml | New configuration file for building custom golangci-lint with kube-api-linter plugin |
Makefile | Adds build target for custom golangci-lint binary with kubeapilinter plugin and updates lint target to use it |
api/v1/clustercatalog_types.go | Replaces +kubebuilder:validation:Required with +required, fixes validation marker casing, and adds +optional markers; also makes metadata field optional |
api/v1/clusterextension_types.go | Replaces +kubebuilder:validation:Required with +required, fixes typo (validation.Required), and adds +optional and +required markers |
api/v1/clusterextensionrevision_types.go | Adds +optional and +required markers for clarity |
hack/tools/crd-generator/testdata/api/v1/clusterextension_types.go | Same marker updates as the main clusterextension_types.go file |
manifests/*.yaml | Updates generated CRD manifests with minimum/maximum validation constraints and removes metadata from required fields |
helm/olmv1/base/catalogd/crd/*/*.yaml | Updates generated Helm CRD files with minimum/maximum validation constraints and removes metadata from required fields |
docs/api-reference/olmv1-api-reference.md | Updates generated API documentation to reflect removal of Required validation markers in the validation column |
Comments suppressed due to low confidence (1)
api/v1/clustercatalog_types.go:267
- The field Image is a pointer type but marked with +required. According to Kubernetes API conventions and the configured linter preference (WhenRequired), required fields should not be pointers. Consider either removing the pointer type if the field must always be present when Type is "Image", or removing the +required marker if the field should remain optional. The XValidation rule on the parent ResolvedCatalogSource struct already validates presence conditionally.
} 💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When set to "Strict", the CRD Upgrade Safety pre-flight check runs during an upgrade operation. | ||
| // |
There was a problem hiding this comment.
The field CRDUpgradeSafety is a pointer type but marked with +required. According to Kubernetes API conventions and the configured linter preference (WhenRequired), required fields should not be pointers. Consider either removing the pointer type if the field must always be present, or removing the +required marker if the field should remain optional. The XValidation rule on the parent PreflightConfig struct already validates presence with has(self.crdUpgradeSafety), which suggests the field should be treated as required when PreflightConfig is specified.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #2394 +/- ## ========================================== + Coverage 68.79% 68.82% +0.02% ========================================== Files 100 100 Lines 7641 7641 ========================================== + Hits 5257 5259 +2 + Misses 1948 1947 -1 + Partials 436 435 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
camilamacedo86 force-pushed the linter-check branch from e3fd8a4 to 5feea3e Compare There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| conditions: | ||
| description: |- | ||
| The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | |
| conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| conditions: | ||
| description: |- | ||
| The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | |
| conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| conditions: | ||
| description: |- | ||
| The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | |
| conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| // metadata is the standard object's metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| metav1.ListMeta `json:"metadata"` | ||
| metav1.ListMeta `json:"metadata,omitempty"` |
There was a problem hiding this comment.
According to Kubernetes API conventions, the metadata field in List types should not have the omitempty tag. The ListMeta contains important pagination and versioning information (like resourceVersion and continue tokens) that should always be present in list responses, even when empty. The metadata field should be json:"metadata" without omitempty.
| metav1.ListMeta `json:"metadata,omitempty"` | |
| metav1.ListMeta `json:"metadata"` |
| type ClusterExtensionStatus struct { | ||
| // The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| // conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| // |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| // | |
| // conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| conditions: | ||
| description: |- | ||
| The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | |
| conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| conditions: | ||
| description: |- | ||
| The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | |
| conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| conditions: | ||
| description: |- | ||
| The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing. | |
| conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing. |
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | The set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether the bundle has been installed for this ClusterExtension:<br /> - When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br /> - When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br /><opcon:experimental:description><br />When Progressing is True and Reason is RollingOut, the ClusterExtension has one or more ClusterExtensionRevisions in active roll out.<br /></opcon:experimental:description><br />When the ClusterExtension is sourced from a catalog, it may also communicate a deprecation condition.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle:<br /> - BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.<br /> - ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.<br /> - PackageDeprecated is set if the requested package is marked deprecated in the catalog.<br /> - Deprecated is a rollup condition that is present when any of the deprecated conditions are present. | | | | ||
| | `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether the bundle has been installed for this ClusterExtension:<br /> - When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br /> - When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br /><opcon:experimental:description><br />When Progressing is True and Reason is RollingOut, the ClusterExtension has one or more ClusterExtensionRevisions in active roll out.<br /></opcon:experimental:description><br />When the ClusterExtension is sourced from a catalog, it may also communicate a deprecation condition.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle:<br /> - BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.<br /> - ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.<br /> - PackageDeprecated is set if the requested package is marked deprecated in the catalog.<br /> - Deprecated is a rollup condition that is present when any of the deprecated conditions are present. | | | |
There was a problem hiding this comment.
The updated description has a subject-verb agreement error. "conditions represents" should be "conditions represent" since "conditions" is plural. Additionally, the sentence structure is awkward with "which apply...are". Consider rephrasing to: "conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing."
| | `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | conditions represents the set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether the bundle has been installed for this ClusterExtension:<br /> - When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br /> - When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br /><opcon:experimental:description><br />When Progressing is True and Reason is RollingOut, the ClusterExtension has one or more ClusterExtensionRevisions in active roll out.<br /></opcon:experimental:description><br />When the ClusterExtension is sourced from a catalog, it may also communicate a deprecation condition.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle:<br /> - BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.<br /> - ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.<br /> - PackageDeprecated is set if the requested package is marked deprecated in the catalog.<br /> - Deprecated is a rollup condition that is present when any of the deprecated conditions are present. | | | | |
| | `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | conditions represent the set of condition types that apply to all spec.source variations: Installed and Progressing.<br />The Installed condition represents whether the bundle has been installed for this ClusterExtension:<br /> - When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br /> - When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br /><opcon:experimental:description><br />When Progressing is True and Reason is RollingOut, the ClusterExtension has one or more ClusterExtensionRevisions in active roll out.<br /></opcon:experimental:description><br />When the ClusterExtension is sourced from a catalog, it may also communicate a deprecation condition.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle:<br /> - BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.<br /> - ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.<br /> - PackageDeprecated is set if the requested package is marked deprecated in the catalog.<br /> - Deprecated is a rollup condition that is present when any of the deprecated conditions are present. | | | |
| rules: | ||
| - path-except: "^api/" | ||
| linters: | ||
| - kubeapilinter |
There was a problem hiding this comment.
The exclusion rule appears to be inverted. The configuration "path-except: ^api/" with "linters: - kubeapilinter" means the kubeapilinter will be excluded from running on paths matching ^api/, which is the opposite of what's intended. The kubeapilinter should specifically check the api/ directory for Kubernetes API conventions. Consider using a positive path filter instead, or use the issues exclusion syntax to only run kubeapilinter on api/ paths.
2 participants
Continuation of : #2058