Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Helm Operator's own Helm chart should use the new directory for CRDs #231

Closed
jwietelmann opened this issue Jan 21, 2020 · 10 comments
Closed
Labels
chart Related to the Helm chart enhancement New feature or request helm v3 Issue or PR related to Helm v3

Comments

@jwietelmann
Copy link

jwietelmann commented Jan 21, 2020

Re: https://helm.sh/docs/topics/chart_best_practices/custom_resource_definitions/

Describe the feature

Starting with Helm 3, a chart can contain a crds directory to ensure prerequisite CRDs are installed before the release. Helm Operator's chart, however, still requires you to manually install its CRDs separately.

What would the new user story look like?

  1. User is on Helm v3
  2. User installs the helm-operator chart
  3. CRDs are automatically created
  4. Helm release for helm-operator is successfully created
@jwietelmann jwietelmann added blocked needs validation In need of validation before further action enhancement New feature or request labels Jan 21, 2020
@stefanprodan
Copy link
Member

stefanprodan commented Jan 21, 2020

We are going to use Helm v3 to apply the CRD when Helm can also update it.

@jwietelmann
Copy link
Author

@stefanprodan One of the things I don't like about the current way is that I'm wary of installing CRDs from master as instructed when for one reason or another (k8 version, helm version, etc.) I might be using an older chart version that isn't compatible. Not sure about helm-operator specifically, but I've definitely been bit by this a few times with other charts.

My ideal middle ground would be something like:

  • Put the CRDs in crds.
  • Put the kubectl apply instructions in the README anyway. Those of us who know we're installing from scratch and won't need to do it can just skip it.
  • Consider changing the README to say this (note the versioned CRD reference):

Upgrade the HelmRelease CRD:

kubectl apply \
  --wait \
  -f https://raw.githubusercontent.com/fluxcd/helm-operator/chart-<YOUR CHART VERSION>/crds/flux-helm-release-crd.yaml
  • Maybe also output the versioned CRD upgrade command in a NOTES.txt template, too, just for good measure as a reminder if the install fails because of CRD incompatibility.

@stefanprodan
Copy link
Member

The CRD in master is backwards compatible with Helm Operator 1.x.x
If we are going to make a breaking change, we will bump the API version and provide another URL. That being said, I agree that we should be using the git tags and update the docs every time we make a new release to include the release number in the URL.

@jwietelmann
Copy link
Author

I won't keep arguing if the answer ends up being no, but I'd like to make one final attempt to explain why I think this is a good idea:

  • The README already has instructions to install CRDs, which would be exactly the same instructions required to upgrade CRDs.
  • Helm 3 will skip installing CRDs from the crds directory if they already exist.
  • Using the crds directory would have no downsides for anyone following the README, even with no changes to it.
  • People like me can write Helm charts that use helm-operator as a child with or without having to add a bash script or something to deal with CRDs.

As things are, I can't just helm install my parent chart. By moving to crds I could. Anyone following README instructions would still be upgrading the CRDs.

I just don't see any downsides to it.

@stefanprodan
Copy link
Member

If you apply the CRDs with kubectl but the CRDs are also in crds wouldn't helm install fail?

@jwietelmann
Copy link
Author

No, the Helm 3 docs say:

If the CRD already exists, it will be skipped with a warning. If you wish to skip the CRD installation step, you can pass the --skip-crds flag.

Additionally if you kubectl apply ... the CRDs twice, this is all that happens:

customresourcedefinition.apiextensions.k8s.io/helmreleases.helm.fluxcd.io unchanged

@StianOvrevage
Copy link

I just need to echo what @jwietelmann is saying here. This would make installation with Helm v3 much nicer and more versatile. There are no down-sides. It does not break documentation or backwards-compatibility.

It's also a tiny change. I could create a PR for it, but the main issue I guess is agreeing that it's a good idea.

@stefanprodan
Copy link
Member

This will be fixed by #270

@hiddeco
Copy link
Member

hiddeco commented Feb 11, 2020

Given #270 will likely take a while to land, I will subtract the chart change from there as it can also be used for the end-to-end test in #282.

@hiddeco hiddeco added chart Related to the Helm chart helm v3 Issue or PR related to Helm v3 and removed blocked needs validation In need of validation before further action labels Feb 12, 2020
@hiddeco
Copy link
Member

hiddeco commented Feb 12, 2020

Resolved in #287.

@hiddeco hiddeco closed this as completed Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
chart Related to the Helm chart enhancement New feature or request helm v3 Issue or PR related to Helm v3
Projects
None yet
Development

No branches or pull requests

4 participants