Issue 249: Don't rely on the bundled certificate file being available. #255
Conversation
…lly specified when creating the connection. Fixes exceptions if the cert.pem file included with the hyper package isn't included in a distribution (such as a generated exe)
…ng Py2Exe) without manually specifying a certificate.
| # if an SSLContext is provided then use it instead of default context | ||
| _ssl_context = ssl_context | ||
| else: | ||
| global _context |
There was a problem hiding this comment.
I...don't think you can do this. global _context, however, is just a statement that makes it clear to Python that I want to be able to assign to the global variable, so it shouldn't be needed.
There was a problem hiding this comment.
Or, more specifically, it should be hoisted to the top of the function block.
|
Thanks for this! I have a few small notes, and then we need a test to deal with the new code branch you've introduced. =) Are you happy to deal with that? |
|
Making the suggested changes now. |
|
It's stylistic. =) As for tests, maybe in |
Hoisted global _context declaration. Added test cases.
Backup & restore the global context to hopefully fix subsequent test failures.
|
|
||
| assert conn.ssl_context.check_hostname | ||
| assert conn.ssl_context.verify_mode == ssl.CERT_REQUIRED | ||
| assert conn.ssl_context.options & ssl.OP_NO_COMPRESSION != 0 |
There was a problem hiding this comment.
In this instance, while I like the extremely thorough testing you've done here, I think we can safely just test _init_context directly. That's the only place where we're concerned.
If you want a slightly more thorough test, you can actually wrap a socket but not bother to connect it. In both of these cases we drastically reduce the amount of system access needed to test the software, which is good.
|
Thanks for the work! I have one tiny style note and one thing about the testing. As always, I appreciate your spending time here, and if you get sick of the code review just say so and I can make whatever changes are still required myself. |
Descope test case to avoid failing on unrelated issues.
|
I'll get there eventually, hopefully it's up to your exacting standards now :)
|
|
Cool, this looks good to me. I'm not going to merge it just this second because I want to be in front of a laptop so I can make some associated docs changes.. But this looks really good, thank you so much for the work! ✨ 🍰 ✨ |
|
Thanks @JasonGowthorpe! ✨ 🍰 ✨ |
No description provided.