Skip to content

Conversation

@marcleblanc2
Copy link
Contributor

Minor fixes to the ordering of attributes in the generated template, to make them more consistent, thus easier to read

Checklist

Test plan

Tested on branch marc-test-helm-fixes on my EKS test instance, works great, no errors

@marcleblanc2 marcleblanc2 requested a review from a team November 25, 2025 09:00
{{- if .Values.pgsql.extraVolumeMounts }}
{{- toYaml .Values.pgsql.extraVolumeMounts | nindent 8 }}
{{- end }}
{{- if .Values.pgsql.extraContainers }}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt we keep this next to the other directive for if extraVolumeMounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest no, based on the tree structure, if extraVolumeMounts only applies to the pgsql container, and we still have the pgsql-exporter container after it, in the list of containers, so any extra containers would come later in the list of containers, after pgsql-exporter

spec:
  template:
    spec:
      initContainers:
      - name: correct-data-dir-permissions
      containers:
      - name: pgsql
        volumeMounts:
        - mountPath: /data
          name: disk
        - ...
        {{- if .Values.pgsql.extraVolumeMounts }}
        {{- toYaml .Values.pgsql.extraVolumeMounts | nindent 8 }}
        {{- end }}
      - name: pgsql-exporter
      {{- if .Values.pgsql.extraContainers }}
        {{- toYaml .Values.pgsql.extraContainers | nindent 6 }}
      {{- end }}
      volumes:
      - name: disk
        persistentVolumeClaim:
          claimName: pgsql
      - ...
      {{- if .Values.pgsql.extraVolumes }}
      {{- toYaml .Values.pgsql.extraVolumes | nindent 6 }}
      {{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I should make this change consistently wherever we have extraContainers in pods with multiple containers, ensure the extraContainers are last in the list.

Copy link
Contributor

@DaedalusG DaedalusG left a comment

Choose a reason for hiding this comment

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

Looks like this is mostly just labels then names at the top of resource definitions and other sensible standardizations lgtm

@marcleblanc2
Copy link
Contributor Author

marcleblanc2 commented Nov 28, 2025

Hey @DaedalusG, I found that extraContainers was broken in cadvisor, node-exporter, and prometheus; using Prometheus as an example, the function was between two pod-level configs:

      terminationGracePeriodSeconds: 120
      {{- if .Values.prometheus.extraContainers }}
        {{- toYaml .Values.prometheus.extraContainers | nindent 6 }}
      {{- end }}
      securityContext:

which resulted in this error:

Error: YAML parse error on sourcegraph/templates/prometheus/prometheus.Deployment.yaml: error converting YAML to JSON: yaml: line 69: did not find expected key

when I tested it with my Helm override file:

prometheus:
  extraContainers:
  - name: test-extra-container
    image: alpine

So I've fixed them, and tested with helm template

Also made some minor formatting fixes.

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.

3 participants