-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[charts/redis-ha] Allows changes on command/args/env redis container #238
Conversation
Signed-off-by: Benoit Sobrie <[email protected]>
Signed-off-by: Benoit Sobrie <[email protected]>
@Benoitsob I would love a second opinion on this one, there's two ways to go about this:
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing some defaults that you've added to the core values.
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):
If ok, I'll make the change & correction. |
@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. |
Signed-off-by: Benoit Sobrie <[email protected]>
Signed-off-by: Benoit Sobrie <[email protected]>
Signed-off-by: Benoit Sobrie <[email protected]>
Hello @DandyDeveloper , I made changes according the discussion. Thanks for the review. |
@DandyDeveloper I think this is the missing piece for using redis stack, could you let us know what is your plan on this? Thanks! |
Signed-off-by: Aaron Layfield <[email protected]>
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:
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.]
[stable/mychartname]
)