🌱 OPRUN-4138 ClusterExtensionRevision .spec.revision must be positive
#2231
Conversation
Added the validation rules and the unit tests.
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
pedjak
changed the title
ClusterExtensionRevision .spec.revision must be positiveClusterExtensionRevision .spec.revision must be positive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2231 +/- ##
=======================================
Coverage 72.16% 72.16%
=======================================
Files 85 85
Lines 8440 8440
=======================================
Hits 6091 6091
Misses 1948 1948
Partials 401 401
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:
|
| // Must be positive. Each ClusterExtensionRevision of the same parent ClusterExtension needs to have | ||
| // a unique value assigned. It is immutable after creation. The new revision number must always be previous revision +1. |
There was a problem hiding this comment.
We can validate uniqueness if we require the metadata.name to have a pattern that matches <ceName>-<revNumber>, right?
But I assume there's no way to guarantee "new rev must be previous rev +1"?
There was a problem hiding this comment.
In theory you could add a webhook, but I'd like to avoid webhooks as much as possible.
I don't think you can add a validation rule to .metadata.name, but we could add a CEL transition rule where .spec.revision must be equal to -digit suffix of the name. 🤔
But I'd add that in a new PR.
There was a problem hiding this comment.
we could probably do that, but for that we need a validation webhook where we could fetch all existing revisions, sort them by creation date and compare their revisions with the one we got in the payload. Not sure if the effort would be justified.
There was a problem hiding this comment.
It is possible to have a CEL validation rule for metadata.name. You just have to declare the validation against the root struct that embeds ObjectMeta.
We could use ValidationAdmissionPolicy, which is now preferred over webhooks in general, but there's still the problem that when there are multiple apiserver replicas, they are each acting independently when making validation decisions and (I'm pretty sure) without holding a lock or transaction against etcd for all the data used to make the validation decision.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva, thetechnick 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 0faf118
into
operator-framework:main
4 participants
Description
Added the validation rules and the unit tests.
Reviewer Checklist