Remove cmd/docker and other directories in cli/ in accordance with the new Moby project scope by tiborvass · Pull Request #32694 · moby/moby

tiborvass · 2017-04-18T15:29:49Z

Moving the client-side binary to the https://github.com/docker/cli repository in accordance with the transition to the Moby project.

See #32691 and #32867 for context.

Starting with this commit, integration tests should no longer rely on
the docker cli, they should be API tests instead. For the existing tests
the scripts will use a frozen version of the docker cli with a
DOCKER_API_VERSION frozen to 1.30, which should ensure that the CI remains
green at all times.

To help contributors develop and test manually with a modified docker
cli, this commit also adds a DOCKER_CLI_PATH environment variable to the
Makefile. This allows to set the path of a custom cli that will be
available inside the development container and used to run the
integration tests.

Example on how to make changes to do iterative dev:

[docker/cli] $ make build # or make -f docker.Makefile build
[docker/cli] $ cd $GOPATH/src/github.com/moby/moby
[moby/moby] $ DOCKER_CLI_PATH=~/go/src/github.com/docker/cli/build/docker make shell
# docker -v     # this is your dev version of the docker cli
# ./hack/make.sh binary test-integration-cli    # to run tests against your custom docker cli

New workflow for PRs impacting the Docker CLI:

You can continue doing test & dev cycles in the dev container as shown in the example above.
When you are ready to submit the changes, you send a PR to moby/moby (in the monolith-less future, to the component itself), and write API tests to make sure the change makes sense in the backend
Once the PR is approved, you submit the CLI changes to the docker/cli repo with unit tests
Potentially, add more tests to https://github.com/docker/docker-e2e

Additionally, in order to get a green CI, the PR fixes a CGO_ENABLED envvar leak with our install-binaries.sh script, and modifies a Go build tag in pkg/term to ensure we catch the issue earlier next time.

AkihiroSuda · 2017-04-18T16:59:04Z

How to test future moby features without CLI?

krasi-georgiev · 2017-04-19T15:02:36Z

@AkihiroSuda hack/make/.fetch-test-client it seems that will be installed as binary

AkihiroSuda · 2017-04-19T16:46:35Z

Docker product CLI could not be used for testing new features that are not available yet in Docker product.

How about renaming cli to testcli rather than completely removing it?

dnephin · 2017-04-19T19:52:20Z

I think the idea would be to write tests that target the API instead of using the CLI to test everything. Writing tests against the API is actually a better idea because it helps verify the quality of the API design. Writing tests using the CLI hides mistakes in API design (and could even hide accidental breaking API changes).

The existing CLI will be available for running the existing tests, but new tests should either be unit tests or target the API.

AkihiroSuda · 2017-04-19T20:36:02Z

Yes, I agree API-oriented testing is better, but I just thought manual ad-hoc testing using CLI is still effective, although it is not ideal.

A concrete example is my PR #26331 (introspection mount).
I guess reviewers would want to do ad-hoc testing manually via CLI rather than just checking CI result, because they would need to set random value to introspection-scope=... for testing security aspect.
(Ideally we should do fuzzing on CI, but fuzzing everything is not realistic)

aaronlehmann · 2017-04-19T20:37:47Z

I think the idea would be to write tests that target the API instead of using the CLI to test everything. Writing tests against the API is actually a better idea because it helps verify the quality of the API design. Writing tests using the CLI hides mistakes in API design (and could even hide accidental breaking API changes). The existing CLI will be available for running the existing tests, but new tests should either be unit tests or target the API.

It looks like the API client is being moved as well (to `cli`, which doesn't make sense to me), so I don't think writing tests against the API will help. Either way, I think it's very important to be able to manually test PRs. We shouldn't assume that a new feature works properly just because its tests pass. We previously had engine-api split out and it was very painful. That repo would often get out of sync with docker, and it would become impossible to merge PRs that needed a newer version. I was surprised by this PR that would abruptly go back to that, in a more extreme form. IMHO we should figure out a usable workflow before making such a drastic change.

Vanuan · 2017-04-21T00:52:53Z

Docker product CLI could not be used for testing new features that are not available yet in Docker product.

Why not? If API stays the same, old client should work? If API changes it would be a different version and warrants a change in the cli repo?

cpuguy83 · 2017-04-22T15:34:35Z

With the goal of moby to split out everything from this repo other than core bits , I think this makes perfect sense although is going to be a rough transition to start off.
We do need to define in detail what will be split out and how, but this is an easy first step that will likely lead to a more well tested API and CLI.

That said, I think the API client library needs to live next to the API definition.
We can run CLI tests in the CLI repo or possibly more appropriately in an integration repo.

shykes · 2017-04-25T20:31:20Z

As others have said, moby tests can continue to use the Docker CLI, just as an external dependency instead of having it built-in.
Moving client libraries to docker/cli is temporary, in the near future I would like us to create docker/sdk with all official client libraries for multiple languages.
I hear you @cpuguy83 but we are not going to keep the client library next to the REST api implementation. That's because we are preparing to gradually move the focus away from the rest API, and instead focus on 1) the Docker SDK, and 2) the new component GRPC interfaces, similar to containerd & swarmd, that will gradually appear as we split up the engine.

So the SDK will gradually become the source of truth for Docker APIs, rather than the rest API.

(of course this can be implemented gradually and carefully, and without breaking existing tools)

shykes · 2017-04-25T20:32:51Z

One last point, on the moby side we want to remove client implementation BUT implement tools which can generate custom clients for your blueprint using the grpc definitions. Both client libraries and cli can be generated in that way.

mavenugo · 2017-04-28T18:31:31Z

@tiborvass I think most of the comments above are for rapid-iterative-development and hence I think a bit more explanation on

The Makefile has been updated to allow bind-mounting the folder the cli
binary resides in, to allow for rapid iterative development.

and an example usage will help.

vieux · 2017-04-28T18:31:34Z

Makefile

sorry if i'm not following, what's /usr/local/cli ?

@vieux a place i decided to bind mount the folder where the cli is. I need to bind mount the folder so that rebuilding the binary propagates inside the container.

dnephin

I think docs/yaml needs to be added to docker/cli since it's being removed here, otherwise I think it looks correct

tophj-ibm · 2017-05-05T01:04:51Z

nice teamwork @tiborvass @clnperez 👋

LGTM if green

tiborvass · 2017-05-05T05:29:37Z

The failing powerpc test is a known failure. So CI can be considered green!

justincormack · 2017-05-05T09:56:59Z

shall we merge then...

AkihiroSuda · 2017-05-05T13:32:32Z

needs rebase

nishanttotla · 2017-05-05T16:43:44Z

Makefile

@tiborvass just to clarify, is it the case that setting the environment variable for the CLI path, and then running something like make BIND_DIR=. binary shell will set up a test dind container?

@nishanttotla yes, it will do the same as usual, but will also have the bind-mounted binary specified by DOCKER_CLI_PATH. It will be added to your PATH so you can use it right away inside the dev container. Note that instead of DOCKER_CLI_PATH, you can also specify a different remote and git sha in hack/dockerfile/binaries-commits if you want to show case the full feature on one branch.

…e new Moby project scope Starting with this commit, integration tests should no longer rely on the docker cli, they should be API tests instead. For the existing tests the scripts will use a frozen version of the docker cli with a DOCKER_API_VERSION frozen to 1.30, which should ensure that the CI remains green at all times. To help contributors develop and test manually with a modified docker cli, this commit also adds a DOCKER_CLI_PATH environment variable to the Makefile. This allows to set the path of a custom cli that will be available inside the development container and used to run the integration tests. Signed-off-by: Arnaud Porterie (icecrime) <arnaud.porterie@docker.com> Signed-off-by: Tibor Vass <tibor@docker.com>

Signed-off-by: Tibor Vass <tibor@docker.com>

vieux · 2017-05-05T22:04:20Z

LGTM 🎉

tiborvass added area/cli Client area/project labels Apr 18, 2017

ijc mentioned this pull request Apr 19, 2017

Add docker build --iidfile=FILE #32406

Merged

rheinwein mentioned this pull request Apr 21, 2017

Should the moby CLI be moved? linuxkit/linuxkit#1680

Closed

AkihiroSuda mentioned this pull request Apr 25, 2017

Fix prefix-matching for service ps #32800

Closed

tiborvass mentioned this pull request Apr 25, 2017

windows: remove workaround for go#15286 and have compiling work on windows docker/cli#8

Merged

This was referenced Apr 27, 2017

Replace integration-cli suite with API test suite #32866

Open

[META] Splitting moby and docker #32867

Open

Define a plan for HTTP engine API #32873

Open

tiborvass force-pushed the cli_removal branch 6 times, most recently from 004ba7b to d28a396 Compare April 28, 2017 17:20

tiborvass added the rebuild/z label Apr 28, 2017

GordonTheTurtle removed the rebuild/z label Apr 28, 2017

vieux reviewed Apr 28, 2017

View reviewed changes

dnephin reviewed Apr 28, 2017

View reviewed changes

tiborvass force-pushed the cli_removal branch 4 times, most recently from c16ba14 to 9b993b7 Compare May 5, 2017 00:59

tophj-ibm approved these changes May 5, 2017

View reviewed changes

tiborvass added the rebuild/windowsRS1 label May 5, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label May 5, 2017

tiborvass force-pushed the cli_removal branch from 9b993b7 to 60df8fb Compare May 5, 2017 16:12

nishanttotla reviewed May 5, 2017

View reviewed changes

tiborvass force-pushed the cli_removal branch 2 times, most recently from 9f45491 to 1a61a99 Compare May 5, 2017 19:06

Arnaud Porterie (icecrime) and others added 4 commits May 5, 2017 12:14

fix expected output in tests

f47a61c

Signed-off-by: Tibor Vass <tibor@docker.com>

Enforce CGO_ENABLED=0 only for docker-proxy

e30c2e8

Signed-off-by: Tibor Vass <tibor@docker.com>

Prevent ppc64le to build pkg/term without cgo

71e9f5c

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the cli_removal branch from 1a61a99 to 71e9f5c Compare May 5, 2017 19:14

tiborvass merged commit 34337db into moby:master May 5, 2017

GordonTheTurtle added this to the 17.06.0 milestone May 5, 2017

This was referenced May 6, 2017

Remove compose 3.3 schema #33062

Merged

Add support for labels during build docker/compose#4735

Closed

Support placement preferences in stack deployment #32743

Closed

tianon mentioned this pull request May 9, 2017

TTY Output Regression #33101

Closed

AkihiroSuda mentioned this pull request May 13, 2017

Fixes #10824. Adjusted docker stats memory output #32777

Closed

albers mentioned this pull request Jun 23, 2017

Add bash completion to the development container #33801

Merged

tiborvass deleted the cli_removal branch July 17, 2019 00:34

Conversation

tiborvass commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Apr 18, 2017

Uh oh!

krasi-georgiev commented Apr 19, 2017

Uh oh!

AkihiroSuda commented Apr 19, 2017

Uh oh!

dnephin commented Apr 19, 2017

Uh oh!

AkihiroSuda commented Apr 19, 2017

Uh oh!

aaronlehmann commented Apr 19, 2017 via email

Uh oh!

Vanuan commented Apr 21, 2017

Uh oh!

cpuguy83 commented Apr 22, 2017

Uh oh!

shykes commented Apr 25, 2017

Uh oh!

shykes commented Apr 25, 2017

Uh oh!

mavenugo commented Apr 28, 2017

Uh oh!

vieux Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

tiborvass Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

tophj-ibm commented May 5, 2017

Uh oh!

tiborvass commented May 5, 2017

Uh oh!

justincormack commented May 5, 2017

Uh oh!

AkihiroSuda commented May 5, 2017

Uh oh!

nishanttotla May 5, 2017

Choose a reason for hiding this comment

Uh oh!

tiborvass May 5, 2017

Choose a reason for hiding this comment

Uh oh!

vieux commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

tiborvass commented Apr 18, 2017 •

edited

Loading