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

Modernize our Helm chart #1414

Closed
20 of 23 tasks
consideRatio opened this issue Oct 10, 2021 · 1 comment
Closed
20 of 23 tasks

Modernize our Helm chart #1414

consideRatio opened this issue Oct 10, 2021 · 1 comment
Labels
code:helm-chart Helm template changes. maintenance Under the hood improvements and fixes

Comments

@consideRatio
Copy link
Member

consideRatio commented Oct 10, 2021

Our Helm chart could use some love and modernizing - to make it easier to reason about, catch mistakes, and modify (rather than just for the sake of modernizing). This isn't about using a framework or anything like that - just cleaning up.

Directly helpful updates

Misc bugs spotted

Refactoring updates

  • This was less work in this than expected, the nested checkboxes are covered by helm-chart: minor refactoring of syntax #1419
    • Use modern with syntax instead of if
      # old syntax - repeating the value needed
      {{- if .Values.somethingX.somethingY }}
      something: {{ .Values.somethingX.somethingY }}
      {{- end }}
      # modern syntax - no need to repeat variable
      # with statements are also conditionals: a truthy value is required
      {{- with .Values.somethingX.somethingY }}
      something: {{ . }}
      {{- end }}
    • Use consistent indentation practices and whitespacing practices.
      Practices I've championed have partially been upstreamed all the way to helm's official documentation.
      • Always whitespace chomp left.
      • Also whitespace chomp right in the first entry of a file or defined template.
      • Use nindent and left whitespace chomp instead of indent to be able to indent lines that have otherwise required 0 indentation to not error for multiline content.
    • Stop trimming new lines after toYaml - it's no longer needed with modern helm that we require already.
        # before
        {{- with .Values.annotations }}
        annotations:
          {{- . | toYaml | trimSuffix "\n" | nindent 4 }}
        {{- end }}
        # after
        {{- with .Values.annotations }}
        annotations:
          {{- . | toYaml | nindent 4 }}
        {{- end }}
  • Use port names instead of port numbers in network config

Categorization of PRs

In order to do work on the Helm templates, I wanted to make sure there isn't a great conflict with open PRs. I went through all the open PRs and labelled them based on the type of code that is changed based on the labels code:python, code:html/js/css, code:helm-chart. These are the PRs open about changes to the helm chart.

Only two small changes in two PRs were made besides my own PRs, so I figure it's fine for me to do some refactoring of the templates at this point without causing disruptions to existing PRs.

@consideRatio consideRatio added maintenance Under the hood improvements and fixes code:helm-chart Helm template changes. labels Oct 10, 2021
@consideRatio
Copy link
Member Author

I think most if not all of this is done now. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code:helm-chart Helm template changes. maintenance Under the hood improvements and fixes
Projects
None yet
Development

No branches or pull requests

1 participant