-
Notifications
You must be signed in to change notification settings - Fork 78
CSV: Add annotations into StrategyDeploymentSpec #350
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
CSV: Add annotations into StrategyDeploymentSpec #350
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hongkailiu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #350 +/- ##
==========================================
- Coverage 39.43% 39.39% -0.05%
==========================================
Files 56 56
Lines 4516 4521 +5
==========================================
Hits 1781 1781
- Misses 2581 2586 +5
Partials 154 154 ☔ View full report in Codecov by Sentry. |
a218ee6
to
ee8365c
Compare
ee8365c
to
7601bc5
Compare
@hongkailiu Thanks for the contribution! Can you tell me the reason for this contribution? We're actively trying to move OLM v0 (which includes the CSV API) into a feature complete state, so we're trying to limit what gets added to the APIs at this point. What are you trying to solve for by adding hardcoded annotations to CSVs as opposed to using the subscription config at runtime on a cluster? |
@hongkailiu Following along here https://issues.redhat.com/browse/OTA-1166 I think there's an even simpler option. OLM will propogate annotations defined on the CSV meta itself to the deployments it creates (see https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/controller/install/deployment.go#L147). So you could just annotate the CSV instead of requiring an update to the API here. Let me know if that's sufficient, and if so I think this pull request isn't needed. We appreciate the work you put into it though! |
Thanks @kevinrizza for the review. My intension is this: And then:
The goal of all this above is that when a deployment is created via the CSV, it has the annotation that we want. I believe this is a similar PR for labels. I am new to the operator world. |
I also found this while reading the code. |
It works. Thanks for the workaround. @kevinrizza /close |
@hongkailiu: Closed this PR. 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. |
The intension is to add the annotations to the deployment ALM creates, like the labels.