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

Specify ca file from certifi #893

Closed
wants to merge 2 commits into from

Conversation

michaelhenry
Copy link

close #892

@michaelhenry michaelhenry force-pushed the master branch 2 times, most recently from acf100f to 5ef5acf Compare June 4, 2023 07:48
@auvipy auvipy self-requested a review June 5, 2023 05:45
Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you please update unit tests as well

@michaelhenry
Copy link
Author

@auvipy thanks for the review. Is it okay if execute an actual request? because the urllib.request.urlopen was mocked, but not sure how to validate the root certificate without executing the actual request. Any guidance on this part? thanks

@jpadilla
Copy link
Owner

jpadilla commented Jun 6, 2023

I'm not sure I see the value of including an additional dependency here. I'd rather see #891 get in

@michaelhenry
Copy link
Author

@jpadilla true, that one is more dynamic. I can close this pr and hopefully to merge it soon. Thanks

@michaelhenry
Copy link
Author

Closing this pr with respect to #891

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