Skip to content
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

[jaeger-operator] Refactor CRD to allow Jaeger CR creation on initial install #372

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

mjnagel
Copy link
Contributor

@mjnagel mjnagel commented Jun 7, 2022

Signed-off-by: Micah Nagel micah.nagel@parsons.com

What this PR does

#368 moved the CRD out of the special chart/crds helm chart folder and into chart/crd. While this did result in the chart making use of the already existing chart/templates/crd.yaml template to install the CRD, the other result is that you must install the helm chart without jaeger.create set to true, then upgrade after the CRD has been installed. If we make use of the chart/crds folder as recommended by the first method here then the CRDs are installed before Helm applies the other manifests.

This unknowingly introduced what I would consider a breaking change into the chart - you now are required to install the operator separately from the Jaeger CR (install then upgrade).

Fixes #373

Checklist

  • DCO signed
  • Commits are GPG signed
  • Chart Version bumped
  • Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
  • README.md has been updated to match version/contain new values

@mjnagel
Copy link
Contributor Author

mjnagel commented Jun 7, 2022

The biggest downside of moving this to the crds folder is that Helm templating is not supported. My understanding is that the labels/annotations added in the previous PR are not functionally necessary so this is not an issue, but if those are required the issue is trickier.

An alternative I have seen if we want the best of both worlds (helm templating in the CRD + installs with jaeger.create): a pre-install / pre-upgrade job that mounts a configmap with the CRDs and applies them against the cluster. Would be more of a lift and introduce a new dependency on a kubectl image, but I could modify this PR if that seems like a better route.

@batazor
Copy link
Member

batazor commented Jun 7, 2022

@czomo Maybe we should put crd.yaml back in the crds folder?

@czomo
Copy link
Contributor

czomo commented Jun 7, 2022

I completely missed that use case cause I tested it on already updated CRD/existing cluster. Thanks for pointing this out. Yes, we should move it back to crds

charts/jaeger-operator/README.md Show resolved Hide resolved
charts/jaeger-operator/crds/crd.yaml Outdated Show resolved Hide resolved
@mjnagel mjnagel changed the title [jaeger-operator] Refactor CRD to allow Jaeger CR creation [jaeger-operator] Refactor CRD to allow Jaeger CR creation on initial install Jun 7, 2022
@czomo
Copy link
Contributor

czomo commented Jun 10, 2022

@mjnagel Let's squash it
@batazor Would you like to look at it as code owner? Thanks

@mjnagel
Copy link
Contributor Author

mjnagel commented Jun 10, 2022

@czomo can do, should I also update to a new patch version since #371 was merged?

@czomo
Copy link
Contributor

czomo commented Jun 10, 2022

@czomo can do, should I also update to a new patch version since #371 was merged?

Yes, indeed. Rebase and squash is all we need 🚀

Signed-off-by: Micah Nagel <micah.nagel@parsons.com>
@mjnagel
Copy link
Contributor Author

mjnagel commented Jun 10, 2022

Yes, indeed. Rebase and squash is all we need 🚀

Should be good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[jaeger-operator] Breaking change to Jaeger CR creation in 2.31.0+
3 participants