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

feat: move websocket-client to extra dependency #3123

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented May 10, 2023

Since websocket-client is only required when using websockets for attaching to containers (#62), I don't think it needs to be a required dependency.

This also bumps the minimum required version of websocket-client to that pinned by #3022.

@akx akx force-pushed the optional-websockets branch 2 times, most recently from 0af8e71 to ba5f6b1 Compare May 12, 2023 05:23
@akx akx force-pushed the optional-websockets branch from ba5f6b1 to 8319d56 Compare June 6, 2023 14:24
@milas milas self-assigned this Aug 14, 2023
@milas milas added kind/enhancement dependencies Pull requests that update a dependency file labels Aug 14, 2023
@milas milas requested review from milas and removed request for ulyssessouza and aiordache August 14, 2023 18:58
Copy link
Contributor

@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!

Out of curiosity, where/how are you using this library that you want to avoid the websocket-client dependency?

Technically, these changes look good. Thanks for the nice error message when it's not there. I'm not going to merge this right now, though: at a minimum, I want to do a major version bump along with this.

I'd also like to hear if others are interested in this (feel free to 👍🏻 this PR or comment with details), as it will potentially require a LOT of downstream consumers of this library to update their requirements.txt (or modern eqvuialent) since I think many do rely on websocket support when using the Engine API.

Also bump minimum version to that prescribed by docker#3022

Signed-off-by: Aarni Koskela <[email protected]>
@akx akx force-pushed the optional-websockets branch from 8319d56 to c9799f8 Compare August 15, 2023 10:23
@akx
Copy link
Contributor Author

akx commented Aug 15, 2023

@milas Hi, thanks for getting back on this! We have a situation where we'd like to avoid downloading extra dependencies in general – and from an attack surface/SBOM point of view, the less code there is, the better.

attach_websocket doesn't seem to be used all that much in the wild but sure, I agree this needs a version bump of some sort. #3126 would probably be nice to get in too at that point :)

@milas milas added this to the v7 milestone Nov 20, 2023
Copy link
Contributor

@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.

Merging this - I'm going to do a major version bump to v7 for Python 3.12 compatibility, as I had to remove some old functionality in the process that constitutes a breaking change, so it's a good time to accept this as well.

Thanks for the PR!

@milas milas merged commit c9e3efd into docker:main Nov 20, 2023
@akx
Copy link
Contributor Author

akx commented Nov 21, 2023

@milas Great, thanks! Should v7 also drop Python 3.7 support? It's EOL date was 4 months ago.

@akx akx deleted the optional-websockets branch December 21, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change dependencies Pull requests that update a dependency file kind/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants