Fix 'url' parameter handling in KartonBackend.make_redis#325
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#292 introduced bug where
urlargument in[redis]configuration section doesn't work withrediss://andunix://URI schemes.In #292, I've parsed the URL using
redis.connection.parse_urlto be able to strip credentials when Redis appears to not require authentication. That was bad decision becauseparse_urlis meant for passing parameters to ConnectionPool class and not the Redis class. So e.g. when we useredissscheme, we getconnection_classparameter set toSSLConnectionwhich is not accepted by the Redis client constructor.So in this PR, I'm using
Redis.from_urlwhenurlis passed in configuration. I'm also not trying to strip credentials from the URL - if someone relies on our the fallback for smooth transition, they must put the credentials inusernameandpasswordconfiguration fields.I have also noticed that we don't allow for
passwordwithoutusernamewhich is incorrect. Redis allows to set authentication both viarequirepassoption globally (which is not associated with any username) and ACLs (see also https://redis.io/docs/latest/commands/auth/). ACL system was introduced in Redis 6.0, so older Redis may reject theAUTH <username> <password>form. I have removed that requirement in this PR.