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

Fix for requests 2.32 #3257

Merged
merged 3 commits into from
May 22, 2024
Merged

Fix for requests 2.32 #3257

merged 3 commits into from
May 22, 2024

Conversation

felixfontein
Copy link
Contributor

The current requests 2.32.0 release breaks Docker SDK for Python (see #3256) due to psf/requests@c0813a2.

That commit makes send() call _get_connection() instead of get_connection(). Now the Docker SDK for Python code overwrites get_connection(), but of course doesn't magically overwrite _get_connection() as well...

This PR introduces a BaseHTTPAdapter._get_connection() method that simply calls get_connection().

See for example https://github.com/docker/docker-py/blob/main/docker/transport/unixconn.py#L66.

Fixes #3256.

@felixfontein
Copy link
Contributor Author

The CI failures seem to be unrelated.

Copy link

@colintle colintle left a comment

Choose a reason for hiding this comment

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

Approved

jmsanders added a commit to dagster-io/dagster that referenced this pull request May 21, 2024
Both docker-py and requests have fixes in flight. We can narrow our pin
to only exclude the breaking version.

docker/docker-py#3257
psf/requests#6707 (comment)
@felixfontein felixfontein marked this pull request as draft May 21, 2024 15:56
@felixfontein
Copy link
Contributor Author

This will need to get updated for psf/requests#6707 (comment). Once that one is finalized I'll update the PR.

@felixfontein
Copy link
Contributor Author

I've updated it to be compatible with requests 2.32.2+, which will include psf/requests#6710.

@felixfontein felixfontein changed the title Hotfix for requests 2.32.0 Fix for requests 2.32 May 21, 2024
@felixfontein felixfontein marked this pull request as ready for review May 21, 2024 16:46
# https://github.com/psf/requests/commit/c0813a2d910ea6b4f8438b91d315b8d181302356
# changes requests.adapters.HTTPAdapter to no longer call get_connection() from
# send(), but instead call _get_connection().
def _get_connection(self, request, *args, proxies=None, **kwargs):

Choose a reason for hiding this comment

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

I'll be yanking 2.32.0 and 2.32.1 once we get 2.32.2 out so this can probably be omitted from the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed these lines. (For everyone else: now 2.32.2 is out, and 2.23.0 and 23.2.1 have been yanked.)

Choose a reason for hiding this comment

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

you mean 2.32.0 and 2.32.1 yanked? I still got it installed as an indirect pip dependency just an hour ago.

Choose a reason for hiding this comment

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

Caching perhaps. You can see here they are yanked: https://pypi.org/project/requests/#history.

@krissetto
Copy link
Contributor

@felixfontein I'm away from my pc right now, but just to be clear: this would be a fix for requests versions 2.32.2 and up, correct? maybe it's also worth updating the dependency definition in pyproject.yaml to avoid accidentally using 2.32.0-2.32.1, if they would still be problematic versions

@nathan-vm
Copy link

@nilbacardit26 We are currently working on getting this released

Any prediction to launch this?

@Rogalek
Copy link

Rogalek commented May 23, 2024

hey guys, will you release this hotfix?

@nilbacardit26 We are currently working on getting this released

@krissetto
Copy link
Contributor

Sorry for the delay, we had some unexpected complications to manage for this release.

Version 7.1.0 should now be live on pypi, thx for the patience <3

jstucke added a commit to fkie-cad/FACT_core that referenced this pull request May 27, 2024
jstucke added a commit to fkie-cad/FACT_core that referenced this pull request May 27, 2024
* ---
updated-dependencies:
- dependency-name: requests
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

* update docker-py to a version compatible with requests 2.32

* updated requests again to fix Not supported URL scheme error

see docker/docker-py#3257

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jörg Stucke <joerg.stucke@fkie.fraunhofer.de>
maxhoesel added a commit to maxhoesel-ansible/ansible-collection-smallstep that referenced this pull request Jun 5, 2024
After yet another breaking package update
(docker/docker-py#3257),
its high time we permanently pin all development packages
*and their dependencies* to a known.-good version.
pip-tools lets us do that without losing the simple requirements.txt
file that allows easy installation, so lets use that.
maxhoesel added a commit to maxhoesel-ansible/ansible-collection-smallstep that referenced this pull request Jun 5, 2024
After yet another breaking package update
(docker/docker-py#3257),
its high time we permanently pin all development packages
*and their dependencies* to a known.-good version.
pip-tools lets us do that without losing the simple requirements.txt
file that allows easy installation, so lets use that.
maxhoesel added a commit to maxhoesel-ansible/ansible-collection-smallstep that referenced this pull request Jun 6, 2024
* lock transitive python dependencies with pip-tools

After yet another breaking package update
(docker/docker-py#3257),
its high time we permanently pin all development packages
*and their dependencies* to a known.-good version.
pip-tools lets us do that without losing the simple requirements.txt
file that allows easy installation, so lets use that.

* rework testing harness for py12, remove pytest-virtualenv dep

a little while ago, we were starting to get "imp" import errors
in CI builds, which I initially were assumed to be from pytest.
In fact, it was the pytest-virtualenv package that we used
to generate a virtualenv with a specific ansible version for tests.

As there is no easy replacement for this package, and the package
itself is stale (no release in 5 years), the testing harness now just
installs the desired version of ansible-core straight into the main
devel virtualenv. This is a bit ugly as it could interfere with other
user tasks, but it leaves the interface the same and helps simplify the test fixture code.
Said code also now reliably isolates tes environments from another and the user-wide collections.

Combined with package pinning introduced in the previous commit,
we should no longer have CI failures due to packages or their deps
updating and breaking things.
@onprema
Copy link

onprema commented Jun 17, 2024

If you found yourself here because you had an error such as:

Error while fetching server API version: Not supported URL scheme http+docker

You can probably resolve it by upgrading your docker Python package to 7.1.0, thanks to the kind people in this thread:

pip install docker==7.1.0

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.

Breaks with requests 2.32.0: Not supported URL scheme http+docker