Skip to content

Conversation

@clrxbl
Copy link
Contributor

@clrxbl clrxbl commented Dec 9, 2025

Description of the change

Ports Redis Helm chart PRs #615, #639 and #701 to the Valkey chart. Also fixes a critical issue where the custom Valkey config is never being mounted so Helm chart config options are not being respected.

Benefits

Sentinel failover is unreliable without these PRs and has impacted our production workloads as a result.

Possible drawbacks

Default behaviour of announcing hostnames can potentially break something, but should be done going forward. Can be reverted by the user with Helm chart setting sentinel.config.announceHostnames=false. Refer to #628 for one user complaining about this behaviour change.

Applicable issues

N/A but fixes Valkey Sentinel failover

Additional information

N/A

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md
  • Title of the pull request follows this pattern [<name_of_the_chart>] Descriptive title

@clrxbl
Copy link
Contributor Author

clrxbl commented Dec 9, 2025

One thing I was uncertain about is the usage of cloudpirates.namespace vs .Release.Namespace. In the Redis chart the cloudpirates variable seems to be used but here it seems to be Release.Namespace. I've kinda blindly find & replaced it with cloudpirates.namespace in this PR, but happy to revert depending on what the choice should be.

@clrxbl clrxbl changed the title [valkey] Port #615, #639, #701 [valkey] Port #615, #639, #701 & fix custom config never being used Dec 9, 2025
@dloewen2 dloewen2 self-requested a review December 10, 2025 10:45
@dloewen2 dloewen2 added enhancement New feature or request valkey Issues regarding the valkey chart labels Dec 10, 2025
Copy link
Member

@dloewen2 dloewen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, only thing missing for me is the value config.mountPath in the ReadMe.md

@clrxbl
Copy link
Contributor Author

clrxbl commented Dec 10, 2025

Looks good to me, only thing missing for me is the value config.mountPath in the ReadMe.md

resolved

@dloewen2 dloewen2 merged commit 58035e4 into CloudPirates-io:main Dec 10, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request valkey Issues regarding the valkey chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants