EugenMayer / helm-charts

Helm charts for applications no official chart exists for
24 stars 18 forks source link

Rundeck chart: sideCars value not applied in deployment template #50

Closed jgtr-blis closed 8 months ago

jgtr-blis commented 10 months ago

Contrary to the readme documentation (https://github.com/EugenMayer/helm-charts/tree/main/charts/rundeck#other-values) which states that the sideCars value "can be used to run additional containers in the pod" it appears that it isn't actually referenced in the templates so setting it's value doesn't do anything.

I believe this can be fixed by adding the following to the rundeck-backend-deployment.yaml template - I would have raised a PR, however I don't have permission to create a branch/PR on the repo:

             {{- toYaml .Values.resources | nindent 12 }}
+        {{- if .Values.sideCars }}
+          {{- toYaml .Values.sideCars | nindent 8 }}
+        {{- end }}
       {{- if .Values.nodeSelector }}

P.S. Thanks for your work on the Rundeck chart - as much as the documentation around Rundeck isn't great, as you stated, your chart certainly takes away some of the pain.

EugenMayer commented 10 months ago

Thank you for the kind words. Yes the entire situation around Rundeck looks rather dire, to be honest.

To be honest, i never planned to support side cars and rather would continue to do so by removing them from the readme (i removed them from the chart actively).

But feel free to convince me otherwise, what is you use case?

jgtr-blis commented 10 months ago

Happy new year! Thanks for the response - in my case I'm running the rundeck-exporter as a sidecar to export metrics to prometheus and the sidecar mechanism allows me to do this in a fairly clean way whereas alternatives approaches would be somewhat messy to manage (i.e. creating a custom version of the Rundeck docker image with the rundeck-exporter already installed and then passing the config values via the rundeck charts env variables section).

sideCars:
  - name: rundeck-exporter
    image: phsmith/rundeck-exporter:2.6.3
    env:
      - name: RUNDECK_EXPORTER_HOST
        value: 0.0.0.0
      - name: RUNDECK_EXPORTER_PORT
        value: "9100"
      - name: RUNDECK_URL
        value: http://127.0.0.1:4440
      - name: RUNDECK_CPU_STATS
        value: "true"
      - name: RUNDECK_MEMORY_STATS
        value: "true"
      - name: RUNDECK_PROJECTS_EXECUTIONS
        value: "true"
      - name: RUNDECK_EXPORTER_DEBUG
        value: "false"
      - name: RUNDECK_SKIP_SSL
        value: "true"
      - name: RUNDECK_API_VERSION
        value: "45"
    securityContext:
      runAsNonRoot: true
    ports:
      - containerPort: 9100
        name: metrics
        protocol: TCP

Out of curiosity what was the issue you encountered that made you take the decision to actively remove sidecars?

EugenMayer commented 10 months ago

That really makes a lot of sense. I removed it mainly because of having no need for it and not seeing a good reason to let people run "more pods" next to rundeck - they could just make additional charts for whatever they need.

But your case really makes a lot of sense as a sidecar and is usually done as a side car (mainly rather directly supported by the helm chart itself).

I would def. consider readding sidecar support again, are you up for an PR for this case?

jgtr-blis commented 10 months ago

Thanks - I will raise a PR when I have a spare moment.