Skip to content

Add Redis AUTH user/password support with updated docs and tests #178

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

Open
wants to merge 4 commits into
base: 3.x
Choose a base branch
from

Conversation

mauricioguim
Copy link

This change implements full ACL-style authentication for Redis 6+ in the Factory class, allowing both username and password to be read from the URI or query parameters and sent as AUTH . The README has been updated to document both legacy (password-only) and new ACL modes in the constructor and callAsync() sections.

In addition, existing tests were revised to expect the three-argument RESP payload when a username is present, and new unit tests were added to verify correct behavior and response handling for URIs supplying both username and password. All changes maintain backward compatibility with single-password authentication.

@PaulRotmann
Copy link
Collaborator

Thanks for this contribution! The functionality you're implementing is definitely needed.

However, there are a few issues that need to be addressed. The code formatting and indentation is inconsistent throughout the changes, and the 4 commits should be squashed into 1-2 meaningful commits. This also represents a breaking change since we currently document that usernames are ignored, so it needs to be part of a major version release. Please also reference issue #133 in your PR description.

The feature itself makes sense for supporting modern Redis versions (6+), but we'll need to plan this as a breaking change for v3.0. Could you clean up the formatting and commit structure? We can then discuss the best approach for integrating this as a major version feature.

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants