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

Portworx: etcdEndpoints list #477

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cabrinha
Copy link

@cabrinha cabrinha commented Apr 21, 2023

What this PR does / why we need it:

So many variables defined at the top of templates/storage-cluster.yaml are not necessary. Instead, defaults should be set in values.yaml.

Regarding this piece of helm code:

    endpoints:
      {{- $endpoints := $etcdEndPoints | split ";" }}
      {{- range $key, $val := $endpoints }}
      - {{$val}}
      {{- end }}

It's not necessary to loop over a list, define the $key when it's not used, but instead we can use the with operator

Simply:

    {{- with .Values.etcdEndpoints }}
    endpoints:
      {{- toYaml . | nindent 4 }}
    {{- end }}

Also, having comments in a single line means that if any line is changed in values.yaml, the in-line comment must be re-aligned and spaced/tabbed all over again.

Why not just have comments above what they're referencing?

thisValue: null # This comment must be moved if default is changed

# This comment never needs to be moved
# thatValue is a list
thatValue: []
# - sane defaults
# - foo
# - bar

Which issue(s) this PR fixes (optional)
Closes #

Special notes for your reviewer:

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

Successfully merging this pull request may close these issues.

2 participants