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.

Sign up for GitHub

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

Jump to bottom

Make compatible with requests 2.29.0 and urllib3 2.0 #3116

Merged
merged 3 commits into from May 5, 2023
Merged

Make compatible with requests 2.29.0 and urllib3 2.0 #3116

merged 3 commits into from May 5, 2023

Conversation

felixfontein
Copy link
Contributor

Fixes at least part of #3113. (Recent requests versions declare that they need urllib3 < 2.0 since at least 2017, so the urllib3 problems themselves shouldn't really appear except when forcing invalid combinations of requests and urllib3. My hope is that the urllib3 2.0 compatibility is something that only needs changes on the side of requests, and not here - but who knows...)

Since requests 2.29.0, requests uses urllib3's native chunking ability (see psf/requests#6226). By using urllib3's HTTPConnection class (which extends httplib.HTTPConnection`) we make sure that our HTTP transports support this as well.

I was able to reproduce the problem by calling APIClient.put_archive(); the change in this PR fixes the problem for me. (I'm working on a modified vendored version of Docker SDK for Python, so YMMV.)

@felixfontein
Make compatible with requests 2.29.0.
8291c94 
Signed-off-by: Felix Fontein <felix@fontein.de>
@felixfontein
Copy link
Contributor Author

Failing CI (https://github.com/docker/docker-py/actions/runs/4861956711/jobs/8667665911?pr=3116) seems to be unrelated since it didn't fail before the force push (which only adjusted the commit message). Restarting CI.

@felixfontein felixfontein reopened this May 2, 2023
@felixfontein felixfontein mentioned this pull request May 2, 2023
Merged
@felixfontein
Copy link
Contributor Author

As a reference, the first test run that didn't fail: https://github.com/docker/docker-py/actions/runs/4861929164/jobs/8667601939

@felixfontein felixfontein mentioned this pull request May 2, 2023
Closed
@felixfontein
Copy link
Contributor Author

According to #3117 and #3113 (comment) this also fixes compatibility with urllib3 2.0, once requests officially supports that.

@felixfontein felixfontein changed the title Make compatible with requests 2.29.0 Make compatible with requests 2.29.0 and urllib3 2.0 May 2, 2023
@felixfontein
Simplify imports since requests >= 2.16.0 no longer uses vendored url…
115dbce 
…lib3.

Signed-off-by: Felix Fontein <felix@fontein.de>
@pquentin pquentin mentioned this pull request May 3, 2023
Closed
@felixfontein
Remove putheader() and response_class().
a48dd4c 
Signed-off-by: Felix Fontein <felix@fontein.de>
@felixfontein felixfontein mentioned this pull request May 3, 2023
Closed
@luzfcb luzfcb mentioned this pull request May 4, 2023
Closed
@felixfontein
Copy link
Contributor Author

@milas can you take a look at this PR? Thanks.

@wadhah101 wadhah101 mentioned this pull request May 5, 2023
Closed
@buluma buluma mentioned this pull request May 5, 2023
Open
@milas milas self-assigned this May 5, 2023
@milas milas self-requested a review May 5, 2023 14:56
milas
milas approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I verified several scenarios under the integration tests (there are calls to put_archive and I see the failures currently without this PR):

  • requests 2.30.0 + urllib3 v2.x
  • requests 2.28.x + urllib3 v1.x
  • requests 2.30.0 + urllib3 v1.x

Looks good across the three. I'll get this merged and a new release tagged momentarily.

@milas milas merged commit 3178c8d into docker:main May 5, 2023
10 checks passed
JonasPammer added a commit to JonasPammer/cookiecutter-ansible-role that referenced this pull request May 5, 2023
@JonasPammer
fix: don't mention docker dep itself
8c87d3c 
as done in JonasPammer/ansible-role-bootstrap#83
i think the whole reason i started said PR was because I wondered "why cap it at that without reason?".
molecule-plugins[docker] brings the docker dep, with a fitting ">" delimiter which for example brings us docker/docker-py#3116 thus this PR closes #103
@JonasPammer JonasPammer mentioned this pull request May 5, 2023
Merged
@felixfontein felixfontein deleted the requests branch May 5, 2023 19:19
@felixfontein
Copy link
Contributor Author

@pquentin @milas thanks a lot for reviewing, testing, and merging!

@k-v1 k-v1 mentioned this pull request May 7, 2023
Closed
@alexdesiqueira alexdesiqueira mentioned this pull request May 8, 2023
Merged
5 tasks
@nikunjdh nikunjdh mentioned this pull request May 8, 2023
Closed
@josephkishan
Copy link

@pquentin @milas @felixfontein we facing the same issue like "TypeError: request() got an unexpected keyword argument 'chunked'" We are using docker as 5.0.3 and requests as 2.30.0 and urllib3 as 2.0.2. Is these versions fine or do we need to pin the version until the issue resolves? If we have to pin then please suggest to us the versions we need to resolve this issue

@pquentin
Copy link

pquentin commented May 9, 2023

@josephkishan You can keep requests 2.30.0 and urllib3 2.0.2, but you should upgrade docker to 6.1.0 or 6.1.1.

k-v1 added a commit to k-v1/sonic-buildimage that referenced this pull request May 9, 2023
@k-v1
[Build] update python package docker in host image to 6.1.1
7efddee 
Fix sonic-net#14974
Refs: docker/docker-py#3116
@k-v1 k-v1 mentioned this pull request May 9, 2023
Merged
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 10, 2023
@k-v1
[Build] update python package docker in host image to 6.1.1 (#14993)
bba4fb8 
Fix #14974
Refs: docker/docker-py#3116
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 12, 2023
@k-v1 @mssonicbld
[Build] update python package docker in host image to 6.1.1 (sonic-ne…
20eb91e 
…t#14993)

Fix sonic-net#14974
Refs: docker/docker-py#3116
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 12, 2023
@k-v1 @mssonicbld
[Build] update python package docker in host image to 6.1.1 (sonic-ne…
36b66ac 
…t#14993)

Fix sonic-net#14974
Refs: docker/docker-py#3116
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 12, 2023
@k-v1 @mssonicbld
[Build] update python package docker in host image to 6.1.1 (sonic-ne…
6b09e86 
…t#14993)

Fix sonic-net#14974
Refs: docker/docker-py#3116
yxieca pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 15, 2023
@mssonicbld @k-v1
[Build] update python package docker in host image to 6.1.1 (#14993) (#…
4ee8224 
…15050)

Fix #14974
Refs: docker/docker-py#3116

Co-authored-by: Konstantin Vasin <126960927+k-v1@users.noreply.github.com>
mssonicbld pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 19, 2023
@k-v1 @mssonicbld
[Build] update python package docker in host image to 6.1.1 (#14993)
7a87170 
Fix #14974
Refs: docker/docker-py#3116
@mssonicbld mssonicbld mentioned this pull request Jul 28, 2023
Merged
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Jul 28, 2023
@k-v1 @mssonicbld
[Build] update python package docker in host image to 6.1.1 (sonic-ne…
9c49de6 
…t#14993)

Fix sonic-net#14974
Refs: docker/docker-py#3116
@cebtenzzre cebtenzzre mentioned this pull request Oct 10, 2023
Closed
10 tasks
@Tangresh Tangresh mentioned this pull request Nov 9, 2023
Closed
9 tasks
@adamnovak adamnovak mentioned this pull request Feb 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pquentin pquentin pquentin approved these changes

@milas milas milas approved these changes

@aiordache aiordache Awaiting requested review from aiordache

@ulyssessouza ulyssessouza Awaiting requested review from ulyssessouza

Assignees

@milas milas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@felixfontein @josephkishan @pquentin @milas