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

helm chart: refactor to use consistent modern syntax #425

Merged

Conversation

consideRatio
Copy link
Collaborator

@consideRatio consideRatio commented Aug 21, 2021

I recognize a lot of practices from the JupyterHub Helm chart made it into this Helm chart. In this PR make the templates consistent in its syntax across helm template files and align with the best practices that have now also to some degree made its way all to Helms official documentation.

This PR should have no influence on anything in practice.

About the various kinds of changes made:

  • trimSuffix "\n" is no longer needed with modern versions of helm and would be harmless to remove anyhow
  • A replica count had a default 1 that served no purpose
  • Sometimes it was . | toYaml | nindent 4, othertimes it was toYaml . | nindent 4. I made it consistently like the former example.
  • Some whitespace chomping practices enforced:
    • always chomp right on the first line in a template file
    • almost always chomp left but not to the right elsewhere
  • if statements became with statements where possible to avoid duplicating a reference to some variable (with statements also function as as if statements).
  • redundant parenthesis removed

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Awesome thanks so much!

@jacobtomlinson jacobtomlinson merged commit 3457732 into dask:main Aug 23, 2021
@consideRatio consideRatio deleted the pr/refactor-syntax-for-consistency branch March 25, 2022 01:15
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.

None yet

2 participants