Add possibility to use different passwords for Sentinel and Redis host (#1698) - #3140
Add possibility to use different passwords for Sentinel and Redis host (#1698)#3140doosterkamp wants to merge 11 commits into
Conversation
mgravell
left a comment
mgravell
left a comment
There was a problem hiding this comment.
Concept is great, +1; a few nits in the ConfigurationOptions piece just because that type is super weird (not your fault), and I'm not a huge fan of the clone usage - I welcome your thoughts there.
Thanks! |
…nOptions - fixed typos, so fields, not properties, are used on the correct places
|
@mgravell is there a nice/correct way to clear the CodeQL security failure or do I need to add an inline suppression message? |
|
I have dismissed the CodeQL alert; the alert is "right" in that this should be a considered decision, but this is an entirely consistent and appropriateusage. |
| @@ -0,0 +1,5 @@ | |||
| #nullable enable | |||
There was a problem hiding this comment.
this should all just be in PublicAPI.Unshipped.txt - it isn't feature-specific
| var config = Multiplexer.RawConfig; | ||
| string? user = config.User; | ||
| string password = config.Password ?? ""; | ||
| var isSentinelConnection = Multiplexer.ServerSelectionStrategy.ServerType == ServerType.Sentinel; |
There was a problem hiding this comment.
should this not be checking something like Multiplexer.IsSentinel, i.e. that bool we just added? Alternatively: if the code shown is valid: we possibly don't need the explicit bool, and a property like
internal bool IsSential => ServerSelectionStrategy.ServerType is ServerType.Sentinel;
is fine, without the explicit bool ? In either case: we should be consistent in the decision process.
mgravell
left a comment
mgravell
left a comment
There was a problem hiding this comment.
super close now; sorry to be pedantic on some of these - I'm not trying to frustrate you
|
(note Windows CI may stall due to a packaging bug in 8.10 - server folks are looking into it; I've added a workaround in |
Does not feel like it :P
Don't worry about that! Better safe than sorry ... |
Co-authored-by: Marc Gravell <marc.gravell@gmail.com>
Co-authored-by: Marc Gravell <marc.gravell@gmail.com>
2 participants
Adding the possibility to use different passwords for Sentinel and Redis host.
Fixing the long standing issue #1698
Checklist