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

feat(Chart)!: 📦 add separated chart for CRDs #1223

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

darkweaver87
Copy link
Contributor

@darkweaver87 darkweaver87 commented Oct 14, 2024

What does this PR do?

This PR introduces a separate chart used by the main one to install CRDs.
The dependency between the main chart and the CRDs one will be added once CRDs one will be released.

Motivation

#1141
#1209

More

  • Yes, I updated the tests accordingly
  • Yes, I updated the schema accordingly
  • Yes, I ran make test and all the tests passed

@darkweaver87 darkweaver87 added kind/enhancement New feature or request kind/proposal a proposal that needs to be discussed. labels Oct 14, 2024
@darkweaver87 darkweaver87 force-pushed the feat/crds branch 2 times, most recently from bd35651 to b674cf7 Compare October 14, 2024 08:39
@darkweaver87 darkweaver87 changed the title feat: 📦 add separated chart for CRDs feat!: 📦 add separated chart for CRDs Oct 14, 2024
traefik/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The CI needs to be tested, with a tag on your fork.
  2. The documented CRDs upgrade process should still work. I see (at least) two options:
    a. Keep and copy crd files
    b. Update CRD upgrade instruction to use them in the new directory

@darkweaver87 darkweaver87 force-pushed the feat/crds branch 2 times, most recently from e9619df to 6a67459 Compare October 21, 2024 06:50
@darkweaver87
Copy link
Contributor Author

darkweaver87 commented Oct 21, 2024

Tested on my own account as seen with you 👍

@mloiseleur mloiseleur changed the title feat!: 📦 add separated chart for CRDs feat(Chart)!: 📦 add separated chart for CRDs Nov 6, 2024
@mloiseleur
Copy link
Contributor

mloiseleur commented Nov 19, 2024

Add the CRDs as templates within the chart like in the cert-manager chart

This morning, I have explored this option. I have encountered two major issues:

  1. On install: It fails to deploy dashboard IngressRoute (CRDs are not loaded at this stage)
  2. On upgrade: It fails to deploy CRD: invalid ownership on metadata and annotations

I haven't found a way to switch CRDs from current Helm CRD management to Helm Template management without failure. Instructions can be provided to patch resources beforehand, and thus get around this failure. Maybe it's possible to deploy IngressRoute in post-install hook, but should we really do this ?

We are really fighting the tide, here.

This use case of selectively picking CRDs is specific. It's clearly not the common use case.

I think we are left with two options on this:

  1. Option Standalone CRD Chart:
    • The common use case works is not modified (current Chart stays as it is today)
    • Users has to opt-in to use this new Chart
    • There is no link between them.
    • Like karpenter, we provide two Charts, the current one with CRDs and a new one with only CRDs.
  2. Option Documentation:
    • Instructions in the README on how to install CRDs separately
    • Users with specifics needs on CRDs deploy them separately and can install the Chart without CRDs after

@jnoordsij @darkweaver87 : Wdyt ?

@mloiseleur
Copy link
Contributor

mloiseleur commented Nov 19, 2024

Additional notes:

  • Helm maintainers have started to work on Helm v4, see here.
  • CRDs management are discussed in the roadmap.
    Helm maintainers have also detailed why it's working this way in HIP-0011.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot/no-merge kind/enhancement New feature or request kind/proposal a proposal that needs to be discussed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don’t bundle Gateway API CRDs CRD only Helm chart for Traefik
3 participants