⚠️ fix: rename owner label and clarify variable naming #2349
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
camilamacedo86
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR improves code clarity and consistency by consolidating label constants and fixing misleading variable naming. The changes rename the ClusterExtensionRevision owner label from "olm.operatorframework.io/owner" to "olm.operatorframework.io/owner-name" and remove code duplication.
- Removed duplicate
ClusterExtensionRevisionOwnerLabelconstant and unified usage throughlabels.OwnerNameKey - Renamed
storeLblsvariable torevisionAnnotationsto accurately reflect that values are stored as annotations, not labels - Updated all references across controllers, appliers, and test files for consistency
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Removed duplicate constant, added labels import, updated all references to use labels.OwnerNameKey |
| internal/operator-controller/controllers/clusterextension_controller.go | Renamed storeLbls to revisionAnnotations with clarifying comment, updated label reference |
| internal/operator-controller/applier/boxcutter.go | Removed unused controllers import, updated all label references to use labels.OwnerNameKey |
| internal/operator-controller/applier/boxcutter_test.go | Removed unused controllers import, updated test assertions with new label constant and hardcoded string value |
| internal/operator-controller/controllers/clusterextensionrevision_controller_test.go | Added labels import, updated test fixture to use labels.OwnerNameKey |
| internal/operator-controller/controllers/clusterextensionrevision_controller_internal_test.go | Added labels import, updated all test fixtures to use labels.OwnerNameKey |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
requested review from
anik120,
joelanford,
pedjak,
perdasilva and
tmshort
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Could you help us by overwriting it? |
| }, | ||
| Labels: map[string]string{ | ||
| "olm.operatorframework.io/owner": "test-123", | ||
| "olm.operatorframework.io/owner-name": "test-123", |
There was a problem hiding this comment.
Should labels.OwnerNameKey be used here, and in other places?
(Which might just be the t.Log() below.)
There was a problem hiding this comment.
I think it is the only place that is not using the const so make sense besides be a test
There was a problem hiding this comment.
@tmshort done
Could you pelase lGTM and allow it get merged?
Rename ClusterExtensionRevision owner label from `owner` to `owner-name` for consistency. Also rename misleading `storeLbls` variable to `revisionAnnotations` to clarify it contains annotations, not labels. This removes the duplicate ClusterExtensionRevisionOwnerLabel constant and uses labels.OwnerNameKey directly throughout the codebase.
camilamacedo86
force-pushed
the
fix-owner
branch
from
17e40e1 to
f040c87
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2349 +/- ##
==========================================
+ Coverage 74.23% 74.30% +0.06%
==========================================
Files 91 91
Lines 7239 7239
==========================================
+ Hits 5374 5379 +5
+ Misses 1433 1429 -4
+ Partials 432 431 -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:
|
|
/lgtm |
|
/override e2e/upgrade-experimental-e2e |
|
@tmshort: /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. In 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-experimental-e2e |
|
@tmshort: Overrode contexts on behalf of tmshort: upgrade-experimental-e2e In 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 12ac242
into
operator-framework:main
3 participants
Rename ClusterExtensionRevision owner label from
ownertoowner-namefor consistency. Also rename misleadingstoreLblsvariable torevisionAnnotationsto clarify it contains annotations, not labels.This removes the duplicate ClusterExtensionRevisionOwnerLabel constant and uses labels.OwnerNameKey directly throughout the codebase.