Transform docker-app as a docker-cli plugin #469

silvin-lubecki · 2019-02-26T16:49:33Z

- What I did
Transform docker-app as a docker-cli plugin using docker-cli helpers.

Add a new binary docker-app-standalone which is in fact the old docker-app before its pluginization.

Now each released tar.gz file per os comes with two binaries in it:

docker-app-plugin-{linux|darwin|windows}
docker-app-standalone-{linux|darwin|windows}

- How to verify it

With a master docker-cli built on your own:

$ make bin/docker-app
$ mkdir ~/.docker/cli-plugins && cp bin/docker-app ~/.docker/cli-plugins
$ docker
...
Management Commands:
  app*        Docker Application Packages (Docker Inc., v0.6.0...)
...

- Description for the changelog

Transform docker-app as a docker-cli plugin

- A picture of a cute animal (not mandatory but encouraged)

ijc · 2019-02-27T11:47:41Z

Gopkg.toml

@@ -29,7 +29,7 @@ required = ["github.com/wadey/gocovmerge"]

 [[override]]
  name = "github.com/docker/cli"
-  revision = "f95ca8e1ba6c22c9abcdbf65e8dcc39c53958bba"
+  revision = "3ddb3133f5b5a74dd4e3f721ced21f4d0b9651b6"


(random location for comment on commit message)

The first commit here is titled Bump docker-cli to include the plugin work but actually it does far more than that -- it actually match the switch to being a plugin. I'd suggest a title like "Refactor as a CLI plugin" (or "Convert to...") and leave the mention of the CLI bump in the body of the commit message.

Another commit is rendered as:

Add a new binary docker-app-standalone which is in fact the old docke… …r-app before its pluginization.

i.e. the first line (summary) is too long. I think:

Add a new binary docker-app-standalone This is in fact the old docker-app before its pluginization. ...

ijc · 2019-02-27T11:53:07Z

e2e/cnab_test.go

-				Env: []string{fmt.Sprintf("DUFFLE_HOME=%s", tmpDir.Path())},
+				Env: append(os.Environ(),
+					fmt.Sprintf("DUFFLE_HOME=%s", tmpDir.Path()),
+					fmt.Sprintf(manager.ReexecEnvvar+"="+dockerCli)),


IIRC since I did the PoC port in in #433 these tests now build a local dockerCli (because of the context stuff) and so that should be "plugin enabled" and IMHO we should use it here instead of doing this workaround.

Basically where below you transform []string{dockerApp, "install" into []string{dockerApp, "app", "install" you should instead transform it to []string{dockerCli, "app", "install" having arranged for the plugin binary to be in ${DOCKER_CONFIG}/cli-plugins or setup ``${DOCKER_CONFIG}/config.json` so it includes the right path.

Looks like you built most of the scaffolding you need for TestInvokePluginFromCLI below.

Yep indeed we should test docker-app as a plugin, called by the CLI, as it is now the preferred way to use docker-app. 👍

ijc · 2019-02-27T11:53:59Z

e2e/plugin_test.go

+  validate    Checks the rendered application is syntactically correct
+  version     Print version information
+
+Run 'docker app COMMAND --help' for more information on a command.`


I think you should use golden for this block

ijc · 2019-02-27T11:56:20Z

internal/packager/extract.go

@@ -31,7 +31,7 @@ func findApp() (string, error) {
 	for _, c := range content {
 		if strings.HasSuffix(c.Name(), internal.AppExtension) {
 			if hit != "" {
-				return "", fmt.Errorf("multiple applications found in current directory, specify the application name on the command line")
+				return "", fmt.Errorf("Error: multiple applications found in current directory, specify the application name on the command line")


Elsewhere we needed to remove the Error: prefix (because core plugin framework deals with it), so is this case correct (doesn't result in Error: Error:)?

There's an e2e test which checks this error. It failed until I added this "Error: "...

Oh right, I even realised this in my WIP PR where I wrote in the commit message:

Finally, the idiom in docker/cli (for better or worse) is to include an "Error: " prefix in the error string itself, rather than injecting it when printing. Since CLI plugins follow the behaviour of the CLI here it is necessary to prepend "Error: " to some messages. I've only done exactly those necessary to pass the e2e tests, a fuller audit is very likely required.

(and then forgot about it, obviously)

Feel free to steal some or all of my words for your commit message.

I let this fuller audit as an exercise for the reviewers

ijc · 2019-02-27T11:58:06Z

internal/commands/root.go

-	cmd := &cobra.Command{
-		Use:   "app",
+// NewRootCmd returns the base root command.
+func NewRootCmd() *cobra.Command {


Either add to the comment users must set the Use field on the result or make it an argument and deal with it here.

Right, I could have done that 😅

ijc · 2019-02-27T11:58:54Z

internal/commands/root.go

 		Short: "Docker Application Packages",
 		Long:  `Build and deploy Docker Application Packages.`,
 	}
-	addCommands(cmd, dockerCli)


Both callers do app.AddCommands(cmd, dockerCli) on the result -- maybe just pass dockerCli as an argument and do it here instead?

docker.Makefile

ijc · 2019-03-01T13:26:41Z

At least the pr-merge CI job is suffering from docker/cli#1654 (comment) (that CI has a daemon <18.09). Awaiting maintainer feedback before deciding how to address.

ijc · 2019-03-01T13:28:59Z

pr-head seems to be a different problem (at least so far), vendoring is unclean apparently:


These files were changed:

 M vendor/github.com/docker/cli/cli/command/cli.go

codecov · 2019-03-07T12:46:29Z

Codecov Report

Merging #469 into master will increase coverage by 0.02%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   69.38%   69.41%   +0.02%     
==========================================
  Files          50       50              
  Lines        2515     2501      -14     
==========================================
- Hits         1745     1736       -9     
+ Misses        540      537       -3     
+ Partials      230      228       -2

Impacted Files	Coverage Δ
internal/commands/bundle.go	`66.07% <ø> (ø)`
internal/commands/validate.go	`88.88% <ø> (ø)`
internal/commands/split.go	`80% <ø> (ø)`
internal/commands/pull.go	`80% <ø> (ø)`
internal/commands/init.go	`100% <ø> (ø)`
internal/commands/render.go	`57.5% <ø> (ø)`
internal/commands/merge.go	`61.01% <ø> (ø)`
internal/commands/completion.go	`48.48% <ø> (ø)`
internal/commands/push.go	`38.6% <ø> (ø)`
internal/commands/parameters.go	`90.69% <ø> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36fd67f...30fda72. Read the comment docs.

chris-crone

Just minor changes

README.md

chris-crone · 2019-03-07T13:44:09Z

README.md


 ### Linux or macOS

+Download your OS tarball:
 ```bash
 export OSTYPE="$(uname | tr A-Z a-z)"
 curl -fsSL --output "/tmp/docker-app-${OSTYPE}.tar.gz" "https://github.com/docker/app/releases/download/v0.6.0/docker-app-${OSTYPE}.tar.gz"


Must remember to bump the version when we do a release.

chris-crone · 2019-03-07T13:45:58Z

README.md

 ```

+
 **Note:** To use Application Packages as images (i.e.: `save`, `push`, or `install` when package is not present locally) on Windows, one must be in Linux container mode.


For this PR, there is no applications as images.

(We can clean this up after Simon's push/pull PR is merged)

e2e/main_test.go

chris-crone

LGTM

Thanks @ulyssessouza and @silvin-lubecki!

simonferquel

A few nits

simonferquel · 2019-03-07T14:34:43Z

Gopkg.toml

@@ -27,9 +27,15 @@ required = ["github.com/wadey/gocovmerge"]
  name = "github.com/opencontainers/runc"
  version = "v1.0.0-rc6"

+#[[override]]


remove commented vendoring

simonferquel · 2019-03-07T14:40:55Z

e2e/commands_test.go

 	icmd.RunCmd(cmd).Assert(t, icmd.Success)
 }

 func TestBundle(t *testing.T) {
 	tmpDir := fs.NewDir(t, t.Name())
 	defer tmpDir.Remove()
 	// Using a custom DOCKER_CONFIG to store contexts in a temporary directory
-	cmd := icmd.Cmd{Env: append(os.Environ(), "DOCKER_CONFIG="+tmpDir.Path())}
+	cmd := icmd.Cmd{Env: os.Environ()}


need to patch DOCKER_CONFIG here

This has been reworked so that we use a new config dir for each test.

We can actually remove the Env: os.Environ() as it's redundant. Let's do so in a followup @ulyssessouza

Looked at the new dir for each test thing. The main problem is that it changes the environment for the whole test project, while only the child processes we invoke require these environment variables changes. Also, this dockerCli global variable makes it impossible to run tests in parallel.

internal/commands/inspect.go

internal/commands/install.go

ijc · 2019-03-07T14:41:07Z

Dockerfile

@@ -8,7 +8,10 @@ RUN apt-get install -y -q --no-install-recommends \

 WORKDIR /go/src/github.com/docker/cli

-RUN git clone https://github.com/docker/cli.git .
+RUN git clone https://github.com/ulyssessouza/cli . && git checkout 29d30ec1a05e8a285007240cc0f85864067e7344
+# FIXME(ulyssessouza): Go back to the line below when PRs https://github.com/docker/cli/pull/1710 and https://github.com/docker/cli/pull/1690 hits the cli


No need to change, but it might actually end up being docker/cli#1718 in preference to docker/cli#1710

I made this change in my rebase

ijc · 2019-03-07T14:42:25Z

Gopkg.toml

 [[override]]
  name = "github.com/docker/cli"
-  revision = "f95ca8e1ba6c22c9abcdbf65e8dcc39c53958bba"
+  source = "https://github.com/ulyssessouza/cli"
+  revision="29d30ec1a05e8a285007240cc0f85864067e7344"


Drop the first commented out one and write:

[[override]] name = "github.com/docker/cli" revision = "f95ca8e1ba6c22c9abcdbf65e8dcc39c53958bba" ### Waiting on PR https://github.com/docker/cli/pull/1710 to land on cli ### source = "https://github.com/ulyssessouza/cli" revision="29d30ec1a05e8a285007240cc0f85864067e7344"

there's no need to keep the other staza around,it's not a useful reference.

ijc · 2019-03-07T14:46:59Z

e2e/commands_test.go

+		"--description", "my cool app",
+		"--maintainer", "bob",
+		"--maintainer", "joe:joe@joe.com",
+		"--single-file",


These are due to docker/cli#1699 I think?

You didn't actually remove the short versions so there is no followup to do when that is fixed, right?

Yep, that's correct

* Use the docker/cli/cli-plugins/plugin.Run helper to create and execute the commands as a plugin * Refactor e2e tests to use “docker app” (docker cli invoking docker-app plugin) * Add CLI plugin invocation e2e tests * Bump docker-cli to include the plugin work * Bump gotest.tools to 2.3.0 Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

…can be re-used by another binary. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

This is in fact the old docker-app before its pluginization. Added it to cross compilation Now each released tar.gz file per os comes with two binaries in it: docker-app-plugin-os and docker-app-standalone-os Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

* Update windows installation too, which seems obsolete Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Perform the git clone and a chechout to avoid that a Dockerfile cache and enforce determinism Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

- Update docker/cli is now pointing to chris-crone/cli. The change needs the merge of docker/cli#1718 and docker/cli#1690 - Fix issues relative paths in Jenkinsfile and Jenkinsfile.baguette - Avoid using '--config' in favor of env variable 'DOCKER_CONFIG' Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

simonferquel · 2019-03-08T12:13:24Z

e2e/main_test.go

+		panic(err)
+	}
+	err = os.Setenv("DOCKER_CONFIG", configDir)
+	if err != nil {


We don't need to change the current process environment car but only impact the child process environment (setting this on the process level makes actually tests having huge side effects, preventing them to run in parallel in the future)

The common rule for this kind of member function of a globally shared object is that it should not have side effects on the current process shared state (e.g. should not change process-wide environement variable).

It should return 2 values:

one string for the config dir

one map[string]string usable to set child processes environment

Instead of this I've renamed the function createTestCmd, it returns an icmd.Command with the Env prefilled and a cleanup function.

simonferquel · 2019-03-08T12:33:51Z

e2e/commands_test.go

 	icmd.RunCmd(cmd).Assert(t, icmd.Success)
 }

 func TestBundle(t *testing.T) {
 	tmpDir := fs.NewDir(t, t.Name())
 	defer tmpDir.Remove()
 	// Using a custom DOCKER_CONFIG to store contexts in a temporary directory
-	cmd := icmd.Cmd{Env: append(os.Environ(), "DOCKER_CONFIG="+tmpDir.Path())}
+	cmd := icmd.Cmd{Env: os.Environ()}


Looked at the new dir for each test thing. The main problem is that it changes the environment for the whole test project, while only the child processes we invoke require these environment variables changes. Also, this dockerCli global variable makes it impossible to run tests in parallel.

internal/commands/install.go

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

simonferquel

lgtm

silvin-lubecki requested a review from chris-crone as a code owner February 26, 2019 16:49

GordonTheTurtle added the status/0-triage label Feb 26, 2019

silvin-lubecki force-pushed the cli-plugin branch from 3d5eeda to 4697791 Compare February 26, 2019 16:56

ijc mentioned this pull request Feb 27, 2019

WIP: refactor as a CLI plugin #433

Closed

ijc reviewed Feb 27, 2019

View reviewed changes

silvin-lubecki force-pushed the cli-plugin branch 2 times, most recently from 911065d to f1c7f52 Compare February 28, 2019 16:06

jcsirot force-pushed the cli-plugin branch 4 times, most recently from 48b7ac0 to 44d0886 Compare March 4, 2019 09:19

ulyssessouza force-pushed the cli-plugin branch 10 times, most recently from 97f22cf to 98407b9 Compare March 5, 2019 17:32

seemethere mentioned this pull request Mar 6, 2019

[TAR-352] Add initial basis for cli-plugins (docker/app) docker/docker-ce-packaging#309

Merged

1 task

ulyssessouza force-pushed the cli-plugin branch 3 times, most recently from a72c61e to 4b49845 Compare March 7, 2019 12:46

ulyssessouza force-pushed the cli-plugin branch 2 times, most recently from 7f7e65c to 6b31ce3 Compare March 7, 2019 13:26

ulyssessouza force-pushed the cli-plugin branch from 6b31ce3 to f5ee4ef Compare March 7, 2019 13:35

chris-crone reviewed Mar 7, 2019

View reviewed changes

ulyssessouza force-pushed the cli-plugin branch from cb6b1b4 to bae6ead Compare March 7, 2019 14:19

chris-crone approved these changes Mar 7, 2019

View reviewed changes

simonferquel suggested changes Mar 7, 2019

View reviewed changes

ijc approved these changes Mar 7, 2019

View reviewed changes

silvin-lubecki and others added 11 commits March 8, 2019 11:59

Move all the commands to their own package internal/commands so they …

2475ad2

…can be re-used by another binary. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Update README with the plugin installation

eee6fb5

* Update windows installation too, which seems obsolete Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Fix CI issues

80357fa

Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>

Remove BUILDKIT=true from Jenkinsfile

b8e5ce5

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Fix the version the of the CLI

90010ff

Perform the git clone and a chechout to avoid that a Dockerfile cache and enforce determinism Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Fix EXPERIMENTAL=on tests

9678c4b

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Make e2e test arguments uniform

c9c81eb

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Fix plugins related text in README.md

02db9b4

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

chris-crone force-pushed the cli-plugin branch from add325b to 40efdd8 Compare March 8, 2019 11:35

chris-crone requested a review from simonferquel March 8, 2019 11:56

simonferquel suggested changes Mar 8, 2019

View reviewed changes

Use unique config for each test

30fda72

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

chris-crone force-pushed the cli-plugin branch from 40efdd8 to 30fda72 Compare March 8, 2019 13:20

simonferquel approved these changes Mar 8, 2019

View reviewed changes

simonferquel merged commit 207a945 into docker:master Mar 8, 2019

silvin-lubecki deleted the cli-plugin branch March 26, 2019 15:11

		```


		Note: To use Application Packages as images (i.e.: `save`, `push`, or `install` when package is not present locally) on Windows, one must be in Linux container mode.

Transform docker-app as a docker-cli plugin #469

Transform docker-app as a docker-cli plugin #469

Conversation

silvin-lubecki commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijc commented Mar 1, 2019

ijc commented Mar 1, 2019

codecov bot commented Mar 7, 2019 • edited Loading

Codecov Report

chris-crone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-crone left a comment

Choose a reason for hiding this comment

simonferquel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonferquel left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2019 •

edited

Loading