tryretool / retool-helm

MIT License
45 stars 57 forks source link

[fix] add configurable port names for istio compatibility #154

Closed avimoondra closed 3 months ago

avimoondra commented 5 months ago

Addresses https://github.com/tryretool/retool-helm/issues/144, alternate to https://github.com/tryretool/retool-helm/pull/151

Addresses: https://github.com/tryretool/retool-helm/issues/155, alternate to https://github.com/tryretool/retool-helm/pull/156

helm template -f ~/retool-helm/charts/retool/values.yaml foo ~/retool-helm/charts/retool --set config.encryptionKey="foo" --set image.tag="5.6.11" --set codeExecutor.portName=http-bloop --set workflows.backend.portName=http-bar --set workflows.worker.portName=http-bleep --set workflows.worker.metricsPortName=http-bleep2 | rg "http-bloop|http-bar|http-bleep" -C 5
  ports:
  - protocol: TCP
    port: 80
    targetPort: 3004

    name: http-bloop
---
# Source: retool/templates/deployment_workflows.yaml
---
apiVersion: v1
kind: Service
--
  ports:
  - protocol: TCP
    port: 80
    targetPort: 3000

    name: http-bar
---
# Source: retool/templates/deployment_workflows_worker.yaml
---
apiVersion: v1
kind: Service
--
  ports:
  - protocol: TCP
    port: 3005
    targetPort: 3005

    name: http-bleep

  - protocol: TCP
    port: 9090
    targetPort: http-bleep2
    name: http-bleep2
---
# Source: retool/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
--
          - name: NODE_OPTIONS
            value: --max_old_space_size=1024
        ports:
        - containerPort: 3004

          name: http-bloop

          protocol: TCP
        livenessProbe:
          httpGet:
            path: /api/checkHealth
--
                name: foo-retool
                key: google-client-secret
        ports:
        - containerPort: 3000

          name: http-bar

          protocol: TCP
        livenessProbe:
          httpGet:
            path: /api/checkHealth
--
                name: foo-retool
                key: google-client-secret
        ports:
        - containerPort: 3005

          name: http-bleep

          protocol: TCP
        - containerPort: 9090
          name: http-bleep2
          protocol: TCP
        livenessProbe:
          httpGet:
            path: /api/checkHealth
            port: 3005

(also fixes https://github.com/tryretool/retool-helm/issues/155)

alexlo03 commented 5 months ago

LGTM

  1. you need to update values.yaml OR document these possible values
  2. IMO there should be a default port name all the time, I believe that is a non-breaking change
alexlo03 commented 5 months ago

LGTM, the defaulting comments are not necessary but I think you could collapse the conditionals down

avimoondra commented 5 months ago

LGTM, the defaulting comments are not necessary but I think you could collapse the conditionals down

The if/else is giving that default - but I ran into an issue using default with "template"... e.g. this wasn't working:

{{ .Values.codeExecutor.portName | default (template "retool.name" .) }}

neither this

{{ default  .Values.codeExecutor.portName (template "retool.name" .) }}

lmk if you have a different suggestion @alexlo03 ! thanks for review.

ryanartecona commented 5 months ago

I ran into an issue using default with "template"...

oh I didn't notice this comment before, but that's because template doesn't return the value, it just prints it out to the output. you can just change template to include, which returns instead of printing, and then it will work as expected with the default pipeline. usually include is preferred over template in helm for exactly this reason.

avimoondra commented 5 months ago

I ran into an issue using default with "template"...

oh I didn't notice this comment before, but that's because template doesn't return the value, it just prints it out to the output. you can just change template to include, which returns instead of printing, and then it will work as expected with the default pipeline. usually include is preferred over template in helm for exactly this reason.

Ah thanks @ryanartecona - will address all of it w/your feedback today! 🙏

alexlo03 commented 4 months ago

gentle ping. would like to see this in before we upgrade charts. thanks 🙏

avimoondra commented 3 months ago

Closing in favor of https://github.com/tryretool/retool-helm/pull/170