Update boxcutter integration for sibling owners API#2699
Draft
perdasilva wants to merge 6 commits into
Draft
Conversation
Adapt the ClusterObjectSet controller to boxcutter's replacement of `WithPreviousOwners` with `WithSiblingOwners`. The new API expects all sibling revision owners (both previous and future) so that boxcutter can distinguish between unknown controllers (collisions) and known sibling revisions (not collisions) during revision handover. Changes: - Replace `WithPreviousOwners` with `WithSiblingOwners` in `buildBoxcutterPhases` - Add `listSiblingRevisions` that returns all active sibling revisions regardless of revision number, while keeping `listPreviousRevisions` for the archiving use case - Add `managedBy` parameter to `NewObjectEngine` call (new required param, passing "" to use the default "boxcutter" value) - Add unit tests for `listSiblingRevisions` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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 |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Tests that when two ClusterExtensions install the same package, the collision is detected at install time and continues to be reported after the conflicting extension is upgraded to a different version. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the operator-controller’s Boxcutter integration to match Boxcutter’s new “sibling owners” API so that revision handover can correctly account for both older and newer revisions when detecting ownership/collisions.
Changes:
- Updated Boxcutter reconcile options from
WithPreviousOwnerstoWithSiblingOwners, backed by a newlistSiblingRevisionshelper. - Extended Boxcutter
NewObjectEngineconstruction to pass the new requiredmanagedByparameter. - Added unit tests covering sibling revision listing behavior (both directions + filtering of archived/deleting/mismatched owner label).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/operator-controller/controllers/revision_engine_factory.go | Passes new required param to Boxcutter object engine constructor. |
| internal/operator-controller/controllers/clusterobjectset_controller.go | Adds listSiblingRevisions and wires it into Boxcutter options. |
| internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go | Adds unit tests for the sibling revision listing helper. |
| go.mod | Updates dependency versions and adds a local replace for boxcutter. |
| go.sum | Updates checksums to match module version changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+319
to
+320
|
|
||
| replace pkg.package-operator.run/boxcutter => /Users/pegoncal/repos/perdasilva/boxcutter |
Comment on lines
37
to
53
| helm.sh/helm/v3 v3.20.2 | ||
| k8s.io/api v0.35.3 | ||
| k8s.io/apiextensions-apiserver v0.35.3 | ||
| k8s.io/apimachinery v0.35.3 | ||
| k8s.io/apiserver v0.35.3 | ||
| k8s.io/api v0.35.4 | ||
| k8s.io/apiextensions-apiserver v0.35.4 | ||
| k8s.io/apimachinery v0.35.4 | ||
| k8s.io/apiserver v0.35.4 | ||
| k8s.io/cli-runtime v0.35.1 | ||
| k8s.io/client-go v1.5.2 | ||
| k8s.io/component-base v0.35.3 | ||
| k8s.io/component-base v0.35.4 | ||
| k8s.io/klog/v2 v2.140.0 | ||
| k8s.io/kubernetes v1.35.0 | ||
| k8s.io/utils v0.0.0-20260319190234-28399d86e0b5 | ||
| pkg.package-operator.run/boxcutter v0.13.1 | ||
| sigs.k8s.io/controller-runtime v0.23.3 | ||
| sigs.k8s.io/controller-tools v0.20.1 | ||
| sigs.k8s.io/crdify v0.5.1-0.20260309184313-54162f2e3097 | ||
| sigs.k8s.io/structured-merge-diff/v6 v6.3.2 | ||
| sigs.k8s.io/structured-merge-diff/v6 v6.4.0 | ||
| sigs.k8s.io/yaml v1.6.0 |
| if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{ | ||
| labels.OwnerNameKey: ownerLabel, | ||
| }); err != nil { | ||
| return nil, fmt.Errorf("listing revisions: %w", err) |
Assert that upgrading the conflicting extension creates a new revision
(${NAME}-dup-2), that revision also reports collisions, and the original
extension's revision (${NAME}-1) remains Succeeded/Available — proving
the higher revision number doesn't cause object takeover.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The active revisions assertion timed out because the first dup revision gets archived when the second is created during upgrade, so both dup-1 and dup-2 are not simultaneously active. Keep the core assertions: collision persists after upgrade and original extension's COS stays Succeeded/Available. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
go.mod:320
- The
replace pkg.package-operator.run/boxcutter => /Users/...directive hard-codes a developer-local absolute path, which will break builds/tests for anyone else (including CI) and makes the module graph non-reproducible. Please remove this before merge, or switch to a temporary branch/tag-based module version if you need to test unpublished boxcutter changes.
replace pkg.package-operator.run/boxcutter => /Users/pegoncal/repos/perdasilva/boxcutter
go.mod:131
- This change updates a number of indirect dependencies (go-openapi/, cel-go, x/, etc.) that aren’t called out in the PR summary and don’t appear directly related to the boxcutter sibling-owners API update. Consider reverting these unrelated version bumps or splitting them into a separate PR to keep the dependency change surface area focused and easier to review/rollback.
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-openapi/jsonpointer v0.23.1 // indirect
github.com/go-openapi/jsonreference v0.21.5 // indirect
github.com/go-openapi/swag v0.26.0 // indirect
github.com/go-openapi/swag/cmdutils v0.26.0 // indirect
github.com/go-openapi/swag/conv v0.26.0 // indirect
github.com/go-openapi/swag/fileutils v0.26.0 // indirect
github.com/go-openapi/swag/jsonname v0.26.0 // indirect
github.com/go-openapi/swag/jsonutils v0.26.0 // indirect
github.com/go-openapi/swag/loading v0.26.0 // indirect
github.com/go-openapi/swag/mangling v0.26.0 // indirect
github.com/go-openapi/swag/netutils v0.26.0 // indirect
github.com/go-openapi/swag/stringutils v0.26.0 // indirect
github.com/go-openapi/swag/typeutils v0.26.0 // indirect
github.com/go-openapi/swag/yamlutils v0.26.0 // indirect
Comment on lines
+38
to
+44
| k8s.io/api v0.35.4 | ||
| k8s.io/apiextensions-apiserver v0.35.4 | ||
| k8s.io/apimachinery v0.35.4 | ||
| k8s.io/apiserver v0.35.4 | ||
| k8s.io/cli-runtime v0.35.1 | ||
| k8s.io/client-go v1.5.2 | ||
| k8s.io/component-base v0.35.3 | ||
| k8s.io/component-base v0.35.4 |
The ${NAME} variable resolves to the dup extension name after the
second apply, so ${NAME}-1 incorrectly references the dup's COS
rather than the original's. The collision message itself proves
objects weren't taken over — if they had been adopted, boxcutter
would not report a collision.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Check that ${NAME}-2 (the dup's second ClusterObjectSet) exists and
reports collision after upgrading the conflicting extension to v1.0.1.
This proves the upgrade created a new revision and that the higher
revision number doesn't cause object takeover.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WithPreviousOwnerswithWithSiblingOwnersto adapt to boxcutter's new sibling owners API (boxcutter@2a11a21)listSiblingRevisionsmethod that returns all active sibling revisions (both previous and future) so boxcutter can properly classify sibling owners and avoid reporting false collisions during revision handovermanagedByparameter toNewObjectEnginecall (new required param in boxcutter, passing""to use the default"boxcutter"value)listSiblingRevisionsTest plan
go test ./internal/operator-controller/controllers/...)make test-experimental-e2e— all 49 scenarios)listSiblingRevisionsunit tests cover: both-direction sibling listing, archived/deleting exclusion, owner label filtering, missing owner label🤖 Generated with Claude Code