add kube-api-linter and resolve API issues #201
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for container-object-storage-interface ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
approved
| // status defines the observed state of Bucket | ||
| // +optional | ||
| Status BucketStatus `json:"status,omitempty,omitzero"` | ||
| Status BucketStatus `json:"status,omitzero"` |
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?
apis/objectstorage/v1alpha2/bucket_types.go:163:2: optionalfields: field Bucket.Status has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer. (kubeapilinter)
Status BucketStatus `json:"status,omitzero"`
^
There was a problem hiding this comment.
KAL will only suggest this to be a pointer if your validations allow {} to be persisted. That means your validations imply a semantic difference between omitted and the empty struct. That's not desirable in 99% of cases.
The solution is to make {} invalid. That can be done by adding a required field to status (this is probably the right path, which fields should the status always have?) or to add a +kubebuilder:validation:MinProperties=1 marker to make sure at least one property is set.
My advice would be to add a required field.
optionalfields: field Bucket.Status has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.
I think I'm just repeating the error message here no? 😅
There was a problem hiding this comment.
I think I finally got my head around this. Thanks for your patience. For now, I made ReadyToUse required, and I realized I needed to configure KAL slightly for CRDs.
BlaineEXE
force-pushed
the
kube-api-linter
branch
from
07c86c2 to
694dd8d
Compare
k8s-ci-robot
added
size/XL
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
BlaineEXE
force-pushed
the
kube-api-linter
branch
from
694dd8d to
5aaae2b
Compare
|
/ok-to-test |
BlaineEXE
force-pushed
the
kube-api-linter
branch
from
5aaae2b to
6e12d2b
Compare
Add kube-api-linter to `make lint`. kube-api-linter is based on golangci-lint v2, so it is upgraded to the latest version. kube-api-linter could be integrated with COSI CI in many ways. After experimentation, it is fastest to run it as a standalone install rather than build it each time `make lint` is run. Signed-off-by: Blaine Gardner <[email protected]>
BlaineEXE
force-pushed
the
kube-api-linter
branch
from
6e12d2b to
67f5f31
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| settings: | ||
| linters: | ||
| enable: | ||
| - statussubresource | ||
| - optionalfields # instead of nonpointerstructs | ||
| - requiredfields # instead of nonpointerstructs | ||
| disable: | ||
| - nonpointerstructs # not intended for CRDs | ||
| - statusoptional |
There was a problem hiding this comment.
@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.
I would avoid statusoptional personally. 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
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BlaineEXE, xing-yang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Add kube-api-linter to
make lint.kube-api-linter is based on golangci-lint v2, so it is upgraded to the latest version.
kube-api-linter could be integrated with COSI CI in many ways. After experimentation, it is fastest to run it as a standalone install rather than build it each time
make lintis run.Related to API review #199
Resolves #195