-
Notifications
You must be signed in to change notification settings - Fork 323
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
Stop providing a hardcoded CA bundle #489
Conversation
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.
Looks good, thanks Max!
ff3a81b
to
3e39f8e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
=======================================
Coverage 64.26% 64.26%
=======================================
Files 31 31
Lines 52609 52606 -3
Branches 3841 3841
=======================================
+ Hits 33807 33808 +1
+ Misses 18641 18638 -3
+ Partials 161 160 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Just a few minor changes
# TODO: backend is no longer returning `BadInputError` | ||
# with pytest.raises(BadInputError,) as cm: | ||
# malformed_token_dbx.files_list_folder('') | ||
# assert 'token is malformed' in cm.value.message |
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.
Can we remove these comments, or is the intention to fix this?
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.
The intention is to fix this in a future PR. This is also not technically introduced in this PR, I've just moved this test out of the class.
The SDK no longer provides a CA bundle to verify SSL connections. This also allows us to remove the runtime dependency on
pkg_resources
and thussetuptools
.The
ca_certs
parameter is still supported, so users can pin with their own CA bundle if they so choose. Otherwise, the default verification mechanism in therequests
library now applies (this usescertifi
and/or system certificates, depending on the configuration).Improves integration tests to cover both scenarios (i.e. when a bundle is provided, and when one isn't).
Checklist
General Contributing
Is This a Code Change?
Validation
tox
pass?