-
-
Notifications
You must be signed in to change notification settings - Fork 980
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
Removed NULL assignement for username parameter #1428
Conversation
$parameters['database'] = null; | ||
$parameters['username'] = null; |
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.
I recall these changes causing issues in the past, but I can only find #807.
@@ -51,17 +51,17 @@ public function testParametersForSentinelConnectionShouldUsePasswordForAuthentic | |||
/** | |||
* @group disconnected | |||
*/ | |||
public function testParametersForSentinelConnectionShouldIgnoreDatabaseAndPassword(): void | |||
public function testParametersForSentinelConnectionShouldIgnoreDatabase(): void |
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.
I think we should merge this into main
instead, unless we can keep the existing unit tests unchanged.
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.
Well, functionality was changed and unit test should be changed according to this. Or you mean that the whole PR should be merged into main?
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.
What I mean is that people might rely on the removed/changed functionality and if we want to change behavior, we have to either merge into main
because it's breaking.
I'm usually being overly precautious and assume people have weird Sentinel setups with varying usersnames for each node, or some other oddities.
If you like we can merge this into 2.x
, but we might crash someone's setup and need to revert.
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.
You're right, it's a breaking change. Here's new PR against main #1429
Since, Redis 6.2 sentinels supports ACL authentication and AUTH command with both username and password. So for now we don't need to override
username
on Sentinel connectionsCloses #1427