-
-
Notifications
You must be signed in to change notification settings - Fork 927
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
Add support for setting redis username #1351
Merged
matusvalo
merged 2 commits into
celery:master
from
open-craft:gabor/add-redis-username-support
Jul 8, 2021
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also update the related docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@auvipy Actually, this is a good question. I'd say yes if the documentation explicitly says that the username part of the connection URL won't be parsed (before this change), otherwise, I'd say that this should be "mainstream" that a connection URL can contain a username.
If you would like me, I'm happy to find the relevant section in the documentation and explicitly state that, from now on, usernames will be parsed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thedrow @auvipy I'm happy to extend any documentation, if you could advise a bit. The documentation has no mention at all about usernames in terms of the Redis transport. In fact, the documentation is defined as
I do believe -- but correct me if I'm wrong -- the reason behind this is that the host parameter (
REDIS_ADDRESS
) may contain username and password as per RFC 2617. If you would like me, I can describe that in the inline documentation, though that would be a bit verbose.If you would like me to extend another documentation that I did not find (yet), please help me with the link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is not 100% complete. I am not 100% whether is good idea to refer to some standard because URL parsing is done by
urllib.parse.urlparse
but some magic is involved during the parsing - seekombu/utils/url.py:url_to_parts()
. Let it keep simple. Please extend the documentation as follows:Maybe we can also add some examples to the documentation.
Otherwise, the PR seems to be fine for me.