🌱 Update boxcutter integration for sibling owners API#2764
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ddb7a76 to
fe0acea
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the operator-controller’s Boxcutter integration to treat all active sibling ClusterObjectSet revisions (both lower and higher revision numbers) as relevant “owners”, aiming to avoid false-positive collision reporting during revision handover. It also adds coverage to validate collision behavior when a conflicting ClusterExtension is upgraded.
Changes:
- Switch Boxcutter ownership inputs from “previous revisions only” to “all active sibling revisions” and add unit tests for the sibling listing logic.
- Add an e2e scenario asserting collisions persist even when a conflicting
ClusterExtensionupgrades to a different version. - Update Go module dependencies for Boxcutter (and related transitive deps).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
test/e2e/features/update.feature |
Adds an e2e scenario for collision persistence across upgrades (but needs step/param fixes). |
internal/operator-controller/controllers/revision_engine_factory.go |
Adjusts Boxcutter object engine factory invocation to match updated dependency API. |
internal/operator-controller/controllers/clusterobjectset_controller.go |
Introduces sibling revision listing and feeds sibling owners into Boxcutter. |
internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go |
Adds unit tests for listSiblingRevisions. |
go.mod |
Bumps Boxcutter + deps, but currently includes merge-conflict markers and an invalid local replace. |
go.sum |
Updates dependency sums, but currently includes multiple unresolved merge-conflict blocks. |
Comments suppressed due to low confidence (2)
go.mod:7
go.modstill contains unresolved merge-conflict markers (<<<<<<< / ======= / >>>>>>>). This makes the module file invalid and will breakgotooling in CI. Resolve the conflict by choosing a singlegodirective line and removing all conflict markers, then re-rungo mod tidyto ensurego.sumis consistent.
go 1.26.3
require (
github.com/BurntSushi/toml v1.6.0
github.com/Masterminds/semver/v3 v3.5.0
go.mod:329
- This
replacepoints to a developer-local filesystem path, which will not exist in CI and will break module resolution for anyone else. Repositorygo.modfiles should not contain machine-specific local replaces; rely on the tagged boxcutter version instead.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe0acea to
ceb6fee
Compare
d81b767 to
507f415
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2764 +/- ##
==========================================
- Coverage 66.78% 66.77% -0.02%
==========================================
Files 149 149
Lines 11386 11394 +8
==========================================
+ Hits 7604 7608 +4
- Misses 3222 3224 +2
- Partials 560 562 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pedjak
left a comment
There was a problem hiding this comment.
question (non-blocking): The Copilot review on an earlier push mentioned an e2e .feature file change for collision persistence across upgrades, but it's absent from the current diff. Is that planned as a follow-up PR? The unit tests cover listSiblingRevisions well, but an e2e test exercising the higher-sibling handover path would add confidence that the behavioral change works end-to-end.
507f415 to
1dea3d1
Compare
pedjak
left a comment
There was a problem hiding this comment.
Review focused on boxcutter library update and coding style.
1c88b5a to
c490544
Compare
c490544 to
c3a323b
Compare
| machinery.NewComparator(f.DiscoveryClient, f.Scheme, f.FieldOwnerPrefix), | ||
| f.FieldOwnerPrefix, f.FieldOwnerPrefix, scopedClient, | ||
| f.FieldOwnerPrefix, f.FieldOwnerPrefix, | ||
| "", // managedBy: empty string defaults to "boxcutter" |
There was a problem hiding this comment.
why not setting to something like "olm" or "operator-controller"? What if boxcutter changes the treatment of empty string in the future and sets to something else than boxcutter?
There was a problem hiding this comment.
I went for using the field owner prefix olm.operatorframework.io
c3a323b to
e82a53d
Compare
Adapt the ClusterObjectSet controller to boxcutter v0.14.0's replacement
of `WithPreviousOwners` with `WithSiblingOwners`. The previous API only
passed lower-numbered revisions, so boxcutter could not distinguish
between an unknown controller (a true collision) and a known
higher-numbered sibling revision during handover. The new API expects all
active sibling revision owners — both lower and higher — so that
boxcutter can properly classify them and avoid false collision reports.
Changes:
- Extract shared `listOtherActiveRevisions` helper that encapsulates the
common logic (label lookup, cache list, skip-self, skip-archived,
skip-deleting) with a predicate parameter for caller-specific filtering.
- Add `listSiblingRevisions` (passes all non-self revisions) and refactor
`listPreviousRevisions` (passes only lower revision numbers) to both
delegate to the shared helper.
- Replace `WithPreviousOwners` with `WithSiblingOwners` in
`buildBoxcutterPhases`
- Pass the new required `managedBy` parameter to `NewObjectEngine`,
set to `FieldOwnerPrefix` ("olm.operatorframework.io")
- Bump boxcutter v0.13.1 → v0.14.0 and transitive dependency updates
- Consolidate unit tests into a single parameterized table-driven test
that exercises both methods against shared scenarios
Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e82a53d to
8b958c5
Compare
pedjak
left a comment
There was a problem hiding this comment.
/lgtm
some e2e test are failing though
Description
Adapt the ClusterObjectSet controller to boxcutter v0.14.0's replacement of
WithPreviousOwnerswithWithSiblingOwners.The previous API only passed lower-numbered revisions, meaning boxcutter could not distinguish between an unknown controller (a true collision) and a known higher-numbered sibling revision during handover. The new
WithSiblingOwnersAPI expects all active sibling revision owners — both lower and higher — so that boxcutter can properly classify them and avoid false collision reports.Changes
listOtherActiveRevisions— extracted shared helper that encapsulates the common logic (label lookup, cache list, skip-self, skip-archived, skip-deleting) with anincludepredicate for caller-specific filtering.listSiblingRevisions— delegates tolistOtherActiveRevisionspassing all non-self revisions. Used bybuildBoxcutterPhaseswithWithSiblingOwners.listPreviousRevisions— refactored to delegate tolistOtherActiveRevisions, passing only lower revision numbers. Retained for the archiving use case.buildBoxcutterPhases— callslistSiblingRevisions+WithSiblingOwnersinstead oflistPreviousRevisions+WithPreviousOwners.NewObjectEngine— passes the new requiredmanagedByparameter, set toFieldOwnerPrefix("olm.operatorframework.io").prometheus/common,kube-openapi,cel.dev/expr).Test_ClusterObjectSetReconciler_listOtherActiveRevisions) that exercises bothlistSiblingRevisionsandlistPreviousRevisionsagainst shared scenarios, eliminating ~120 lines of duplicate setup.Reviewer Checklist
🤖 Generated with Claude Code