🌱 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. |
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
2 times, most recently
from
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.
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
from
fe0acea to
ceb6fee
Compare
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
3 times, most recently
from
d81b767 to
507f415
Compare
perdasilva
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2764 +/- ##
==========================================
+ Coverage 70.42% 70.56% +0.13%
==========================================
Files 143 143
Lines 10617 10625 +8
==========================================
+ Hits 7477 7497 +20
+ Misses 2579 2571 -8
+ Partials 561 557 -4
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
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.
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
from
507f415 to
1dea3d1
Compare
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Review focused on boxcutter library update and coding style.
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
2 times, most recently
from
1c88b5a to
c490544
Compare
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
from
c490544 to
c3a323b
Compare
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
2 times, most recently
from
e82a53d to
8b958c5
Compare
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
/lgtm
some e2e test are failing though
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>
perdasilva
force-pushed
the
update-boxcutter-sibling-owners
branch
from
8b958c5 to
72f2dd3
Compare
|
[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 |
|
/lgtm |
…bjectSet Verify that a conflicting ClusterExtension with a higher-revision ClusterObjectSet does not take over resources owned by the original ClusterExtension. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
New changes are detected. LGTM label has been removed. |
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Review focused on test structure (unit + e2e) and a minor code comment placement nit.
| Then ClusterExtension is available | ||
| And ClusterExtension reports Progressing as True with Reason Succeeded | ||
| And ClusterExtension reports Installed as True | ||
|
|
There was a problem hiding this comment.
suggestion: This scenario has three When-Then cycles (install first CE → assert available, apply dup CE → assert collision, update first CE → assert still colliding). Multiple When-Then blocks in a single Gherkin scenario is generally considered an anti-pattern — each scenario should specify one behavior, not a multi-step integration script.
Having multiple transitions makes it harder to diagnose which step failed and why, and the scenario title ("does not take over resources") only describes the final assertion.
Consider splitting into two scenarios:
- Conflicting CE gets collision on install
- Conflicting CE with higher revision still gets collision after update
They could share setup via a Background or a reusable Given step.
Also, this scenario involves updating the CE spec (adding DeploymentConfig) and asserting behavior after the update. That makes it a better fit for update.feature than install.feature. The earlier Copilot review also referenced this being in update.feature — was there a reason to move it here?
| require.NoError(t, ocv1.AddToScheme(testScheme)) | ||
|
|
||
| type methodUnderTest struct { | ||
| name string |
There was a problem hiding this comment.
suggestion: The double loop (test cases × methods) with a map[string][]string for expected results works, but it obscures the arrange-act-assert structure. A reader has to mentally cross-reference the map key with the method definition at the top to understand what any single test case asserts.
An alternative structure that would be clearer:
- Test
listOtherActiveRevisionsdirectly with explicit predicates (the real unit under test). This covers the shared filtering logic once. - Add a couple of trivial tests for
listSiblingRevisionsandlistPreviousRevisionsthat just verify they delegate with the correct predicate (e.g., one passes all, the other filters by revision number).
This separates "does the shared logic work" from "do the wrappers delegate correctly" and makes each test read as a clean given-when-then.
|
|
||
| require.ElementsMatch(t, tc.expectedRevs, names) | ||
| }) | ||
| for _, m := range methods { |
There was a problem hiding this comment.
suggestion: The name format fmt.Sprintf("%s/%s", m.name, tc.name) puts the method name first, but the outer loop iterates over test cases. This means Go test output interleaves methods:
listSiblingRevisions/should exclude self...
listPreviousRevisions/should exclude self...
listSiblingRevisions/should exclude archived...
listPreviousRevisions/should exclude archived...
Swapping to tc.name/m.name (or swapping the loop order) would group output by method, making it easier to scan all cases for one method when debugging a failure.
| rev1 := newTestClusterObjectSetInternal(t, "rev-1") | ||
| rev1.Finalizers = []string{"test-finalizer"} | ||
| rev1.DeletionTimestamp = &metav1.Time{Time: time.Now()} | ||
| rev2 := newTestClusterObjectSetInternal(t, "rev-2") |
There was a problem hiding this comment.
suggestion: The old test marked rev-2 (middle revision) as deleting with currentRev: "rev-3", which tested the case where a middle revision is deleting while both lower and higher revisions exist. The new test moves the deletion to rev-1 (lowest revision). Both validate the "skip deleting" filter, but the old arrangement was a more interesting interleaving — it verified that a deleting revision between two active ones is correctly excluded regardless of its position.
Consider keeping deletion on rev-2 and adjusting the expected results accordingly, or adding a second sub-case for middle-revision deletion.
| return nil | ||
| } | ||
|
|
||
| // listSiblingRevisions returns all active revisions belonging to the same ClusterExtension, excluding the current one. |
There was a problem hiding this comment.
suggestion: The godoc here explains why boxcutter needs siblings (avoiding false collisions during handover). That is caller context — it belongs at the call site in buildBoxcutterPhases where WithSiblingOwners is passed. The method itself just returns "all active non-self revisions for the same owner," which is already clear from its name and signature.
Keeping method-level docs focused on what the method does (not why a specific caller needs it) prevents the comment from becoming stale if another caller is added.
| And ClusterExtension reports Progressing as True with Reason Retrying and Message includes: | ||
| """ | ||
| revision object collisions | ||
| """ |
There was a problem hiding this comment.
suggestion: The scenario asserts that the dup CE shows collision and that the higher revision still shows collision after update. But it doesn't verify that the original ${NAME} CE remains Available/Installed throughout the conflict. Adding something like And ClusterExtension "${NAME}" reports Installed as True after the dup's collision assertion would strengthen the test — confirming the original owner is unaffected is arguably the most important property to verify here.
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