-
Notifications
You must be signed in to change notification settings - Fork 35
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
Specify API version when querying Docker API #51
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.
Thank you for your contribution !
It seems good. But I think it's desirable that docker configuration is set in the instance of struct Docker
(or in docker module layer).
And for avoiding break existing code, it is preferable to allow users use dockworker without api version.
How do you think?
@@ -8,7 +8,7 @@ | |||
### Environment | |||
|
|||
- Docker | |||
- API 1.31.1 | |||
- API version 1.26 |
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.
thx 😭
I do see how reading
Here are some facts: Docker started supporting versioned endpoints even before 1.0 release, in version 0.3.3, which was like 6 years ago, see moby/moby@faae7220c019882a2160aeeexplicitlya6bebc46c15d702be. In the very same commit there's a check introduced to make sure Docker daemon won't talk to a client with a higher version: https://github.com/moby/moby/blob/faae7220c019882a2160aeea6bebc46c15d702be/api.go#L653-L655 This check indeed won't trigger when we don't specify a version, but allowing users to disable versioned endpoints and hoping for the best sounds like a bad idea. Also, Docker API version 1.25 requires that you use versioned endpoints, but AFAIU this won't be in effect until this minimum version is explicitly enforced by the daemon: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/api/common_unix.go#L6 Perhaps this PR needs a little more thought from the GH-21 point of view, i.e. how exactly version negotiation and library method versioning are going to be implemented. |
This should partially fix GH-21.
Not sure if
HyperClient::new()
is the best place to query Docker-specific environment variables, but otherwise the code wouldn't be DRY.