SchwarzIT / node-red-chart

Node-red Helm Chart
Apache License 2.0
38 stars 25 forks source link

npm install from values.yaml #208

Closed beasteers closed 1 year ago

beasteers commented 1 year ago

Thank you for making node-red ⚙ better

Fixes:

Let me know what you think and if you think there's a better way to do it.

Also verify you have:

dirien commented 1 year ago

Hi @beasteers,

The issue I have with this approach is that you are overloading the specific use case of this "hardcoded" sidecar. This sidecar is only resposible for loading flows from configmaps/secrets via the k8s-sidecar

See https://github.com/SchwarzIT/node-red-chart/tree/main/charts/node-red#sidecar-%EF%B8%8F

And assumes, that the image in the sidecar has nodejs installed. Or you need to change the image of the sidcar too.

I would rather use the existing function of defining a new extraSidcars

https://github.com/SchwarzIT/node-red-chart/blob/93feb40c903f5d0733ce30ca09a00bcf2bd9d342/charts/node-red/templates/deployment.yaml#L48-L50

WDYT?

beasteers commented 1 year ago

Oh oops you're right sorry I got lost in the weeds trying to fix the git history and accidentally re-added those lines back to the wrong container. I meant to add it to the main container which I can fix.

Do you think this would be best suited for an initContainer instead? It should happen before settings.js is loaded so that there isn't a race condition.

dirien commented 1 year ago

According to the K8s docs https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#define-a-command-and-arguments-when-you-create-a-pod, when you set the command field it will overwrite the entrypoint of the container image.

Did you tested this? Is node-red still starting?

beasteers commented 1 year ago

Yes I made sure to include the original entrypoint (/entrypoint.sh) in the command so node-red does start up.

"{{- if .Values.installPackages }}npm install {{ join " " .Values.installPackages }};{{ end }} ./entrypoint.sh"

My concern with this is in case someone is using a custom image with an entrypoint that's different from the default entrypoint script. I just tried to implement it using an initContainer, but the packages weren't showing up in the nodered instance. I'd have to dig deeper, but I suspect that the packages are being installed inside the initContainer's file system which is being discarded.

Here's the initContainer config I tried (not sure how much of this is needed)

      {{- if or .Values.initContainers }}
      initContainers:
        {{- if .Values.initContainers }}
          {{- toYaml .Values.initContainers | nindent 8 }}
        {{- end }}
        {{- if .Values.installPackages  }}
        - name: {{ .Chart.Name }}-npm-install
          {{- if .Values.securityContext }}
          securityContext:
            {{- toYaml .Values.securityContext | nindent 12 }}
          {{- end }}
          image: "{{ .Values.image.registry }}/{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
          imagePullPolicy: {{ .Values.image.pullPolicy }}
          command:
           - sh
           - -c
           - npm install {{ join " " .Values.installPackages }}
          {{- if .Values.env }}
          env:
          {{- toYaml .Values.env | nindent 10 }}
          {{- end }}
          volumeMounts:
            {{- if and .Values.npmrc.enabled }}
            - name: npmrc-volume
              mountPath: /usr/src/node-red/.npmrc
              subPath: npmrc
            {{- end }}
            {{- with .Values.extraVolumeMounts }}
              {{- toYaml . | nindent 12 }}
            {{- end }}
            - name: data
              mountPath: /data
              {{- if .Values.persistence.subPath }}
              subPath: {{ .Values.persistence.subPath }}
              {{- end }}
          {{- if .Values.resources }}
          resources:
            {{- toYaml .Values.resources | nindent 12 }}
          {{- end }}
        {{- end }}
      {{- end }}

fyi the indentation was still broken in the main container (the fix you implemented only fixed the sidecar). I can take it out and move it to a different PR if you want but it was small and breaking my tests so I just threw it in

dirien commented 1 year ago

fyi the indentation was still broken in the main container (the fix you implemented only fixed the sidecar). I can take it out and move it to a different PR if you want but it was small and breaking my tests so I just threw it in

@beasteers that would be awesome

dirien commented 1 year ago

I'd have to dig deeper, but I suspect that the packages are being installed inside the initContainer's file system which is being discarded.

@beasteers maybe check the volumeMount. It should share it.

dirien commented 1 year ago

Hi @beasteers,

with https://github.com/SchwarzIT/node-red-chart/pull/216 I fixed the issue with the indentation on the main container!

I will close this issue for now. Feel free to open a new one for the npm enhancements.