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

fix for the redis password not being set in the connection string #325

Merged
merged 4 commits into from
Aug 12, 2022

Conversation

rebrowning
Copy link
Contributor

I'd like some feedback on how we want to handle this.

There are other options with how to configure redis in the underlying bitnami chart (generated password, secretfile). However I don't know how to grab either the generated helm value or the value from the secret file to then inject it into the redis url. Are there some additional coordination parameters we can use in st2 to set the password or an environment variable that st2 will consume for the redis password?

Additionally, you can set sentinel to be protected by the same password as well, however this DOES NOT work due to the tooz driver not populating the password for the sentinel client, only for the redis client that gets created from the sentinel get master request.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Aug 3, 2022
@rebrowning rebrowning changed the title add an initial fix for the redis password not being set in the connection string fix for the redis password not being set in the connection string Aug 3, 2022
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I don't use the subchart, so I can't say which method will be better to use here. From reading, this looks good to me, but I'd prefer someone else who uses the redis subchart chime in before I merge this.

@arms11
Copy link
Contributor

arms11 commented Aug 9, 2022

Hi @rebrowning I am one of the users who uses the sub-chart. As long as you have tested the deployment without password and it works, I think it's a good change. We want to make sure it's backward compatible. Could you please confirm?

@robertorch
Copy link

@arms11 I tested both with and without a password configuration for Redis when doing this. I'm more than happy to provide a couple screenshots after hours of the config/template/deployment success for completeness.

@arms11
Copy link
Contributor

arms11 commented Aug 9, 2022

Nope. I appreciate the response.

@cognifloyd
Copy link
Member

Please also add a changelog entry.

@cognifloyd cognifloyd merged commit 806ef07 into StackStorm:master Aug 12, 2022
@rebrowning rebrowning deleted the fix_redis_password_issue branch December 6, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants