🌱 Add enum validation annotation to CollisionProtection #2334
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes three small issues discovered during an automatic review of the ClusterExtensionRevision API: corrects a typo in the kubebuilder validation annotation for the Revision field's minimum value, adds missing enum validation to the CollisionProtection field, and changes the Revision field type from int64 to int32.
- Fixed typo in kubebuilder validation annotation (
Minimum:=1→Minimum=1) - Added enum validation to CollisionProtection field
- Changed Revision field type from int64 to int32
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| api/v1/clusterextensionrevision_types.go | Fixed validation typo, added CollisionProtection enum validation, and changed Revision field from int64 to int32 |
| manifests/experimental.yaml | Updated generated CRD manifest with CollisionProtection enum values and int32 format for Revision |
| manifests/experimental-e2e.yaml | Updated generated e2e CRD manifest with CollisionProtection enum values and int32 format for Revision |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml | Updated generated Helm CRD manifest with CollisionProtection enum values and int32 format for Revision |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
It looks that this PR could be reduced to the validation of CollisionProtection field only.
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="revision is immutable" | ||
| Revision int64 `json:"revision"` | ||
| Revision int32 `json:"revision"` |
There was a problem hiding this comment.
what we are improving/fixing with changing the type?
There was a problem hiding this comment.
It also looks like this change is causing type mismatch errors as boxcutter still expects int64 in some places.
There was a problem hiding this comment.
In that case we should revert it back to int64.
There was a problem hiding this comment.
Isn't this technically an API change (due to the size change)?
There was a problem hiding this comment.
And possibly a breaking change too from an API perspective.
There was a problem hiding this comment.
It seems to be best practice to use int32 if the extra precision isn't needed as it avoid issues with JSON serialization on some clients.
I don't have a big dog in this hunt since I don't think we'll experience the json serialization issues (since we'll never reach a number large enough for it to show up). I guess we could also cap the value at the JSON max int to avoid this? I'd also like to understand whether we think we need the extra precision and what do we lose by not having it...
It would be a breaking change. But, is my understanding correct that we are allowed to have breaking API changes while in experimental mode? Or is the expectation that we change the version? e.g. v1alpha2?
| // | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Minimum:=1 | ||
| // +kubebuilder:validation:Minimum=1 |
There was a problem hiding this comment.
The version with := is documented: https://book.kubebuilder.io/reference/markers/crd-validation
You can also see that this is a no-op change - check the generated CRD.
There was a problem hiding this comment.
Ah, I didn't know that. Is there value in using a single format in the CRDs we produce?
perdasilva
changed the title
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
revision-api-fixes
branch
from
49e1da6 to
325050f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2334 +/- ##
=======================================
Coverage 74.30% 74.30%
=======================================
Files 91 91
Lines 7083 7083
=======================================
Hits 5263 5263
Misses 1405 1405
Partials 415 415
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:
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pedjak, rashmigottipati, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit bf7e39f
into
operator-framework:main
4 participants
Description
Adds missing enum validation on CollisionProtection
+kubebuilder:validation:Enum=Prevent;IfNoController;NoneReviewer Checklist