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

fix: match file names in helm registry index and release assets #1373

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

rauno56
Copy link
Contributor

@rauno56 rauno56 commented Jul 21, 2024

Adding a separate Helm chart for CRDs.

❗ The charts are not strictly dependent to one another and need to be separately managed and installed. Meaning we need to document that both of those need to be updated and installed.

Fair questions that might arise:

Why isn't CRD chart a declared dependency for odigos?

Helm merges dependent resources together with the ones from dependencies. We'd be exactly where we are right now if we did that.

Why not use the official crd support?

Helm really doesn't "support" CRDs. It's an opt-out feature of "we don't know how we want to do it so we don't for now". Resources declared as CRDs(different from declaring them under templates) are not updated.

How do other charts do it then?

All charts I've encountered(cert-manager, jaeger, contour) have CRDs under a flag and include them in the same chart as the rest of the resources but then do not depend on them in in the install. CRs are created either only by the user or later in the applications life-cycle. This doesn't work for us since we declare Odigos Config among resources in the application chart. That begs the question whether Odigos Config must be a CR at all or could it be a config map instead - we wouldn't get "type-checking" by k8s API but would gain some flexibility and could release our charts as one.

@blumamir
Copy link
Collaborator

I support migrating the odigos config into a config map to make the helm installation simpler. The type checking is nice, but I think managing the CR should not be done manually anyway (either via helm or the CLI on day).

@edeNFed @RonFed - WDYT?

@edeNFed
Copy link
Contributor

edeNFed commented Jul 22, 2024

I support migrating the odigos config into a config map to make the helm installation simpler. The type checking is nice, but I think managing the CR should not be done manually anyway (either via helm or the CLI on day).

@edeNFed @RonFed - WDYT?

I totally agree. The type checking is not worth the complication of another CRD, another helm chart, etc.

@rauno56
Copy link
Contributor Author

rauno56 commented Jul 22, 2024

Thanks for the feedback.

I tend to agree as well and the signal about the importance of Odigos Config validation on the k8s API level from you is valuable. Just to be absolutely clear: That also means we ought to avoid defining CRs in the charts in the future as well.

An alternative would be to just create a CR with default values during startup in some cases, but it's not an option for Odigos Config because we want to template that during the install of the chart.

@rauno56 rauno56 changed the title feat: release helm CRDs in a separate helm chart fix: match file names in helm registry index and release assets Jul 22, 2024
@rauno56 rauno56 requested a review from blumamir July 22, 2024 08:47
@rauno56 rauno56 merged commit 0df64e9 into odigos-io:main Jul 22, 2024
13 checks passed
@rauno56 rauno56 deleted the feat/release-crd-chart branch July 22, 2024 10:14
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.

3 participants