-
Notifications
You must be signed in to change notification settings - Fork 525
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
Move otel operator CRDs to templates #1175
Conversation
8b655b9
to
9d0f99d
Compare
Looks like the failing test is flaky, it passes locally for me. |
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.
Similar to how managing the CRDs via the crd
directory is optional I think installing the CRDs via the template should be optional as well. We should add a new section to the values.yaml that allows controlling when the templated CRDs are installed.
You will need to bump the minor version of the chart and regenerate the examples via |
9d0f99d
to
bbc87cd
Compare
Does a top-level |
Since we're allowing templating, it feels possible someone will want to set a specific value, so I think a section called |
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
+1 to tyler's recommendation |
charts/opentelemetry-operator/templates/crds/crd-opentelemetry.io_opampbridges.yaml
Outdated
Show resolved
Hide resolved
a0f5519
to
413977d
Compare
Added a It's increasingly annoying to automatically convert CRDs published by the operator into the form the Helm Chart wants. I added a make function to make this easier, but at some point we may want to look for a more holistic solution here. @TylerHelmuth can you have another look? Also @pavolloffay. |
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.
🎉
413977d
to
4c80273
Compare
4c80273
to
fe7e411
Compare
I think something I'm missing here is that this change only works with cert-manager enabled. I should inject the On that note, we should really run the e2e tests with cert-manager disabled. |
Currently the chart has a cert-manager requirement by default, but supports other options. We definitely need the CRDs to work without the cert manager like they were before. I'm ok with the CI continuing to use cert manager for now since that is our default requirement |
Thinking about it a bit more, that change isn't necessary in this PR, as there's no conversion webhooks defined, and the CRDs don't need a caBundle. So we can merge this as-is and solve this problem in the actual upgrade do 0.99.0. |
6ec7a7b
to
5a8651f
Compare
5a8651f
to
0aa80fc
Compare
I've added running e2e tests without certmanager to the CI. We can remove it later if you want, but I'd like to have confidence everything works right in both cases for the CRD migration. |
0aa80fc
to
93bb8e6
Compare
.../opentelemetry-operator/templates/admission-webhooks/operator-webhook-with-cert-manager.yaml
Show resolved
Hide resolved
* Move otel operator CRDs to templates * Allow CRD creation to be disabled * Fix webhook cert annotations when cert-manager disabled * Run operator tests without cert-manager * Add README note about CRD creation * Apply suggestions from code review --------- Co-authored-by: Tyler Helmuth <[email protected]>
* Move otel operator CRDs to templates * Allow CRD creation to be disabled * Fix webhook cert annotations when cert-manager disabled * Run operator tests without cert-manager * Add README note about CRD creation * Apply suggestions from code review --------- Co-authored-by: Tyler Helmuth <[email protected]>
Partially resolves #1167. See the issue for more details. I ended up patching both the webhook name and the certificate name in addition to the namespace in the CRD.
I verified the upgrade procedure manually.