🌱 (fix) ClusterExtensionRevision had inconsistent owner labeling and confusing code#2344
🌱 (fix) ClusterExtensionRevision had inconsistent owner labeling and confusing code#2344camilamacedo86 wants to merge 1 commit into
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2344 +/- ##
==========================================
+ Coverage 74.19% 74.30% +0.10%
==========================================
Files 91 91
Lines 7228 7239 +11
==========================================
+ Hits 5363 5379 +16
+ 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:
|
5fafe40 to
8696a77
Compare
|
Is there a use case for querying by any of the values that are currently in annotations? As far as I know, those annotations are there to serve as hints to the resolver (?), not to be queried. If there aren't any specific query use cases yet, maybe we should hold off on changing this right now. Annotations have fewer restrictions on their value, so I think we should only put values in labels when we know we need to query by them? Maybe @pedjak would have a better rubric than I do on the label vs annotation debate in this case. |
8696a77 to
b63f090
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 |
b63f090 to
953a533
Compare
953a533 to
dbd066c
Compare
dbd066c to
5bbbf3e
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/operator-controller/applier/boxcutter_test.go:410
- [nitpick] Inconsistent field ordering in ObjectMeta initialization. Here
Labelscomes beforeAnnotations, but in the test case at line 798-804,Annotationscomes beforeLabels. Consider using consistent ordering throughout the file for better maintainability.
Annotations: revisionAnnotations,
},
Spec: ocv1.ClusterExtensionRevisionSpec{
Phases: []ocv1.ClusterExtensionRevisionPhase{
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5bbbf3e to
813e20e
Compare
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.
f0e1b47 to
17e40e1
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @joelanford Thank you for your input.
This PR is doing more than just moving the annotations to labels. I reverted the changes and kept the other fix. BUTI will open a new PR only for that so the smaller change doesn’t get blocked by this discussion.** |
Problem
ClusterExtensionRevision had inconsistent owner labeling and confusing code:
owner-kind+owner-name, but ClusterExtensionRevision used onlyownerstoreLblsactually contained annotationsSolution
Fixed owner label consistency and improved code clarity while keeping bundle metadata in annotations (no breaking changes).
Changes
✅ Owner Label Unification
owner-kind+owner-nameconsistently✅ Code Clarity
storeLbls→ separaterevisionLabels+revisionAnnotations✅ Bundle Metadata Location
Why This Matters