Skip to content
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

[stable/redis-ha] ✨ feat: multiple values for redis configs #243

Merged

Conversation

mhkarimi1383
Copy link
Contributor

@mhkarimi1383 mhkarimi1383 commented Jan 30, 2023

What this PR does / why we need it:

When we need to have multiple values for some configs (e.g. loadmodule), yaml is not okay with multiple values for the same key.

Checklist

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

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

Signed-off-by: Muhammed Hussein Karimi <[email protected]>
@mhkarimi1383 mhkarimi1383 force-pushed the feat-multiple-modules branch from 70a274d to d52c06a Compare January 30, 2023 11:54
Signed-off-by: Muhammed Hussein Karimi <[email protected]>
@mhkarimi1383 mhkarimi1383 force-pushed the feat-multiple-modules branch from 0a0493e to 8facbaf Compare January 30, 2023 15:52
@mhkarimi1383 mhkarimi1383 changed the title [stable/redis-ha] ✨ feat: setting multiple modules [stable/redis-ha] ✨ feat: multiple values for redis configs Jan 30, 2023
Copy link
Owner

@DandyDeveloper DandyDeveloper left a comment

Choose a reason for hiding this comment

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

Can we add some examples to the CI.yaml so it picks up this test case?

@mhkarimi1383
Copy link
Contributor Author

I didn't find that CI.yaml file
I should add a new values file into the ci directory in the chart?

@DandyDeveloper
Copy link
Owner

@mhkarimi1383

https://github.com/dandydeveloper/charts/blob/f39ee0e89964e90e781c682d5333f65b75083eaf/.ci/ct-config.yaml#L3

Apparently we just use base values right now, but we could include a specific ci.yaml that can handle all the test cases. I thought we already had this! I was wrong.

But under the ci/ folder in the chart, I'd recommend adding something here and then updating the file I've linked. If you have any Qs or think it's not worth the effort, let me know!

Signed-off-by: Muhammed Hussein Karimi <[email protected]>
@mhkarimi1383
Copy link
Contributor Author

@DandyDeveloper I added a values file for testing, etc.

As my reading that should work without any changes in https://github.com/dandydeveloper/charts/blob/f39ee0e89964e90e781c682d5333f65b75083eaf/.ci/ct-config.yaml

@DandyDeveloper DandyDeveloper merged commit 308521e into DandyDeveloper:master Apr 7, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants