-
Notifications
You must be signed in to change notification settings - Fork 387
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
Added support for SNI #755
Added support for SNI #755
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #755 +/- ##
==========================================
- Coverage 97.01% 96.81% -0.20%
==========================================
Files 27 27
Lines 3549 3553 +4
==========================================
- Hits 3443 3440 -3
- Misses 106 113 +7 ☔ View full report in Codecov by Sentry. |
Formatting issues have been resolved. I'm not sure why one of the pypi tests failed with a segfault when all of the others passed. |
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.
Thank you for this PR. Just have some comments here and there. No worries about the segfault, the pypy testing pipeline just triggers it from time to time, it probably relates to the GA infrastructure, but I really never investigated it since I was not able to replicate it locally.
6c045ea
to
05d5b27
Compare
@StephenSorriaux Thank you for the feedback. I have made several tweaks based upon your comments. Please let me know if you see any additional things that need attention. |
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.
Thanks a lot, that's great.
I will take care of doing a squash of the commits when merging, so that the final commit message follows our guidelines.
I will also wait a little for another maintainer to gives another feedback.
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 didn't review it deeply as I'm not deeply familiar with SNI, but what I saw seems reasonable.
Thanks for plumbing it through and also including tests.
Co-authored-by: Jeff Widman <[email protected]>
2dc4ff8
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.
LGTM. Thanks a lot for your contribution!
@StephenSorriaux I believe you wanted to handle the squash & merge? |
Fixes #754
Why is this needed?
Supports SNI
Proposed Changes
Add server_hostname to the wrap call
Does this PR introduce any breaking change?
No