-
Notifications
You must be signed in to change notification settings - Fork 37
Add support for sentinel password #300
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
Add support for sentinel password #300
Conversation
Currently if you have authentication enabled for sentinel the application cannot get master information
Currently if you have authentication enabled for sentinel the application cannot get master information
snake14
left a comment
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.
Thank you for taking the time to create this PR. I requested one small change.
snake14
left a comment
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.
Apologies. I just thought of a couple more change requests.
Co-authored-by: Jacob R <snake14@users.noreply.github.com>
Co-authored-by: Jacob R <snake14@users.noreply.github.com>
|
@BjoernHelmbold Thank you for making the requested changes. Apologies for taking over your PR. I got looking into how to fix the broken builds and realised that always using the password could break functionality for any customers using a password for their Redis master, but not their sentinels. I added a new setting to protect against that. |
snake14
left a comment
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.
Aside from the UI test failures, I think this is good to go. The failures are because I added a new setting. CI isn't saving the screenshots, so I'll see if I can generate some locally.
|
@AltamashShaikh Can you please look over the latest changes. I think everything is good, but I'd like a second opinion. |
AltamashShaikh
left a comment
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.
Looks good to me 👍
I can see the new setting appears only for redis section
|
Thank you again for taking the time to create this PR @BjoernHelmbold . Your contribution is appreciated. |
|
Thank you very much. The solution goes in the direction of one of my open issues which would support to set different credentials for redis and sentinel any ways. |
Currently if you have authentication enabled for sentinel the application cannot get master information
Description
If you want to use Sentinel with authentication the plugin currently cannot connect to the Sentinel.
With this change it is possible to support the same password as the Redis itself uses.
Steps to Replicate the Issue