-
Notifications
You must be signed in to change notification settings - Fork 71
✨ Add bundle olm.properties to revision annotations #2471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add bundle olm.properties to revision annotations #2471
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds the olm.properties annotation from bundle metadata to ClusterExtensionRevision annotations. This annotation aggregates properties from the bundle's metadata/properties.yaml file and is important for cluster operators (e.g., OpenShift) to detect compatibility information like olm.maxOpenShiftVersion that might affect cluster upgrades. The implementation selectively adds only the olm.properties annotation to avoid cluttering the revision with potentially confusing CSV annotations.
Changes:
- Added logic to extract and copy the
olm.propertiesannotation from bundle CSV to ClusterExtensionRevision annotations - Introduced a new E2E test step to verify annotation presence on revisions
- Added comprehensive unit tests for the annotation copying behavior
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/operator-controller/rukpak/bundle/source/source.go |
Added constant for olm.properties key and nil check for CSV annotations |
internal/operator-controller/applier/provider.go |
Added getBundleAnnotations helper function to extract CSV annotations from bundle filesystem |
internal/operator-controller/applier/boxcutter.go |
Implemented logic to selectively copy olm.properties annotation to revision annotations during revision generation |
internal/operator-controller/applier/boxcutter_test.go |
Added unit tests for bundle annotation copying, including edge cases for missing properties and CSV annotations |
test/e2e/steps/steps.go |
Added E2E test step function to verify ClusterExtensionRevision annotations |
test/e2e/features/install.feature |
Added E2E test scenario to validate olm.properties annotation appears on revisions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6657401 to
4070119
Compare
pedjak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
/approve |
|
Unfortunately, the |
4070119 to
53472e8
Compare
53472e8 to
52db1fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
52db1fe to
d071ac4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2471 +/- ##
==========================================
+ Coverage 69.52% 69.55% +0.03%
==========================================
Files 102 102
Lines 8339 8354 +15
==========================================
+ Hits 5798 5811 +13
- Misses 2078 2079 +1
- Partials 463 464 +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:
|
d071ac4 to
150f12c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
150f12c to
681e933
Compare
681e933 to
1bbc64a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Per Goncalves da Silva <[email protected]>
1bbc64a to
651e1c8
Compare
camilamacedo86
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, tmshort 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 |
e617310
into
operator-framework:main
Description
The Helm applier currently adds the CSV annotations including the olm.properties annotation, which aggregates the properties present in the metadata/properties.yaml file in the bundle, to the Helm release annotations.
The olm.properties is especially important because it contains properties of interest for other operators on the cluster such as olm.maxOpenShiftVersion, which is used by OpenShift to detect whether there are any operators currently installed that might block an upgrade.
In this PR we add the "olm.properties" annotation to the ClusterExtensionRevision annotations. I've avoided adding all of the CSV annotations because it may contains many that are a bit confusing (e.g.
createdAtandcapabilities). This can be modified in the future after the appropriate discussions. But, because its so visible now (as opposed to hidden with the Helm release secret), it might be prudent to add only was is necessary for now.We don't have an e2e test that checks the Helm release within the release secret for the appropriate annotations. So, I've added one only for the BoxcutterRuntime feature.
Reviewer Checklist