cli-plugins: add concept of experimental plugin, only enabled in experimental mode by tiborvass · Pull Request #1898 · docker/cli

tiborvass · 2019-05-21T16:53:44Z

Closes: #1897

To test, add $(pwd)/build/plugins-linux-amd64 to "cliPluginsExtraDirs" config and run:

make plugins
make binary
HELLO_EXPERIMENTAL=1 docker helloworld

To show it enabled:

HELLO_EXPERIMENTAL=1 DOCKER_CLI_EXPERIMENTAL=enabled docker helloworld

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

cpuguy83 · 2019-05-21T17:16:48Z

Not sure I understand the plugin code specifically.
I would expect a plugin would be told if experimental mode is enabled or not and let it decide how to handle it?

tiborvass · 2019-05-21T18:05:45Z

@cpuguy83 Hmm, didn't think of that but it sounds weird to me. It'd be functionally the same though: in both cases the plugin decides whether it is experimental or not, in one case it is through a metadata field, the other would be through code.

The idea in the current PR is that if experimental is off on the cli, we do not consider experimental plugins.

codecov-io · 2019-05-21T21:10:43Z

Codecov Report

Merging #1898 into master will decrease coverage by <.01%.
The diff coverage is 35.29%.

@@ Coverage Diff @@## master #1898 +/- ##
==========================================
- Coverage 56.75% 56.74% -0.01% 
==========================================
Files 309 309 Lines 21680 21692 +12 ==========================================
+ Hits 12305 12310 +5 - Misses 8476 8482 +6 - Partials 899 900 +1

cpuguy83 · 2019-05-21T21:14:20Z

What if a plugin has some experimental functionality but is not wholly experimental?

cpuguy83 · 2019-05-21T21:15:45Z

I guess it's ok to have a plugin which is entirely experimental and not list it at all if so.

cpuguy83 · 2019-05-21T21:17:25Z

 	})

-	for _, tc := range []struct {
+	for i, tc := range []struct {


It'd be really nice to give these test cases a name and use t.Run.
Debugging with an index like this is painful.

Exactly what i was struggling with, hence the i that i wanted to remove.

tiborvass · 2019-05-21T22:24:38Z

@cpuguy83 added test names in a separate commit.

cpuguy83

Is there a way to make a negative test, i.e. experimental plugin without experimental mode?

tiborvass · 2019-05-22T02:57:40Z

@cpuguy83 it's part of the tests. The test case is experimental required.

cpuguy83 · 2019-05-22T03:14:34Z

My bad! Overlooked it.

cpuguy83

LGTM

silvin-lubecki · 2019-05-22T08:27:53Z

 // non-recoverable error.
-func newPlugin(c Candidate, rootcmd *cobra.Command) (Plugin, error) {
+//
+// nolint: gocyclo


Can be done in a follow-up, but I think this function should be split into pieces as it reached the go-cyclo threshold.

ijc · 2019-05-22T08:32:11Z

Actual code changes look fine to me. I'd suggest adding some variants to the tests under e2e/cli-plugins/ to ensure that the experimental plugins appear in the right places at the right time wrt the various variations on docker helpdocker --help, in particular I think we want to avoid having to have people remember to rerun the test suit with the envvar set and the help/--help stuff has been tricky to get right.

Or maybe rather than just doubling ~every test in that suite just add another stunt plugin as e2e/cli-plugins/experimental (using e2e/cli-plugins/plugins/nopersistentprerun as the ~simplest template rather than the hello one) and then update the various golden files to accommodate that showing up in the various places it will show up. That will mean splitting some of the *.golden files into two versions and conditionally adding -experimental to the filename in the golden.Assert() calls (perhaps best done with a helper/wrapper?)

Even with the stunt plugin I think you want a new case which does docker experimental help and docker help experimental (given the stunt plugin is docker-experimental) since that presumably needs to DTRT...

silvin-lubecki

LGTM with small comments fixed 👍

silvin-lubecki · 2019-05-22T08:34:38Z

Linter is complaining

cli-plugins/manager/candidate_test.go:76:157:warning: unknown field allowExperimental in struct literal (gosimple)
cli/command/formatter/reflect_test.go:15:34:warning: nolint directive did not match any issue (nolint)
cli-plugins/manager/candidate_test.go:76:157:warning: unknown field allowExperimental in struct literal (unused)

silvin-lubecki · 2019-05-22T08:37:32Z

Some tests are failing:

# github.com/docker/cli/cli-plugins/manager [github.com/docker/cli/cli-plugins/manager.test]
cli-plugins/manager/candidate_test.go:76:157: unknown field 'allowExperimental' in struct literal of type struct { name string; c *fakeCandidate; err string; invalid string }
cli-plugins/manager/candidate_test.go:77:127: unknown field 'allowExperimental' in struct literal of type struct { name string; c *fakeCandidate; err string; invalid string }

tiborvass · 2019-05-22T14:35:15Z

@silvin-lubecki fixed. @ijc thanks for the pointers, I'll do that in a follow up.

…rimental mode To test, add $(pwd)/build/plugins-linux-amd64 to "cliPluginsExtraDirs" config and run: make plugins make binary HELLO_EXPERIMENTAL=1 docker helloworld To show it enabled: HELLO_EXPERIMENTAL=1 DOCKER_CLI_EXPERIMENTAL=enabled docker helloworld Signed-off-by: Tibor Vass <tibor@docker.com>

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

silvin-lubecki · 2019-05-22T15:52:25Z

ping @thaJeztah

tonistiigi · 2019-05-22T15:58:26Z

To me, it makes much more sense if there is a separate experimental directory (or a naming scheme) for experimental plugins instead of a metadata command.

Plugins have their own versioning/revisions that clearly shows in the output. Eg. I have no desire to mark buildx experimental. If you build it or pull a binary from github there is no need to punish the user so that they need to get all the other experimental things as well and use environment the few other users are using. Adding a plugin does not make the rest of the docker experience more unstable and having buildx with its 0.x version needing to modify the configuration while a random hello-world plugin does not, does not make sense. In practice, this means that people would just start to call buildx directly and not install it as a plugin at all, leading to fragmentation.

Things are different when a plugin is deployed together with Docker CE/EE suites and has a different compatibility guarantees. Then it does makes sense to not enable them automatically. And for that these suites should install the plugins in a way that only enables them when the global experimental mode is on. Eg. by putting them in a separate directory where cli looks for them or naming them docker-buildx-experimental.

andrewhsu · 2019-05-22T19:35:34Z

Talked with @tonistiigi about his #1898 (comment) and i've updated the issue proposal for change request with details on the implementation #1897 (comment) to accommodate for a user installing a plugin explicitly. cc @tiborvass

tiborvass · 2019-05-23T01:23:09Z

I opened #1902.

tiborvass · 2019-05-23T17:55:40Z

I think @tonistiigi's points are valid and we should still consider the alternative I opened in #1902, but it turns out it requires more work on Desktop and it's blocking an RC release, so I'm advocating for merging this to unblock.

andrewhsu

LGTM

thaJeztah

LGTM

GordonTheTurtle added the status/0-triage label May 21, 2019

tiborvass force-pushed the plugin_experimental branch from d0c799b to fd85a6fCompare May 21, 2019 16:57

tiborvass requested review from ijc, silvin-lubecki and thaJeztah May 21, 2019 16:57

seemethere approved these changes May 21, 2019
View reviewed changes

tiborvass force-pushed the plugin_experimental branch from fd85a6f to c483c63Compare May 21, 2019 21:04

cpuguy83 reviewed May 21, 2019
View reviewed changes

tiborvass force-pushed the plugin_experimental branch from c483c63 to 10b899bCompare May 21, 2019 22:14

cpuguy83 reviewed May 21, 2019
View reviewed changes

Comment threadcli-plugins/manager/candidate_test.go Outdated

tiborvass force-pushed the plugin_experimental branch from caba26d to 8711e0fCompare May 22, 2019 00:22

cpuguy83 reviewed May 22, 2019
View reviewed changes

cpuguy83 approved these changes May 22, 2019
View reviewed changes

tiborvass force-pushed the plugin_experimental branch from 8711e0f to 0155e5bCompare May 22, 2019 07:59

silvin-lubecki reviewed May 22, 2019
View reviewed changes

Comment threadcli-plugins/manager/metadata.go Outdated

silvin-lubecki approved these changes May 22, 2019
View reviewed changes

Comment threadcli-plugins/manager/manager.go Outdated

silvin-lubecki mentioned this pull request May 22, 2019
Mark docker app plugin as experimental docker-archive-public/docker.app#537
Merged

tiborvass force-pushed the plugin_experimental branch from 0155e5b to 49c6dbeCompare May 22, 2019 14:34

Tibor Vass added 2 commits May 22, 2019 15:35

cli-plugins: add test names for easier debugging
bb8e89b
Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the plugin_experimental branch from 49c6dbe to bb8e89bCompare May 22, 2019 15:37

andrewhsu mentioned this pull request May 22, 2019
docker cli experimental flag for plugins #1897
Closed

tiborvass mentioned this pull request May 22, 2019
[WIP] cli-plugins: look for plugins in experimental folder when experimental is enabled #1902
Closed

tiborvass closed this May 23, 2019

tiborvass reopened this May 23, 2019

andrewhsu approved these changes May 23, 2019
View reviewed changes

thaJeztah added area/plugins kind/experimental status/2-code-review and removed status/0-triage labels May 23, 2019

thaJeztah approved these changes May 23, 2019
View reviewed changes

thaJeztah merged commit ab688a9 into docker:masterMay 23, 2019

GordonTheTurtle added this to the 19.09.0 milestone May 23, 2019

thaJeztah mentioned this pull request May 23, 2019
[19.03 backport] cli-plugins: add concept of experimental plugin, only enabled in experimental mode #1905
Merged

tiborvass mentioned this pull request May 23, 2019
Allow Docker to ship buildx guarded by the experimental flag docker/buildx#81
Merged

Uh oh!

Conversation

tiborvass commented May 21, 2019• edited by andrewhsu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented May 21, 2019

Uh oh!

tiborvass commented May 21, 2019

Uh oh!

codecov-io commented May 21, 2019

Codecov Report

Uh oh!

cpuguy83 commented May 21, 2019

Uh oh!

cpuguy83 commented May 21, 2019

Uh oh!

cpuguy83May 21, 2019

Choose a reason for hiding this comment

Uh oh!

tiborvassMay 21, 2019

Choose a reason for hiding this comment

Uh oh!

tiborvass commented May 21, 2019

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

tiborvass commented May 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented May 22, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubeckiMay 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ijc commented May 22, 2019

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

silvin-lubecki commented May 22, 2019

Uh oh!

silvin-lubecki commented May 22, 2019

Uh oh!

tiborvass commented May 22, 2019

Uh oh!

silvin-lubecki commented May 22, 2019

Uh oh!

tonistiigi commented May 22, 2019

Uh oh!

andrewhsu commented May 22, 2019

Uh oh!

tiborvass commented May 23, 2019

Uh oh!

tiborvass commented May 23, 2019

Uh oh!

andrewhsu left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tiborvass commented May 21, 2019•
edited by andrewhsu
Loading

tiborvass commented May 22, 2019•
edited
Loading