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

Handle missing pyopenssl dependency #243

Merged

Conversation

ryanwilsonperkin
Copy link
Contributor

Alternative to #242
Fixes #241

Handle the case where pyopenssl is not installed and fall back to default of setting PyOpenSSLContext to None.

@altendky
Copy link

Maybe add test coverage with and without?

@ryanwilsonperkin
Copy link
Contributor Author

@altendky not really sure how I'd go about doing that. Any recommendations?

@greatestape
Copy link

Could you test the fix by adding an additional tox build that omits pyopenssl?

@greatestape
Copy link

@ryanwilsonperkin I took a crack at adding test coverage of this. I tried rolling back your change to confirm that it would cause the test to fail, and it seems to cover it.

Let me know what you think.

ryanwilsonperkin#1

@ryanwilsonperkin
Copy link
Contributor Author

Thanks @greatestape, added a couple of comments onto that PR. If you're available to respond now that'd be great, otherwise I'll accept as is and then pull your changes into this PR

@sigmavirus24 sigmavirus24 merged commit fa49fad into requests:master Jan 30, 2019
@sigmavirus24
Copy link
Collaborator

Feel free to send the tests along in another PR. I'd like to get this released tonight

@nsoranzo
Copy link

Thanks @ryanwilsonperkin and @sigmavirus24!

@sigmavirus24
Copy link
Collaborator

you got it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants