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

Specify API version when querying Docker API #51

Closed
wants to merge 2 commits into from
Closed

Specify API version when querying Docker API #51

wants to merge 2 commits into from

Conversation

edigaryev
Copy link
Contributor

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.

@eldesh eldesh added the reviewing Reviewing, wait a moment label Jul 1, 2019
Copy link
Collaborator

@eldesh eldesh left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

thx 😭

@eldesh eldesh added review-done Review is done. Next actor is the author and removed reviewing Reviewing, wait a moment labels Jul 3, 2019
@edigaryev
Copy link
Contributor Author

But I think it's desirable that docker configuration is set in the instance of struct Docker (or in docker module layer).

I do see how reading DOCKER_API_VERSION environment variable in a module responsible for HTTP communication is somewhat ugly, but at the same time I'm not sure how I'd go about this one without introducing unjustified code duplication/complication.

And for avoiding break existing code, it is preferable to allow users use dockworker without api version. How do you think?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-done Review is done. Next actor is the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

specify api version
2 participants