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
Get coverage back to 100% #1726
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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
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.
We probably shouldn't remove this, what happens if a user has an unverified HTTPS connection, is this class used?
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.
No, it's not used, unverified connections go through VerifiedHTTPSConnection.
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.
Oh, huh. I wonder what the impact of nuking the class and rebinding the name would do to downstream. 🤷♂️
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.
I don't think anybody is using this class: connect() referenced variables that don't exist, so could not work. An additional wrinkle here is the bottom of the file:
So one option is to merge VerifiedHTTPSConnection into HTTPSConnection, and add
UnverifiedHTTPSConnection = HTTPSConnection
at the end of the file.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.
@sethmlarson I opened #1730 to unblock this pull request and talk about our options here, including the upstream impact.
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.
@zogzog Sorry for causing an issue. I've edited your above comment as I read it as sarcastic which isn't constructive. Hope you agree with my edit.
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.
Aurélien, je comprends ta colère, je sais ce que c'est de tomber sur un commit quelque part sur GitHub qui casse ton code. Je te présente d'ailleurs mes excuses. Maintenant que ça c'est dit, on apprécierait des interventions plus courtoises !
We would have appreciated a heads-up at this point! This would have avoided the whole issue.
I'd be happy to accept a pull request that restores a working and covered connect().
Can you use our fingerprinting feature if the certificate does not change too often?
Is silencing the warning an option?
By the way, it looks like the TLS certificate of https://pythonian.fr/ is currently expired.
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.
I mean, why don't you just silence the warning? Isn't this working for you?
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.
Also, for what it's worth, here are the thoughts of Seth on verifying HTTPS: https://sethmlarson.dev/blog/2019-11-26/designing-for-real-world-https. I happen to fully agree.
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.
There's also this method (using certificate pinning as mentioned above) that doesn't require you to disable security warnings. :)
Just remember this PoolManager will only work for the one website. Might be worth even using a ConnectionPool instead.