🌱 Add collisionProtection inheritance hierarchy to ClusterExtensionRevision#2513
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 updates the ClusterExtensionRevision API to introduce a phases wrapper (.spec.phases) that supports collision protection inheritance (phases-level default → per-phase → per-object), and wires the new shape through controller logic, tests, and published CRD manifests.
Changes:
- Restructure
.spec.phasesfrom[]Phaseto*ClusterExtensionRevisionPhases{ collisionProtection, items[] }, and make per-objectcollisionProtectionoptional. - Implement collision protection resolution (object > phase > phases-level > default Prevent) in the controller.
- Update CRD manifests, deepcopy generation, and unit/e2e tests for the new schema and behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Update phase iteration to handle .spec.phases.items and nil .spec.phases. |
| manifests/experimental.yaml | CRD schema update for phases wrapper + collisionProtection inheritance fields. |
| manifests/experimental-e2e.yaml | Same CRD schema update for the e2e manifest variant. |
| internal/operator-controller/controllers/clusterextensionrevision_controller_test.go | Update test fixtures to construct Phases: &...{Items: ...}. |
| internal/operator-controller/controllers/clusterextensionrevision_controller_internal_test.go | Add unit test for collision protection resolution; update fixtures to new phases shape. |
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Add resolveCollisionProtection and apply effective CP when configuring boxcutter options. |
| internal/operator-controller/applier/boxcutter_test.go | Update tests to assert against .spec.phases.items and new phases struct construction. |
| internal/operator-controller/applier/boxcutter.go | Generate revisions with Phases wrapper; update object collection to handle nil .spec.phases. |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml | Helm-packaged CRD schema update matching the new phases wrapper. |
| api/v1/zz_generated.deepcopy.go | Regenerated deepcopy to include ClusterExtensionRevisionPhases and pointer field copying. |
| api/v1/clusterextensionrevision_types_test.go | Update immutability/validity tests for new schema and collisionProtection validation. |
| api/v1/clusterextensionrevision_types.go | Define ClusterExtensionRevisionPhases, add collisionProtection fields, and make object CP optional. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2513 +/- ##
==========================================
+ Coverage 73.21% 73.27% +0.05%
==========================================
Files 102 103 +1
Lines 8505 8516 +11
==========================================
+ Hits 6227 6240 +13
+ Misses 1802 1801 -1
+ Partials 476 475 -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:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // EffectiveCollisionProtection resolves the collision protection value using | ||
| // the inheritance hierarchy: object > phase > spec > default ("Prevent"). | ||
| func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.CollisionProtection { | ||
| ecp := ocv1.CollisionProtectionPrevent | ||
| for _, c := range cp { | ||
| if c != "" { | ||
| ecp = c | ||
| } | ||
| } | ||
| return ecp |
There was a problem hiding this comment.
EffectiveCollisionProtection is exported and uses a variadic parameter, but its correctness depends on callers supplying values in a specific order (spec, phase, object) even though the doc comment describes an inheritance hierarchy (object > phase > spec). To prevent accidental misuse, consider changing the signature to explicit named parameters (specCP, phaseCP, objectCP) (or otherwise enforcing/documenting the required argument order) so future callers can’t invert precedence silently.
| // | ||
| // The resolution order is: object > phase > spec > default ("Prevent"). | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
api review feedback encouraged us to make this required + no default. Maybe it makes sense?
| }, | ||
| valid: true, | ||
| }, | ||
| "spec collisionProtection rejects invalid values": { |
There was a problem hiding this comment.
If we're going with omitempty and "" for unset, maybe it's also worth checking that "" is rejected?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When set, this value is used as the default for any phase or object that does not | ||
| // explicitly specify its own collisionProtection. | ||
| // | ||
| // The resolution order is: object > phase > spec > default ("Prevent"). |
There was a problem hiding this comment.
should we remove the default call out here since it's required now?
| // The resolution order is: object > phase > spec > default ("Prevent"). | |
| // The resolution order is: object > phase > spec |
| // When set, this value is used as the default for any object in this phase that does not | ||
| // explicitly specify its own collisionProtection. | ||
| // | ||
| // The resolution order is: object > phase > spec > default ("Prevent"). |
There was a problem hiding this comment.
should we carry the same comment here or have a phase specific one that says it inherits from spec by default and the allowable values, etc.?
| // the same resource, resulting in increased load on the API server and etcd. | ||
| // | ||
| // +required | ||
| // When omitted, the value is inherited from the phase, then spec, then defaults to "Prevent". |
There was a problem hiding this comment.
Same with the default here, since it's now required
| // When omitted, the value is inherited from the phase, then spec, then defaults to "Prevent". | |
| // When omitted, the value is inherited from the phase, then spec. |
| // EffectiveCollisionProtection resolves the collision protection value using | ||
| // the inheritance hierarchy: object > phase > spec > default ("Prevent"). | ||
| func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.CollisionProtection { | ||
| ecp := ocv1.CollisionProtectionPrevent |
There was a problem hiding this comment.
should we set any default? maybe we should error if unset (although that should be caught at ingress time anyway)
There was a problem hiding this comment.
well, this is the default: Prevent - if passed collision protection are empty, we return this.
…sion
Add collisionProtection as a required field at the spec level and an
optional field at the phase level, with inheritance resolution:
object > phase > spec > default ("Prevent"). This reduces verbosity by
letting users set a single spec-level default instead of repeating the
value on every object.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/approve |
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva, rashmigottipati 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 |
|
@pedjak: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@pedjak: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override lint-api-diff we have introduced a new field in this PR |
|
@pedjak: Overrode contexts on behalf of pedjak: lint-api-diff DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override upgrade-ex2ex-e2e We have introduced a new required |
|
@pedjak: Overrode contexts on behalf of pedjak: upgrade-ex2ex-e2e DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-bot
bot
merged commit af6733e
into
operator-framework:main
3 participants
Description
Add collisionProtection as a required field at the spec level and an optional field at the phase level, with inheritance resolution: object > phase > spec. This reduces verbosity by letting users set a single spec-level default instead of repeating the value on every object.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Reviewer Checklist