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

refactor: remove redundant trimSuffix of new lines after toYaml #2358

Merged

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Aug 21, 2021

In the past toYaml created a new line that we trimmed away - but I contributed a fix to that in helm/helm so now it doesn't in the modern versions of the helm CLI which the JupyterHub helm chart now require anyhow :)

@manics
Copy link
Member

manics commented Aug 23, 2021

Is the minimum Helm version enforced in the chart?

@consideRatio
Copy link
Member Author

consideRatio commented Aug 23, 2021

The minimum Helm version for this chart is 3.5.0, and it will error without that version so it is enforced due to that. Helm 3.5.0 behaves in a way making the trimSuffix part redundant.

I'm not sure when in helm 3 this was fixed because it was first merged and then reverted and then merged back due to them wanting to wait for the change until helm3 sometime as there was some chart that broke when this toYaml behaviour changed.

I concluded also that our helm diff test step in the CI jobs making a upgrade from a previous version also didn't report any changes to the rendered outcome.

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Thanks for confirming. I was worried changes like this might lead to strange error messages for someone using an out of date Helm (as opposed to new syntax which would fail immediately) but it sounds like it's all taken care of 😄

@manics manics merged commit 6ec062d into jupyterhub:main Aug 24, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Aug 24, 2021
jupyterhub/zero-to-jupyterhub-k8s#2358 Merge pull request #2358 from consideRatio/pr/remove-trimsuffix-no-longer-needed
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.

2 participants