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

Remove vendored Requests #1817

Closed
wants to merge 1 commit into from

Conversation

Jamim
Copy link

@Jamim Jamim commented Aug 28, 2019

Hi everyone,

The documentation says that vendored versions of requests and urllib3 are no longer being used and have been replaced with a direct dependency on upstream urllib3 and requests is no longer a dependency of botocore.

However, the requests is no longer a dependency of botocore statement is not true currently, since requests is used in botocore/exceptions.py and in tests.

I believe it's time to remove vendored requests.

This PR closes #1122, closes #1608, closes #1615, and closes jantman/awslimitchecker#421.

Documentation says that vendored versions of requests and urllib3
are no longer being used and have been replaced with
a direct dependency on upstream urllib3 and requests
is no longer a dependency of botocore.
https://botocore.amazonaws.com/v1/documentation/api/latest/index.html#upgrading-to-1-11-0

However, the "requests is no longer a dependency of botocore"
statement is not true currently, since requests are used
in botocore/exceptions.py and in tests.

I believe it's time to remove vendored requests.
@codecov-io
Copy link

codecov-io commented Aug 28, 2019

Codecov Report

Merging #1817 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1817      +/-   ##
===========================================
+ Coverage    92.51%   92.54%   +0.03%     
===========================================
  Files           53       53              
  Lines         9958     9958              
===========================================
+ Hits          9213     9216       +3     
+ Misses         745      742       -3
Impacted Files Coverage Δ
botocore/endpoint.py 98.58% <ø> (ø) ⬆️
botocore/exceptions.py 99.39% <100%> (ø) ⬆️
botocore/credentials.py 98.85% <0%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae55c98...6fa86a3. Read the comment docs.

@Jamim
Copy link
Author

Jamim commented Sep 10, 2019

Hello @jamesls,
Could you please take a look at this PR?
Thank you!

@jamesls
Copy link
Member

jamesls commented Sep 20, 2019

Hi, thanks for the PR. We won't be able to remove the entire requests directory. We want to keep the exceptions in place in order to help with backwards compatibility.

Additionally, some of the changes in here won't work as is. For example in the exceptions.py the imports to vendored requests are removed but replaced with a direct dependency on requests (https://github.com/boto/botocore/pull/1817/files#diff-7768fbeb174f209a8fc3ed77a823ac1c). We explicitly do not want to depend on requests (only urllib3) and this won't fix the reason they were left in the first place (to preserve backwards compatibility for people catching those vendored exceptions).

To help speed this along, I've gone ahead and added a PR that I think this is a compromise that can make everyone happy:

  • Remove all the requests/urllib3 code with the exception of the exception classes
  • Update the tests to not use vendored requests unless specifically testing backwards compatibility related to vendored requests.

Let me know if you have any questions or concerns.

@jamesls jamesls closed this Sep 20, 2019
@Jamim Jamim deleted the feature/remove-vendored-requests branch October 24, 2019 11:19
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