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

Jump to bottom

Add rediss (Redis over SSL) protocol to RedisDsn #1911

Merged
merged 21 commits into from Dec 30, 2020
Merged

Add rediss (Redis over SSL) protocol to RedisDsn #1911

merged 21 commits into from Dec 30, 2020

Conversation

mykolasolodukha
Copy link
Contributor

@mykolasolodukha mykolasolodukha commented Sep 7, 2020 •
edited

Change Summary

Added rediss to RedisDsn.allowed_schemes.

Related issue number

This PR closes #1877

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/1911-TrDex.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Sep 7, 2020 •
edited

Codecov Report

Merging #1911 (2dbbe5b) into master (31bc243) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1911   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         4093      4133   +40     
  Branches       823       832    +9     
=========================================
+ Hits          4093      4133   +40
Impacted Files Coverage Δ
pydantic/networks.py 100.00% <100.00%> (ø)
pydantic/json.py 100.00% <0.00%> (ø)
pydantic/types.py 100.00% <0.00%> (ø)
pydantic/fields.py 100.00% <0.00%> (ø)
pydantic/schema.py 100.00% <0.00%> (ø)
pydantic/validators.py 100.00% <0.00%> (ø)
pydantic/dataclasses.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31bc243...2dbbe5b. Read the comment docs.

@mykolasolodukha mykolasolodukha marked this pull request as ready for review September 7, 2020 18:27
PrettyWood
PrettyWood reviewed Sep 9, 2020
View reviewed changes
Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Small typo in changelog. Otherwise LGTM !

changes/1911-TrDex.md Outdated Show resolved Hide resolved
@PrettyWood
Copy link
Member

PrettyWood commented Sep 9, 2020 •
edited

@TrDex I reckon you should link this issue #1877 as your PR adds rediss.
You could also go a bit further by changing here ?P<user>[^\s:/]+ into ?P<user>[^\s:/]*.
This way you could add both examples 'rediss://redis' and 'redis://:123@redis' in your tests and fully solve the issue. WDYT ?

@mykolasolodukha @PrettyWood
Fix typo in changes/1911-TrDex.md
eec3cef 
Co-authored-by: PrettyWood <em.jolibois@gmail.com>
@mykolasolodukha
Copy link
Contributor Author

@TrDex I reckon you should link this issue #1877 as your PR adds rediss.

Thanks a lot @PrettyWood ! I updated the PR comment (hope it works that way, that's one of my first PRs, lol)

You could also go a bit further by changing here ?P<user>[^\s:/]+ into ?P<user>[^\s:/]*.
This way you could add both examples 'rediss://redis' and 'redis://:123@redis' in your tests and fully solve the issue. WDYT ?

Okay, I'll update that too, and make corresponding tests as well

dependabot bot and others added 2 commits September 10, 2020 15:20
@dependabot @PrettyWood @mykolasolodukha
Bump black from 19.10b0 to 20.8b1 (#1909)
56cf0ba 
* Bump black from 19.10b0 to 20.8b1

Bumps [black](https://github.com/psf/black) from 19.10b0 to 20.8b1.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/master/CHANGES.md)
- [Commits](https://github.com/psf/black/commits)

Signed-off-by: dependabot[bot] <support@github.com>

* fix: run `make format`

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: PrettyWood <em.jolibois@gmail.com>
@mykolasolodukha
Allow URLs without user part
035b181 
Refer to #1877
samuelcolvin
samuelcolvin reviewed Oct 9, 2020
View reviewed changes
tests/requirements.txt Outdated Show resolved Hide resolved
pydantic/utils.py Outdated Show resolved Hide resolved
pydantic/networks.py
@@ -54,7 +54,7 @@ def url_regex() -> Pattern[str]:
if _url_regex_cache is None:
_url_regex_cache = re.compile(
r'(?:(?P<scheme>[a-z][a-z0-9+\-.]+)://)?' # scheme https://tools.ietf.org/html/rfc3986#appendix-A
r'(?:(?P<user>[^\s:/]+)(?::(?P<password>[^\s/]*))?@)?' # user info
r'(?:(?P<user>[^\s:/]*)(?::(?P<password>[^\s/]*))?@)?' # user info
Copy link
Member

Choose a reason for hiding this comment

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

this is quite a significant change - allow password without user.

I think it's a good idea but it needs:

  • to be noted in documentation
  • to have it's own item in change log
  • to have tests covering this case.

Copy link
Member

Choose a reason for hiding this comment

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

these items still need resolving.

Copy link
Member

Choose a reason for hiding this comment

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

@TrDex this can't move forward until you act upon this comment.

@PrettyWood PrettyWood mentioned this pull request Nov 5, 2020
Closed
@nuno-andre
Copy link
Contributor

Additionally:

  • The path, if any, should only be numeric (without leading zeros) and with a single segment.
  • All fields after the scheme are optional. hostname defaults to localhost, port to 6379 and path to 0.

https://www.iana.org/assignments/uri-schemes/prov/redis

samuelcolvin
samuelcolvin reviewed Nov 29, 2020
View reviewed changes
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Also please fix linting.

pydantic/networks.py
@@ -54,7 +54,7 @@ def url_regex() -> Pattern[str]:
if _url_regex_cache is None:
_url_regex_cache = re.compile(
r'(?:(?P<scheme>[a-z][a-z0-9+\-.]+)://)?' # scheme https://tools.ietf.org/html/rfc3986#appendix-A
r'(?:(?P<user>[^\s:/]+)(?::(?P<password>[^\s/]*))?@)?' # user info
r'(?:(?P<user>[^\s:/]*)(?::(?P<password>[^\s/]*))?@)?' # user info
Copy link
Member

Choose a reason for hiding this comment

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

@TrDex this can't move forward until you act upon this comment.

pydantic/networks.py Outdated Show resolved Hide resolved
@mykolasolodukha
Copy link
Contributor Author

@samuelcolvin I worked on your comments about allowing password without user.

  • It's noted in the documentation.
  • It has its own item in the changelog.
  • It has tests covering this case.

PrettyWood
PrettyWood reviewed Dec 18, 2020
View reviewed changes
tests/test_networks.py Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin merged commit aacf592 into pydantic:master Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daviskirk daviskirk daviskirk left review comments

@samuelcolvin samuelcolvin samuelcolvin left review comments

@nuno-andre nuno-andre nuno-andre left review comments

@PrettyWood PrettyWood PrettyWood left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RedisDsn canot enable redis url with password and ssl schema
5 participants
@mykolasolodukha @PrettyWood @nuno-andre @daviskirk @samuelcolvin