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(CRDs-schema): add global and enabled placeholders for subchart/dependency #1348

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

Conversation

ArlonAntonius
Copy link
Contributor

@ArlonAntonius ArlonAntonius commented Feb 26, 2025

What does this PR do?

Fixes #1311
Superseeds #1334
TL:DR Allows the usage of the traefik-crds chart as a subchart/dependency.

Motivation

To use the traefik-crds Helm Chart as a dependency in your projects, the validation needs to be slightly looser to support the usage of the chart. Helm passes specific values by default (global in this case), causing the Schema Validation to give an error.

In line with other Helm Charts (like cert-manager), it would be beneficial to allow users to set:

  • global to fix the error that will always be caused by Helm passing the global values
  • enabled to provide users with the default (recommended) flag for conditions on Helm Charts

The one thing that I did, that is more opinionated and would like the opinion of Traefik's maintainer on is the usage of additionalProperties on the global object. Cert-manager specifically provides a set of variables, defeating part of the feature's purpose (in my opinion).
https://github.com/cert-manager/cert-manager/blob/23d3aea079ce88cec6f9394f4a4027720f8d7c2a/deploy/charts/cert-manager/values.schema.json#L686

More

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

@ArlonAntonius
Copy link
Contributor Author

@mloiseleur Especially interested to hear your take on this as I know you are involved in #1334 and have not provided a specific opinion on #1331 yet

@mloiseleur
Copy link
Member

@ArlonAntonius I guess you mean on #1311 . We can add required properties from some use cases and keep the benefits of the schema.

FYI, the schema is generated from values:

# Requires to install schema generation plugin beforehand
# $ helm plugin install https://github.com/losisin/helm-values-schema-json.git
schema:
cd traefik && helm schema
cd traefik-crds && helm schema

Would you please update your PR accordingly, so the schema will be kept ?

@ArlonAntonius
Copy link
Contributor Author

@mloiseleur Made the required changes, let me know if this aligns correctly with Traefik's coding and documentation standards 😉
If it's easier to discuss something in real-time, feel free to let me know.

@mloiseleur
Copy link
Member

@ArlonAntonius See my suggestion for improved documentation. Otherwise, LGTM.

@mloiseleur mloiseleur changed the title Fix: Allow using the traefik-crds chart as a subchart/dependency feat(CRDs-schema): add global and enabled placeholders for subchart/dependency Mar 3, 2025
@ArlonAntonius
Copy link
Contributor Author

@mloiseleur Included your change 🚀 Any chance we can include this in the release you have planned in #1351 ?

@mloiseleur
Copy link
Member

@ArlonAntonius You need to rebase this PR (and/or allow edit from maintainers)

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

Successfully merging this pull request may close these issues.

Json schema validation fails when using traefik-crds as chart dependency
3 participants