Skip to content
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

Fix sentinel connections #3376

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

garionphx
Copy link

@garionphx garionphx commented Sep 10, 2024

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:

  • 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)?
  • Is there an example added to the examples folder (if applicable)?
  • 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

John Sutherland added 2 commits September 9, 2024 20:13
When passing args to Redis(), using the connection_kwargs args allows us to use authentication and other connection features.
@garry-t
Copy link

garry-t commented Sep 10, 2024

great, thank you!

@vladvildanov vladvildanov added the breakingchange API or Breaking Change label Sep 12, 2024
@vladvildanov
Copy link
Collaborator

@garionphx Thanks for your contribution! Make sure that tests passes

@vladvildanov
Copy link
Collaborator

@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.

@@ -240,7 +240,7 @@ def __init__(
self.sentinel_kwargs = sentinel_kwargs

self.sentinels = [
Redis(hostname, port, **self.sentinel_kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is the only place where self.sentinel_kwargs are used we have to remove it

@rad-pat
Copy link

rad-pat commented Oct 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingchange API or Breaking Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants