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.

Sign up for GitHub

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

Move ssl_match_hostname to urllib3.utils. #2198

Merged
merged 3 commits into from Apr 21, 2021
Merged

Move ssl_match_hostname to urllib3.utils. #2198

merged 3 commits into from Apr 21, 2021

Conversation

hramezani
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Apr 12, 2021 •
edited

Codecov Report

Merging #2198 (bcc635f) into main (51f4c67) will not change coverage.
The diff coverage is 100.00%.

❗ Current head bcc635f differs from pull request most recent head 975f7ae. Consider uploading reports for the commit 975f7ae to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2198   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        25    +1     
  Lines         2240      2284   +44     
=========================================
+ Hits          2240      2284   +44
Impacted Files Coverage Δ
src/urllib3/connection.py 100.00% <ø> (ø)
src/urllib3/connectionpool.py 100.00% <ø> (ø)
src/urllib3/util/ssl_match_hostname.py 100.00% <100.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 51f4c67...975f7ae. Read the comment docs.

sethmlarson
sethmlarson requested changes Apr 16, 2021
View reviewed changes
Copy link
Member

@sethmlarson sethmlarson left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for this, instead of pulling the whole module over, can we instead move everything to a single file urllib3.util.ssl_match_hostname?

src/urllib3/util/__init__.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@hramezani hramezani force-pushed the move_ssl_match_hostname branch 3 times, most recently from 71fb278 to 201e714 Compare April 19, 2021 09:52
@hramezani
Copy link
Member Author

@SethMichaelLarson
We have a mypy validation error
src/urllib3/connection.py:591: error: Call to untyped function "match_hostname" in typed context.
Should I ignore validation for this line?

@pquentin
Copy link
Member

No, please don't ignore: we need to include the types from https://github.com/urllib3/urllib3/pull/2198/files#diff-e60901ca7e95a57497217ad9b1a4228bbe3319f6d264575a4f667ac349b015c7 inline in the new file, since we're now Python 3.6+. Do you need help with that?

@hramezani
Copy link
Member Author

hramezani commented Apr 19, 2021 •
edited

No, please don't ignore: we need to include the types from https://github.com/urllib3/urllib3/pull/2198/files#diff-e60901ca7e95a57497217ad9b1a4228bbe3319f6d264575a4f667ac349b015c7 inline in the new file, since we're now Python 3.6+. Do you need help with that?

We have the same definition in connection.py. Should we keep them as is or import them from src/urllib3/util/ssl_match_hostname.py

@pquentin
Copy link
Member

or import them from src/urllib3/util/ssl_match_hostname.py

That sounds better to me!

@hramezani hramezani force-pushed the move_ssl_match_hostname branch 2 times, most recently from 4d48cff to 123c627 Compare April 19, 2021 13:04
hramezani
hramezani commented Apr 20, 2021
View reviewed changes
noxfile.py Show resolved Hide resolved
@pquentin
Copy link
Member

This is looking quite good, but I'd like to fix the Python 3.10 tests (#2201) before doing a thorough review.

sethmlarson
sethmlarson requested changes Apr 20, 2021
View reviewed changes
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Couple more comments for you

src/urllib3/util/ssl_match_hostname.py Show resolved Hide resolved
src/urllib3/util/ssl_match_hostname.py Outdated


def match_hostname(cert, hostname):
def match_hostname(cert: _PeerCertRetType, hostname: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't cert be _PeerCertRetDictType here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied it from https://github.com/urllib3/urllib3/pull/2198/files#diff-e60901ca7e95a57497217ad9b1a4228bbe3319f6d264575a4f667ac349b015c7 as @pquentin suggested.

@hramezani
Copy link
Member Author

Just rebased with main!

@sethmlarson
Copy link
Member

sethmlarson commented Apr 21, 2021 •
edited by hramezani

@hramezani It looks like we're missing coverage by moving the file to utils, perhaps we can create a few test cases to ensure match_hostname is covered?

@hramezani
Copy link
Member Author

hramezani commented Apr 21, 2021 •
edited

@hramezani It looks like we're missing coverage by moving the file to utils, perhaps we can create a few test cases to ensure match_hostname is covered?

@sethmlarson I added some test to cover them

sethmlarson
sethmlarson approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This looks good to me now, once CI passes I'll merge :)

@sethmlarson sethmlarson merged commit c321319 into urllib3:main Apr 21, 2021
@hramezani hramezani deleted the move_ssl_match_hostname branch April 21, 2021 21:52
@pquentin pquentin mentioned this pull request Oct 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pquentin pquentin pquentin left review comments

@sethmlarson sethmlarson sethmlarson approved these changes

@shazow shazow Awaiting requested review from shazow

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@hramezani @pquentin @sethmlarson