Repository navigation
🌱 stop stamping owner-name on ClusterObjectSet managed objects - #3038
openshift-merge-bot[bot] merged 1 commit into
Conversation
The ClusterObjectSet controller copied the owner-name label off the COS onto every managed object. That label is redundant and fragile: managed objects are already labeled app.kubernetes.io/managed-by by the boxcutter revision engine, the controller never reads owner-name (revision lookups use the spec.group index and teardown uses owner references), and copying it off the COS breaks for standalone ClusterObjectSets created without the label. Remove the propagation; managed-object labeling is now just the managed-by label (matching orb-operator). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title is concise, uses the required seedling icon, and accurately identifies the primary change: stopping owner-name stamping on ClusterObjectSet managed objects. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The pull request description is complete and relevant. It explains the motivation, implementation changes, compatibility impact, testing, and related issue. It does not explicitly address API Go Docum… |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
Full details: Description check
-
Explanation
The pull request description is complete and relevant. It explains the motivation, implementation changes, compatibility impact, testing, and related issue. It does not explicitly address API Go Documentation or Comprehensive Commit Messages, and the reviewer checklist remains unchecked, but these are non-critical omissions because the change has no exported API changes and the description provides sufficient validation details.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford 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 |
openshift-ci
Bot
added
the
approved
e2b424a
into
operator-framework:main
With the ClusterObjectSet controller no longer propagating owner-name onto managed objects (operator-framework#3038), the owner-name label on COD-created revisions serves no purpose: ownership is expressed through the controller owner reference and the revision's spec.group, and the controller never reads owner-name. Generated revisions now carry only the template-hash label. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
2 participants
Description
Follow-up to a review discussion on #3034: the ClusterObjectSet controller copied the
olm.operatorframework.io/owner-namelabel off the COS onto every managed object. This removes that propagation.The copy was:
owner-kindandowner-nameonto every managed object (operator-controller buildsobjLblsinboxcutter_reconcile_steps.go; the applier applies them viaobj.SetLabels(merge(..., objectLabels))inboxcutter.go). The removed line re-setowner-nameto the value already present. Managed objects are also labeledapp.kubernetes.io/managed-by: olm.operatorframework.ioby the boxcutter revision engine (matching orb-operator).cos.Labels[owner-name]), which is absent for standalone ClusterObjectSets created without it..spec.groupindex and teardown uses owner references.Changes
owner-namepropagation inbuildBoxcutterPhases(keeping the collision-protection handling that shared the loop) and drop the now-unusedlabelsimport.e2e impact
Safe.
uninstall.feature(ClusterExtensionResourcesCreatedAndAreLabeled) asserts managed objects carryowner-kind+owner-name, and that continues to hold because those labels come from the applier's baked-inobjectLabels, not from the removed line. No e2e assertsowner-nameon managed objects created by a standalone ClusterObjectSet.Scope / compatibility
owner-nameon the ClusterObjectSet objects themselves and on referred secrets (both applier-set, asserted by the install e2e) is untouched.Testing
go build ./..., full./internal/object-controller/...suite, andgolangci-lintall pass.make e2e/uninstall→ 2 scenarios passed, 26 steps passed (BoxcutterRuntimeenabled, no scenarios skipped), including the@BoxcutterRuntimescenario and theClusterExtension resources are created and labeledstep that checks managed objects still carryowner-kind/owner-name.🤖 Generated with Claude Code