add kube-api-linter and resolve API issues #201
+403
−267
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| # This is a golangci-lint configuration file specifically for kube-api-linter. | ||
|
|
||
| version: "2" | ||
|
|
||
| linters: | ||
| default: none | ||
| enable: | ||
| - kubeapilinter | ||
|
|
||
| settings: | ||
| custom: | ||
| kubeapilinter: | ||
| type: module | ||
| description: Kube API Linter lints Kube like APIs based on API conventions and best practices. | ||
| settings: | ||
| linters: | ||
| enable: | ||
| - statussubresource | ||
| - optionalfields # instead of nonpointerstructs | ||
| - requiredfields # instead of nonpointerstructs | ||
| disable: | ||
| - nonpointerstructs # not intended for CRDs | ||
| - statusoptional | ||
|
|
||
| lintersConfig: | ||
| optionalfields: | ||
| pointers: | ||
| preference: WhenRequired | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,23 +37,23 @@ const ( | |
| ) | ||
|
|
||
| // BucketSpec defines the desired state of Bucket | ||
| // +kubebuilder:validation:XValidation:message="parameters map cannot be added or removed after creation",rule="has(oldSelf.parameters) == has(self.parameters)" | ||
| // +kubebuilder:validation:XValidation:message="protocols list cannot be added or removed after creation",rule="has(oldSelf.protocols) == has(self.protocols)" | ||
| // +kubebuilder:validation:XValidation:message="existingBucketID cannot be added or removed after creation",rule="has(oldSelf.existingBucketID) == has(self.existingBucketID)" | ||
| type BucketSpec struct { | ||
| // driverName is the name of the driver that fulfills requests for this Bucket. | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:message="driverName is immutable",rule="self == oldSelf" | ||
| DriverName string `json:"driverName,omitempty"` | ||
|
|
||
| // deletionPolicy determines whether a Bucket should be deleted when its bound BucketClaim is | ||
| // deleted. This is mutable to allow Admins to change the policy after creation. | ||
| // Possible values: | ||
| // - Retain: keep both the Bucket object and the backend bucket | ||
| // - Delete: delete both the Bucket object and the backend bucket | ||
| // +required | ||
| DeletionPolicy BucketDeletionPolicy `json:"deletionPolicy,omitempty"` | ||
|
|
||
| // parameters is an opaque map of driver-specific configuration items passed to the driver that | ||
| // fulfills requests for this Bucket. | ||
|
|
@@ -72,68 +72,70 @@ type BucketSpec struct { | |
| // For statically-provisioned buckets, set the namespace and name of the BucketClaim that is | ||
| // allowed to bind to this Bucket. | ||
| // +required | ||
| BucketClaimRef BucketClaimReference `json:"bucketClaim,omitzero"` | ||
|
|
||
| // existingBucketID is the unique identifier for an existing backend bucket known to the driver. | ||
| // Use driver documentation to determine how to set this value. | ||
| // This field is used only for Bucket static provisioning. | ||
| // This field will be empty when the Bucket is dynamically provisioned from a BucketClaim. | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:message="existingBucketID is immutable",rule="self == oldSelf" | ||
| ExistingBucketID string `json:"existingBucketID,omitempty"` | ||
| } | ||
|
|
||
| // BucketClaimReference is a reference to a BucketClaim object. | ||
| // +kubebuilder:validation:XValidation:message="namespace cannot be removed once set",rule="!has(oldSelf.namespace) || has(self.namespace)" | ||
| // +kubebuilder:validation:XValidation:message="uid cannot be removed once set",rule="!has(oldSelf.uid) || has(self.uid)" | ||
| type BucketClaimReference struct { | ||
| // name is the name of the BucketClaim being referenced. | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:XValidation:message="name is immutable",rule="self == oldSelf" | ||
| Name string `json:"name,omitempty"` | ||
|
|
||
| // namespace is the namespace of the BucketClaim being referenced. | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:XValidation:message="namespace is immutable",rule="self == oldSelf" | ||
| Namespace string `json:"namespace,omitempty"` | ||
|
|
||
| // uid is the UID of the BucketClaim being referenced. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:message="uid is immutable once set",rule="oldSelf == '' || self == oldSelf" | ||
| UID types.UID `json:"uid,omitempty"` | ||
| } | ||
|
|
||
| // BucketStatus defines the observed state of Bucket. | ||
| // +kubebuilder:validation:XValidation:message="bucketID cannot be removed once set",rule="!has(oldSelf.bucketID) || has(self.bucketID)" | ||
| // +kubebuilder:validation:XValidation:message="protocols cannot be removed once set",rule="!has(oldSelf.protocols) || has(self.protocols)" | ||
| type BucketStatus struct { | ||
| // readyToUse indicates that the bucket is ready for consumption by workloads. | ||
| // +required | ||
| ReadyToUse *bool `json:"readyToUse,omitempty"` | ||
|
|
||
| // bucketID is the unique identifier for the backend bucket known to the driver. | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:message="boundBucketName is immutable once set",rule="self == oldSelf" | ||
| BucketID string `json:"bucketID,omitempty"` | ||
|
|
||
| // protocols is the set of protocols the Bucket reports to support. BucketAccesses can request | ||
| // access to this BucketClaim using any of the protocols reported here. | ||
| // +optional | ||
| // +listType=set | ||
| Protocols []ObjectProtocol `json:"protocols,omitempty"` | ||
|
|
||
| // bucketInfo contains info about the bucket reported by the driver, rendered in the same | ||
| // COSI_<PROTOCOL>_<KEY> format used for the BucketAccess Secret. | ||
| // e.g., COSI_S3_ENDPOINT, COSI_AZURE_STORAGE_ACCOUNT. | ||
| // This should not contain any sensitive information. | ||
| // +optional | ||
| BucketInfo map[string]string `json:"bucketInfo,omitempty"` | ||
|
|
||
| // error holds the most recent error message, with a timestamp. | ||
| // This is cleared when provisioning is successful. | ||
| // +optional | ||
| Error *TimestampedError `json:"error,omitempty"` | ||
|
|
@@ -154,11 +156,11 @@ type Bucket struct { | |
|
|
||
| // spec defines the desired state of Bucket | ||
| // +required | ||
| Spec BucketSpec `json:"spec,omitzero"` | ||
|
|
||
| // status defines the observed state of Bucket | ||
| // +optional | ||
| Status BucketStatus `json:"status,omitzero"` | ||
|
Comment on lines
159
to
+163
Contributor
Author
There was a problem hiding this comment. @JoelSpeed I'm having some challenges coming up with the best action to take based KAL's response on the status here. Do you have a recommended pattern for validations to apply here to avoid a pointer status and allow KAL to pass? There was a problem hiding this comment. KAL will only suggest this to be a pointer if your validations allow The solution is to make My advice would be to add a required field.
I think I'm just repeating the error message here no? 😅
Contributor
Author
There was a problem hiding this comment. I think I finally got my head around this. Thanks for your patience. For now, I made |
||
| } | ||
|
|
||
| // +kubebuilder:object:root=true | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@JoelSpeed do these linters make sense for our CRD-based project? I am not 100 certain about
statusoptional, but I presume this to be reasonable based on your suggestion to make a status field required.The others are documented well regarding CRD usage.
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 would avoid
statusoptionalpersonally. It's a preference for some (historically all status fields were optional) but I've discussed this with other API reviewers and I think that's a legacy pattern that we don't need to repeat