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

Allow enabled config, for use by charts depending on this chart conditionally #3162

Merged

Conversation

monoakg
Copy link
Contributor

@monoakg monoakg commented Jul 19, 2023

Linked issue #3160 - Update values schema to allow key enabled

Closes #3160

@welcome
Copy link

welcome bot commented Jul 19, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@manics manics linked an issue Jul 22, 2023 that may be closed by this pull request
jupyterhub/values.schema.yaml Outdated Show resolved Hide resolved
@@ -3,6 +3,9 @@
fullnameOverride: ""
nameOverride:

# enable or disable jupyterhub if it is included as a dependency in a parent helm chart
enabled:
Copy link
Member

Choose a reason for hiding this comment

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

What do other charts typically do? Do they leave this null, or default to true?

Copy link
Member

Choose a reason for hiding this comment

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

I suggest its made null as default here, true/false has no meaning by this chart anyhow so making it null seems less confusing, and both false/true and can be defaults for a chart depending on this - such chart should define enabled to false/true explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

Most charts dont need an entry for this, but we have a strict schema.

Co-authored-by: Simon Li <orpheus+devel@gmail.com>
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Thanks @monoakg!!

From experience and examples from helm's documentation, its clear that enabled indeed is the common naming choice for this.

I've updated the language to make it really clear that this is a feature implemented in a chart conditionally depending on this chart

jupyterhub/values.schema.yaml Outdated Show resolved Hide resolved
jupyterhub/values.yaml Outdated Show resolved Hide resolved
@consideRatio consideRatio changed the title Feature: allow enabled as key in values.yaml Allow enabled config, for use by charts depending on this chart conditionally Jul 23, 2023
@consideRatio consideRatio merged commit 800307a into jupyterhub:main Jul 23, 2023
15 checks passed
@welcome
Copy link

welcome bot commented Jul 23, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jul 23, 2023
jupyterhub/zero-to-jupyterhub-k8s#3162 Merge pull request #3162 from monoakg/feature/allow-enabled-in-values-yaml
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.

Update values schema to allow key enabled
3 participants