Skip to content

Conversation

@BjoernHelmbold
Copy link
Contributor

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

  1. Setup up Sentinel with auth
  2. Try to connect to Sentinel

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
@AltamashShaikh AltamashShaikh requested a review from a team August 22, 2025 06:52
Copy link
Contributor

@snake14 snake14 left a 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.

Copy link
Contributor

@snake14 snake14 left a 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.

@snake14
Copy link
Contributor

snake14 commented Aug 26, 2025

@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
snake14 previously approved these changes Aug 26, 2025
Copy link
Contributor

@snake14 snake14 left a 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.

@snake14
Copy link
Contributor

snake14 commented Aug 26, 2025

@AltamashShaikh Can you please look over the latest changes. I think everything is good, but I'd like a second opinion.

Copy link
Contributor

@AltamashShaikh AltamashShaikh 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 👍
I can see the new setting appears only for redis section

@snake14 snake14 merged commit 35268fd into matomo-org:5.x-dev Aug 26, 2025
7 checks passed
@snake14
Copy link
Contributor

snake14 commented Aug 26, 2025

Thank you again for taking the time to create this PR @BjoernHelmbold . Your contribution is appreciated.

@BjoernHelmbold
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants