Api client lib by calavera · Pull Request #18472 · moby/moby

calavera · 2015-12-07T17:16:10Z

⚠️ Too big to review, go to my fork and navigate the api/client package ⚠️

I extracted a standalone client library from the cli that can be used outside of docker. In the process of doing that I end up removing a bunch of code from the cli.

Why do you do this to us??!!

We should own the main go library to interact with docker. This makes the docker cli be the first consumer of this library.

How did you do this??!

I extracted all the calls the client needs into an interface, you can see it here:
https://github.com/calavera/docker/blob/api_client_lib/api/client/client.go#L19

Functions are very well defined but there are a couple things to consider.

I removed `cli.stream` and `cli.hijack` entirely.

After reading every single line in the client, I believe nobody knows how those two functions work and they made our code very complicated. You can compare the start command, no need for a diff:

Master: https://github.com/docker/docker/blob/master/api/client/start.go
My branch: https://github.com/calavera/docker/blob/api_client_lib/api/client/start.go

Every response in the new library that can be streamed returns an io.ReadCloser. It's up to the caller to implement streaming. This makes the intention of the caller much more clear. Compare the image create functions:

Master: https://github.com/docker/docker/blob/master/api/client/create.go#L53-L70
My branch: https://github.com/calavera/docker/blob/api_client_lib/api/client/create.go#L51-L63

did you know that the response of that function was always a json message that we stream in the client?

Breaking changes

Of course there are always consequences. I introduced what can be considered as a breaking change. In my opinion, the new behavior is more correct 😸

Until now, docker inspect continued evaluating containers if you typed a wrong id, see this example:

$ docker run -d --name test alpine true
$ docker inspect this_container_does_not_exist test

The message about this_container_does_not_exist not existing would be followed by the test container output, a json payload of about 100 lines. This makes almost impossible to see the error.

With the new behavior, the cli stops execution as soon as it finds an error. This is a more predictable behavior. Users know sooner and more clear that there is a problem with the command they just executed.

The code in inspect has also been highly simplified. Compare both files:

Master: https://github.com/docker/docker/blob/master/api/client/inspect.go
My branch: https://github.com/calavera/docker/blob/api_client_lib/api/client/inspect.go
https://github.com/calavera/docker/blob/api_client_lib/api/client/inspect/inspector.go

This is impossible to review

I'm afraid so too. I tried very hard to keep changes very well defined. But the best way to review this change might be to browse my branch directly, and to play with the cli.

The new client library is in https://github.com/calavera/docker/blob/api_client_lib/api/client/lib

LK4D4 · 2015-12-07T17:20:53Z

I think probably inspect should fail if any of containers doesn't exist :)

icecrime · 2015-12-07T17:38:38Z

Oh my 😍 That looks pretty impressive!

LK4D4 · 2015-12-07T17:46:15Z

Everything failed 🎆

LK4D4 · 2015-12-07T18:46:56Z

Haha, seems like tests fixing won't be easy

duglin · 2015-12-07T18:59:30Z

Its kind of disturbing that the change in semantics you mentioned didn't result in a change to a testcase. Aside from squashing this down :-) can we get a new testcase for that?

btw - I really like the idea of a cli lib!

vdemeester · 2015-12-07T19:03:01Z

😻 Will have to take a look but \o/.

tonistiigi · 2015-12-07T20:51:20Z

api/client/client.go

Do we need both ImageCreate and ImagePull? Their implementation looks the same and ImageCreate is only called by pull helper. Also Parent in ImageCreate looks misleading as it is still just a reference to pull(together with tag).

we probably don't need both, it's something to consider. I agree that the implementation is very similar.

tonistiigi · 2015-12-07T21:12:36Z

api/client/client.go

Does this have to return pointer? + few other places (ExecStartCheck, ExecConfig). I think if there is no strict requirement for it we should be consistent with other methods.

I've changed these references to be consistent.

tonistiigi · 2015-12-07T22:23:14Z

api/client/stats.go

should still check for all flag

good catch! thank you.

tonistiigi · 2015-12-07T22:45:31Z

docker cp seems to be broken. I get http: read on closed response body when copying from container and Error response from daemon: Untar re-exec error: exit status 1: output: unexpected EOF when copying to container. ~~May need to add more tests if this is the case(maybe they only check API atm).~~ edit: sorry for double post, janky has same error

calavera · 2015-12-07T22:49:26Z

@tonistiigi yep, there are still a few tests failing that I'm currently fixing. docker cp included.

tianon · 2015-12-07T23:59:07Z

Definitely high-level +1! ❤️

jessfraz · 2015-12-08T02:26:35Z

nice love the description definitely helps :)

calavera · 2015-12-08T03:09:26Z

@duglin I totally agree with you, I was very suspicious about not seen inspect test failing. I've added some in my last commit. All inspects commands are implemented exactly in the same way, so all of them behave alike 🙌

19cdcd1

tianon · 2015-12-09T21:44:41Z

https://github.com/docker/docker/blob/master/project/PACKAGERS.md#build-dependencies We do still officially support compiling with Go 1.4, so we'll need to figure out something (or decide whether that requirement can be bumped, but it's sounding like that's not really plausible right at the moment).

calavera · 2015-12-09T21:47:02Z

okay, I'll remove that call and go back to the ugly workaround 😔

Thanks for checking!

LK4D4 · 2015-12-09T21:48:06Z

@calavera You can use version build tags! Like +build !go1.5

calavera · 2015-12-09T21:57:01Z

@LK4D4 indeed. Doing that.

lowenna · 2015-12-09T22:18:40Z

@calavera Hang fire - I'll update the CI nodes, shouldn't take too long. I have already updated node 1 as I'm playing with Windows CI on it. @mikedougherty I assume you're OK me updating 2..9?

brahmaroutu · 2015-12-09T22:46:06Z

gccgo master builds failed because of this. I think the best would be to add gccgo PR builds to catch go1.5 related breakage earlier?
@tianon go1.5 support is on main branch, can we build gcc:6.0 image from master and use it for x86, Power and Z?

tianon · 2015-12-09T22:54:24Z

It's OK, he's adding a shim for Go 1.4 support (which is still worthwhile to keep for now IMO).

lowenna · 2015-12-09T23:07:12Z

We have to move to 1.5.2 on those machines anyway. I just accelerated the update. All nodes are updated with the exception of Jenkins-prs-3. @mikedougherty I sent you an email about that node - I was unable to logon to it, even after a restart through the portal.

WeiZhang555 · 2015-12-10T12:04:31Z

Wow, this is really amazing!! 👍

mikedougherty · 2015-12-10T19:10:52Z

@jhowardmsft just got host 3 back up and running. sent you an email with details. thanks for the ping

relates to: - moby/moby#11397 - moby/moby#18472 This warning was added in [moby@4a8b3ca] to print a warning when building Linux images from a Windows client. Window's filesystem does not have an "executable" bit, which mean that, for example, copying a shell script to an image during build would lose the executable bit. So for Windows clients, the executable bit would be set on all files, unconditionally. Originally this was detected in the client, which had direct access to the API response headers, but when refactoring the client to use a common library in [moby@535c4c9], this was refactored into a `ImageBuildResponse` wrapper, deconstructing the API response into an `io.Reader` and a string field containing only the `OSType` header. This was the only use and only purpose of the `OSType` field, and now that BuildKit is the default builder for Linux images, this warning didn't get printed unless BuildKit was explicitly disabled. This patch removes the warning, so that we can potentially remove the field, or the `ImageBuildResponse` type altogether. [moby@4a8b3ca]: moby/moby@4a8b3ca [moby@535c4c9]: moby/moby@535c4c9 Signed-off-by: Sebastiaan van Stijn <[email protected]>

GordonTheTurtle added the status/0-triage label Dec 7, 2015

LK4D4 added status/2-code-review status/1-design-review and removed status/0-triage status/2-code-review labels Dec 7, 2015

calavera force-pushed the api_client_lib branch 2 times, most recently from 3328c57 to fc61f80 Compare December 7, 2015 18:08

calavera mentioned this pull request Dec 7, 2015

Make sure the input reader is closed after loading an image from file. #18447

Closed

calavera force-pushed the api_client_lib branch 2 times, most recently from 3737202 to b77aed4 Compare December 7, 2015 19:40

tonistiigi reviewed Dec 7, 2015
View reviewed changes

calavera force-pushed the api_client_lib branch from b77aed4 to d418fa3 Compare December 7, 2015 21:05

tonistiigi reviewed Dec 7, 2015
View reviewed changes

calavera force-pushed the api_client_lib branch from d418fa3 to 07c1961 Compare December 7, 2015 22:02

tonistiigi reviewed Dec 7, 2015
View reviewed changes

calavera force-pushed the api_client_lib branch from 07c1961 to 13d6f25 Compare December 7, 2015 23:33

calavera force-pushed the api_client_lib branch from 13d6f25 to 1db5748 Compare December 8, 2015 03:06

calavera force-pushed the api_client_lib branch from 1db5748 to 19cdcd1 Compare December 8, 2015 03:10

thaJeztah mentioned this pull request Dec 10, 2015

Change the quiet flag behavior in the build command [closes #17623] #17428

Merged

This was referenced Dec 11, 2015

Add support for blkio read/write iops device #15879

Merged

Added additional container information to "docker info". #17414

Closed

Add support for forwarding Docker client through SOCKS proxy #18373

Closed

nalind mentioned this pull request Dec 14, 2015

Add authentication to the docker daemon #18514

Closed

jimmyxian mentioned this pull request Dec 21, 2015

Proposal: Use docker client library docker-archive/classicswarm#1560

Closed

allencloud mentioned this pull request Feb 2, 2017

do not fail fast when executing inspect command #30648

Merged

This was referenced Feb 28, 2021

ContainerListOptions keyword Quiet not worked! #42092

Closed

API-client: remove unused ContainerListOptions.Quiet field #42098

Merged

thaJeztah mentioned this pull request Dec 20, 2022

FIXME: Got an API for which error does not match any expected type!!! #38877

Closed

thaJeztah mentioned this pull request Mar 22, 2023

Respect Latest option in ContainerList #45189

Open

This was referenced Jul 13, 2025

cli/command/container: don't set CopyToContainerOptions.AllowOverwriteDirWithFile docker/cli#6176

Merged

daemon: ContainerExtractToDir: make AllowOverwriteDirWithFile opt-in #50404

Merged

thaJeztah mentioned this pull request Sep 5, 2025

cli/command/image: build: remove permissions warning on Windows docker/cli#6432

Merged

This was referenced Sep 16, 2025

api/types/container: move ExecInspect type to client #50991

Merged

client: ImageBuildResponse: remove OSType field #50995

Merged

Conversation

calavera commented Dec 7, 2015

Why do you do this to us??!!

How did you do this??!

I removed cli.stream and cli.hijack entirely.

Breaking changes

This is impossible to review

Uh oh!

LK4D4 commented Dec 7, 2015

Uh oh!

icecrime commented Dec 7, 2015

Uh oh!

LK4D4 commented Dec 7, 2015

Uh oh!

LK4D4 commented Dec 7, 2015

Uh oh!

duglin commented Dec 7, 2015

Uh oh!

vdemeester commented Dec 7, 2015

Uh oh!

tonistiigi Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

calavera Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

tonistiigi Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

calavera Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

tonistiigi Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

calavera Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Dec 7, 2015

Uh oh!

calavera commented Dec 7, 2015

Uh oh!

tianon commented Dec 7, 2015 via email

Uh oh!

jessfraz commented Dec 8, 2015

Uh oh!

calavera commented Dec 8, 2015

Uh oh!

tianon commented Dec 9, 2015 via email

Uh oh!

calavera commented Dec 9, 2015

Uh oh!

LK4D4 commented Dec 9, 2015

Uh oh!

calavera commented Dec 9, 2015

Uh oh!

lowenna commented Dec 9, 2015

Uh oh!

brahmaroutu commented Dec 9, 2015

Uh oh!

tianon commented Dec 9, 2015 via email

Uh oh!

lowenna commented Dec 9, 2015

Uh oh!

WeiZhang555 commented Dec 10, 2015

Uh oh!

mikedougherty commented Dec 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

I removed `cli.stream` and `cli.hijack` entirely.