Provide CRDs in nack helm chart

[manuelottlik]

Hey guys,

when installing the nack helm chart I figured that the CRDs for the JetStream controller are not installed. This is possible in an helm chart by providing a crds folder next to templates (see helm docs).

I think the nack helm chart should provide these CRDs. Do you agree? I would be happy to provide a PR for this.

[wallyqs]

Thanks @manuelottlik, they used to be part of the Helm chart but we took them out because that meant that updates to the CRDs were not being applied and these CRDs can have new fields fairly often. Managing CRDs via Helm has many gotchas as described in the following discussion from Helm maintainers, so for now decided to keep them out of the repo: https://github.com/helm/community/blob/main/hips/hip-0011.md#helm-is-still-working-on-thus-but-we-cannot-solve-it-alone

[manuelottlik]

Thank you for the quick reply, @wallyqs. There are two things I would like to say:

1. Couldn't we just provide the CRDs in the crds folder and give a migration guide whenever there is a new version with changed definitions? The migration guide could contain a kubectl apply -f ... link that users of the helm chart could copy to update their CRDs in their cluster. Since users of the helm chart have to do this anyways in order to update this helm chart, it feels like the best UX for me.
2. If CRDs will not be included, this should be stated somewhere in the documentation / README of this helm chart. A link to the CRD file etc. would be nice. New versions that require updated CRDs should also have some kind of migration guide. I would open a PR if you want me to.

What do you think?

[wallyqs]

I think both sounds good, just noticed that they are not mentioned in the readme, agree that we need a migration guide / better strategy for handling updates to the CRDs as well... For a bit there used to be a copy of the CRDs both in these repo and in the nack repo but eventually only left this copy since there was some confusion with duplication: https://github.com/nats-io/nack/blob/main/deploy/crds.yml

[manuelottlik]

What about a mix of the two options?

1. Providing the CRDs in /crds makes it more user-friendly when getting started with the nack helm chart as you can just throw it in your cluster without too many steps.
2. Providing a migration guide with kubectl apply -f ... whenever there are changes to the CRDs.
3. Leaving a note in the README that these CRDs do not update themselves and there is a migration guide to follow.

I've seen many helm charts follow this way already and would love to see it adapted here as well – if you agree of course.

[wallyqs]

thanks @manuelottlik, could you point to some of the examples of Helm charts that follow something like that?
Maybe what we need is a separate Helm chart just for the CRDs? Like nack-crds so that their lifecycle is different from the controller?

[manuelottlik]

Hey @wallyqs, of course:

• https://github.com/prometheus-community/helm-charts/tree/main/charts/kube-prometheus-stack
• https://github.com/traefik/traefik-helm-chart/tree/master/traefik
• https://github.com/grafana/helm-charts/tree/main/charts/agent-operator
• https://github.com/zalando/postgres-operator/tree/master/charts/postgres-operator
• https://github.com/bitnami/charts/tree/master/bitnami/grafana-operator
• and many bitnami charts more

[wallyqs]

Thanks, I think our approach might be similar to how grafana does with the agent-operator in that they are synced from another repo:
https://github.com/grafana/helm-charts/tree/main/charts/agent-operator#crds
https://github.com/grafana/agent/tree/main/production/operator/crds

[manuelottlik]

Okay @wallyqs, so you plan on keeping it that way? If thats the case, this issue can be closed.

[wallyqs]

Hi @manuelottlik I meant that we could do a similar approach like grafana does and keep a copy of the crds in both places probably.

[manuelottlik]

Hey @wallyqs, I see. Should I just provide a PR with the copied helm charts from the other repository? Should there be any note in the README about the need to keep these up to sync or will you handle that internally?

[mathieubergeron]

Hi @manuelottlik, @wallyqs,

I wonder why this issue was closed? CRDs are still provided separately, which complicates automatically updating our dependency on nack.

Thanks!

[manuelottlik]

Hey @mathieubergeron, that's right. I closed it as "not planned" because I did not receive any more feedback. I still think it's a good idea but stopped using the CRDs, so I didn't put in more effort.

[wallyqs]

Sorry @manuelottlik should have responded that your approach sounded good. I think it might be fine to put them now in the Helm charts as well with the caveat that in case there are updates to the CRDs they won't be updated and still has to be done manually.

[christiang830]

As a gitops user it would be really really good to have the CRDs included in the chart. Is there an update when this open PR is going to be merged @manuelottlik ? Thank you very much for your efforts

[manuelottlik]

Hey @christiang830, I stopped working on this since we do not use the CRDs at the moment, you are welcome to create a PR.

[christiang830]

Hi @manuelottlik, thanks for the quick reply. But there is already an open PR #694

[manuelottlik]

Ah oops, did not see that. I am not a maintainer, so I do not have control about merging it.

[christiang830]

Ah sorry, assumed from the previous replies that you actually were. My bad