Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Adding Redis db index and username properties #3847

Merged
merged 1 commit into from
Nov 25, 2023

Conversation

Anupchat
Copy link
Contributor

Pull Request type

Feature

Changes in this PR

This adds properties for database index and username to the redis properties for standalone and sentinel.

Issue/Discussion

#3713

@@ -63,16 +63,33 @@ protected JedisCommands createJedisCommands(
}
// We use the password of the first sentinel host as password and sentinelPassword
String password = getPassword(hostSupplier.getHosts());
if (password != null) {
if (properties.getUsername() != null && password != null) {

Choose a reason for hiding this comment

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

Shall we add null + empty check both? I was also thinking if we should also throw error if password is null/empty, basically make password a mandatory field regardless and rather do that check right after line 65. I can't think of a reason why someone would run Redis without password (and if we must support it, one can look at anonymous flag)... Lets see what community says about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The password is optional for various persistence layers that conductor supports, and is the reason why it is obtained from the Host properties. Redis is one of the persistence options and even with Redis it doesn't mandate the use of password. Null check is what existed earlier we can add empty check in addition if needed.

@v1r3n v1r3n merged commit fc3657a into Netflix:main Nov 25, 2023
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants