Fix sentinel connections
When passing args to Redis(), using the connection_kwargs args allows us to use authentication and other connection features.
Pull Request check-list
Please make sure to review and check all of these items:
- [x] Do tests and lints pass with this change?
- [ ] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- [ ] Is the new or changed code fully tested?
- [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- [x] Is there an example added to the examples folder (if applicable)?
- [x] Was the change added to CHANGES file?
NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Modified the arguments passed to Redis() when forming Sentinel connections
great, thank you!
@garionphx Thanks for your contribution! Make sure that tests passes
@garionphx Since this changes makes self.sentinel_kwargs argument completely redundant, it should be removed from constructor properties and it will be a breaking change.
This doesn't seem correct. The sentinel_kwargs are the connection_kwargs passed in to make the sentinel connections and the connection_kwargs are those used for making Redis connections. The arguments can be different for each and should be allowed to be.
Hi @garionphx,
Thanks for the contribution! We should keep sentinel_kwargs and connection_kwargs as separate arguments. These serve distinct purposes: we may need different connection settings for Sentinel nodes and for Redis servers (both master and slaves). For example, SSL configuration, authentication credentials, or even different users might vary between the two connection types.
Keeping them as separate dictionaries is currently the only flexible and clean way to support these differences. That said, you can still supply usernames and credentials specifically for Sentinel connections via sentinel_kwargs.
For these reasons, I am going to close this PR.