Skip to content

Fix indentation of environmentVariables#60

Merged
anna-yn merged 1 commit intotryretool:mainfrom
discord:fix-env-var-values
Jun 15, 2022
Merged

Fix indentation of environmentVariables#60
anna-yn merged 1 commit intotryretool:mainfrom
discord:fix-env-var-values

Conversation

@goodspark
Copy link
Contributor

When using environmentVariables, the template was being rendered like
this:

          - name: CLIENT_SECRET
            valueFrom:
              secretKeyRef:
                name: retool
                key: google-client-secret
                    - name: asd
            value: asd
          - name: zxc
            valueFrom:
              fieldRef:
                fieldKey: asd

Notice how the indentation for 'asd' is wrong.

When using environmentVariables, the template was being rendered like
this:
```yaml
          - name: CLIENT_SECRET
            valueFrom:
              secretKeyRef:
                name: retool
                key: google-client-secret
                    - name: asd
            value: asd
          - name: zxc
            valueFrom:
              fieldRef:
                fieldKey: asd
```

Notice how the indentation for 'asd' is wrong.
{{- end }}
{{- with .Values.environmentVariables }}
{{ toYaml . | indent 10 }}
{{ toYaml . | indent 10 }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After helm template retool . -f test-values.yaml

          - name: CLIENT_SECRET
            valueFrom:
              secretKeyRef:
                name: retool
                key: google-client-secret
          - name: asd
            value: asd
          - name: zxc
            valueFrom:
              fieldRef:
                fieldKey: asd

Copy link
Contributor

Choose a reason for hiding this comment

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

oh great catch! I have one request: instead of moving the block leftwards, can you try just gettin grid of the | indent 10 ? It's nice to keep the indentation for the curly brackets so it's easier to reason about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the indentation is needed because toYAML isn't smart enough to detect and prefix current indentation. So once you hit a newline, the subsequent lines lose the indentation, which generates invalid YAML or invalid k8s manifests:

          - name: CLIENT_SECRET
            valueFrom:
              secretKeyRef:
                name: retool
                key: google-client-secret
          - name: asd
  value: asd
- name: zxc
  valueFrom:
    fieldRef:
      fieldKey: asd

For reference, this is already an established pattern elsewhere in this file: https://github.com/tryretool/retool-helm/blob/main/templates/deployment_backend.yaml#L187

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok! makes sense

@prashantmital prashantmital requested a review from anna-yn June 15, 2022 18:06
Copy link
Contributor

@anna-yn anna-yn left a comment

Choose a reason for hiding this comment

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

great catch!

{{- end }}
{{- with .Values.environmentVariables }}
{{ toYaml . | indent 10 }}
{{ toYaml . | indent 10 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

oh great catch! I have one request: instead of moving the block leftwards, can you try just gettin grid of the | indent 10 ? It's nice to keep the indentation for the curly brackets so it's easier to reason about

{{- end }}
{{- with .Values.environmentVariables }}
{{ toYaml . | indent 10 }}
{{ toYaml . | indent 10 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok! makes sense

@anna-yn anna-yn merged commit 8805479 into tryretool:main Jun 15, 2022
@goodspark goodspark deleted the fix-env-var-values branch June 15, 2022 21:34
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