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 previously ignored revisionHistoryLimit config #3357

Merged

Conversation

SchutteJan
Copy link
Contributor

Sometimes Helm reports type float64 for integer numbers set in values.yaml. See this Helm issue for an example:
helm/helm#4982

This fix replaces the type check with a simple check + integer cast.

Copy link

welcome bot commented Mar 7, 2024

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

If you haven't done so already, check out Jupyter's Code of Conduct.
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! 🎉

Comment on lines 8 to 9
{{- if .Values.hub.revisionHistoryLimit }}
revisionHistoryLimit: {{ .Values.hub.revisionHistoryLimit | int }}
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to set this to 0 as well, and that was the point of having the typeIs check. We could also change this check to a "not null" check since we have a schema for the helm chart enforcing that makes it either null or int.

Do you have an example reproduction of the bug you look to fix with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, forgot that 0 was treated as a false in this case. I've changed the check to check for not-nil.

Setting the following value file will not render the revisionLimitHistory in the chart:

hub:
  revisionHistoryLimit: 1

This is on helm version:

version.BuildInfo{Version:"v3.14.2", GitCommit:"c309b6f0ff63856811846ce18f3bdc93d2b4d54b", GitTreeState:"clean", GoVersion:"go1.21.7"}

Sometimes Helm reports type float64 for integer numbers set in values.yaml.
This fix replaces the type check with a nil check.
@SchutteJan SchutteJan force-pushed the replace-type-check-revision-limit branch from 6e3512a to 1b1fd35 Compare March 7, 2024 10:15
@consideRatio consideRatio changed the title Replace revisionHistoryLimit type check Fix previously ignored revisionHistoryLimit config Mar 7, 2024
@consideRatio
Copy link
Member

Nice catch @SchutteJan! I could reproduce the issue and concluded that this PR fixes it!

Reproduction steps I took:

tools/generate-json-schema.py
# manually update jupyterhub/values.yaml to provide various defaults for hub.revisionHistoryLimit
helm template jupyterhub --show-only templates/hub/deployment.yaml | grep revision

@consideRatio consideRatio merged commit 5c419fb into jupyterhub:main Mar 7, 2024
14 checks passed
Copy link

welcome bot commented Mar 7, 2024

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 Mar 7, 2024
jupyterhub/zero-to-jupyterhub-k8s#3357 Merge pull request #3357 from SchutteJan/replace-type-check-revision-limit
@consideRatio
Copy link
Member

I also verified this was fixing the issue in all places, and that there wasn't any other issues related to using typeIs "int" checks on integers.

Thank you @SchutteJan! ❤️ 🎉 🌻

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

Successfully merging this pull request may close these issues.

None yet

2 participants