charts icon indicating copy to clipboard operation
charts copied to clipboard

[charts/redis-ha] Allows changes on command/args/env redis container

Open Benoitsob opened this issue 3 years ago • 5 comments

What this PR does / why we need it:

When containers are restarted, initContainers are not re-executed as the pod is not restarted. To ensure that a script is executed before starting redis, the script should be included in the redis container. This PR allows this by allowing:

  • Overriding of Redis container command and args.
  • Adding extra environment variables from ConfigMap and Secrets.
redis:
  command:
  - bash
  - "/scripts/init.sh" 
  args:
  - redis-server
  - /data/conf/redis.conf
  extraVolumeMounts:
  - mountPath: /scripts
    name: init-script
  envFrom:
  - secretRef:
      name: add-env-secret
....
extraVolumes:
- name: init-script
  configMap:
    name: redis-init-script
init.sh
#!/bin/bash
echo "Do stuff before redis startup with custom EnvVar"
exec "$@"

Which issue this PR fixes

Doesn't concern an issue.

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • [x] DCO signed
  • [x] Chart Version bumped
  • [x] Variables are documented in the README.md
  • [x] Title of the PR starts with chart name (e.g. [stable/mychartname])

Benoitsob avatar Oct 26 '22 12:10 Benoitsob

@Benoitsob I would love a second opinion on this one, there's two ways to go about this:

  • We keep the default and condition with an if/else to map the customCommand. I say this, because for some, it would be VERY easy to break what they already have in a not so obvious way,

  • The way you've done things.

I like both, one of the frustrations of being an independent maintainer is figuring which one I like better.

If you had to choose between these both. Which would you prefer?

DandyDeveloper avatar Mar 02 '23 01:03 DandyDeveloper

Hello @DandyDeveloper ,

The second one is less code and more straightforward. But it's fair to make clear that the command is/may be overridden by the user. I would go then with the proposed if/else clause.

It would look like this (also applied for args):

#values.yaml
  customCommand: []
  # - bash
  customArgs: []
  # - "custom-startup.sh"

#redis-ha-statefulset.yaml
        command:
        {{- if .Values.redis.customCommand }}
{{ toYaml .Values.redis.customCommand | indent 10 }}
        {{- else }}
          - redis-server
        {{- end }}
        args:
        {{- if .Values.redis.customArgs }}
{{ toYaml .Values.redis.customArgs | indent 10 }}
        {{- else }}
          - /data/conf/redis.conf
        {{- end }}

If ok, I'll make the change & correction.

Benoitsob avatar Mar 02 '23 11:03 Benoitsob

@Benoitsob I think this is the happy middleground to suit all user cases (I had nothing wrong with the original, but I think this is more thorough and won't hurt existing users).

Throw me an @ when you finish and I'll get this merged asap.

DandyDeveloper avatar Mar 08 '23 00:03 DandyDeveloper

Hello @DandyDeveloper ,

I made changes according the discussion. Thanks for the review.

Benoitsob avatar Mar 16 '23 16:03 Benoitsob

@DandyDeveloper I think this is the missing piece for using redis stack, could you let us know what is your plan on this?

Thanks!

maxisam avatar Jul 12 '23 17:07 maxisam