Add support for experimental Cli configuration by vdemeester · Pull Request #758 · docker/cli

vdemeester · 2017-12-20T14:10:13Z

Allow to mark some commands and flags experimental on cli (i.e. not
depending to the state of the daemon). This will allow more flexibility
on experimentation with the cli.

Marking docker trust as cli experimental as it is documented so.

$ docker trust --help
docker trust is only supported on a Docker cli with experimental features enabled
# edit docker client configuration file
$ cat ~/.docker/config.json
{
        "experimental": "enabled"
}
$ docker trust --help
Usage:  docker trust COMMAND

Manage trust on Docker images (experimental)

Options:


Management Commands:
  key         Manage keys for signing Docker images (experimental)
  signer      Manage entities who can sign Docker images (experimental)

Commands:
  revoke      Remove trust for an image
  sign        Sign an image
  view        Display detailed information about keys and signatures

Run 'docker trust COMMAND --help' for more information on a command.

This is required for #721 (as we will put the k8s support under experimental at first)

Signed-off-by: Vincent Demeester vincent@sbr.pm

AkihiroSuda · 2017-12-20T14:12:37Z

cli/command/cli.go

+func isExperimentalCli(configfile *configfile.ConfigFile) bool {
+	if e := os.Getenv("DOCKER_EXPERIMENTAL_CLI"); e != "" {
+		value := strings.ToLower(e)
+		return value == "1" || value == "y" || value == "yes"


no support for "true"?

Probably strconv.ParseBool should be used

Thx @AkihiroSuda 🤗

silvin-lubecki · 2017-12-20T14:32:42Z

cli/command/cli.go

 }

+func isExperimentalCli(configfile *configfile.ConfigFile) bool {
+	if e := os.Getenv("DOCKER_EXPERIMENTAL_CLI"); e != "" {


I don't think this environment variable is useful. Contrary to the DOCKER_ORCHESTRATOR one which you can use to switch between orchestrators, I think switching to experimental features should be a "long term" choice from the user.

Agreed, we can probably skip it for now. Easy to add in future if there's a real need for it.

codecov-io · 2017-12-20T14:39:36Z

Codecov Report

Merging #758 into master will increase coverage by 0.01%.
The diff coverage is 41.02%.

@@            Coverage Diff             @@
##           master     #758      +/-   ##
==========================================
+ Coverage   53.45%   53.46%   +0.01%     
==========================================
  Files         218      218              
  Lines       14613    14642      +29     
==========================================
+ Hits         7811     7829      +18     
- Misses       6321     6327       +6     
- Partials      481      486       +5

thaJeztah

From a user-perspective, I wonder if we should have some connection between "server experimental" and "client experimental".

Thinking something like;

auto (default) CLI follows daemon's "experimental"
disabled Disable all experimental features; hide experimental commands, even if they are enabled on the daemon
enabled Enable experimental (cli) commands; experimental features that require the daemon to have experimental are still disabled

It's a bit tricky if people want to be able to enable/disable them separate (without having access to the daemon), in which case it'd become;

auto | disabled | enabled | cli-only | daemon-only ? (most likely too detailed)

We'll need some documentation around this, because for users it will not be clear which features are client-side, and which ones are daemon-side.

thaJeztah · 2017-12-20T14:55:53Z

cli/command/cli.go

 		return err
 	}
+	cli.clientInfo = ClientInfo{
+		HasExperimental: isExperimentalCli(cli.configFile),


We probably need to add this information to the docker version output as well;

cli/cli/command/system/version.go

Lines 61 to 72 in 7138d6e

type clientVersion struct {

Platform struct{ Name string } `json:",omitempty"`

Version string

APIVersion string `json:"ApiVersion"`

DefaultAPIVersion string `json:"DefaultAPIVersion,omitempty"`

GitCommit string

GoVersion string

Os string

Arch string

BuildTime string `json:",omitempty"`

}

thaJeztah · 2017-12-20T14:57:01Z

cli/command/cli.go

 }

+func isExperimentalCli(configfile *configfile.ConfigFile) bool {
+	if e := os.Getenv("DOCKER_EXPERIMENTAL_CLI"); e != "" {


Agreed, we can probably skip it for now. Easy to add in future if there's a real need for it.

vdemeester · 2017-12-20T15:28:41Z

@thaJeztah Interesting, I like that approach 👼

dnephin

I thought we were considering calling this "beta" instead of experimental. What happened with that discussion?

dnephin · 2017-12-20T19:57:07Z

cli/command/cli_test.go

+			}
+
+			cli := &DockerCli{client: apiclient, err: os.Stderr}
+			t.Log("yo ", tmpDir)


dnephin · 2017-12-20T19:59:27Z

cli/command/cli_test.go

+	}
+}
+
+func createTemporaryConfigfile(t *testing.T, name, content string) (string, func()) {


Could be replaced with

dir := fs.NewDir(t, name, fs.WithFile("config.json", content)) defer dir.Remove()

dnephin · 2017-12-20T20:00:46Z

cli/command/cli.go

 	defaultVersion string
-	server         ServerInfo
+	serverInfo     ServerInfo
+	clientInfo     ClientInfo


We could move defaultVersion to clientInfo I think, but doesn't need to happen in this PR.

dnephin · 2017-12-20T20:03:32Z

cmd/docker/docker.go

 				errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker daemon with experimental features enabled", f.Name))
 			}
+			if _, ok := f.Annotations["experimentalCli"]; ok && !hasExperimentalCli {
+				errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with experimental features enabled", f.Name))


"only supported when experimental cli features are enabled"

I just realized all of this was duplicated. We should be able to declare this visitor once, and call it for the root command case. Doesn't need to be part of this PR.

@dnephin yep, we realized that too in #721 just didn't want to add more code to it 😛

dnephin · 2017-12-20T20:04:00Z

cmd/docker/docker.go

 			return fmt.Errorf("%s is only supported on a Docker daemon with experimental features enabled", cmd.CommandPath())
 		}
+		if _, ok := curr.Annotations["experimentalCli"]; ok && !hasExperimentalCli {
+			return fmt.Errorf("%s is only supported on a Docker cli with experimental features enabled", cmd.CommandPath())


"only supported when experimental cli features are enabled"

dnephin · 2017-12-20T20:07:23Z

From a user-perspective, I wonder if we should have some connection between "server experimental" and "client experimental".

I think this implies there is some relationship between the two, when in reality there is not. I think it would be better to keep them separate. Even using different names for them would be great (beta vs experimental).

thaJeztah · 2017-12-20T20:34:47Z

The tricky bit is that something being "client side" or "daemon side" should largely be an implementation detail; most users won't be aware where the functionality lives, and I'm not sure we should put that burden onto them if not needed for the 99% (but we can still consider having more fine-grained options in addition).

Some features may even move from cli to daemon (or vice versa)

dnephin · 2017-12-20T21:00:02Z

I think the reality is that the user needs to be aware of where it's implemented because a single client can connect to more than one server.

We could maybe avoid some of the confusion by using feature flags instead of a global "experimental". So users could enable trust, or k8s-stacks, instead of enabling everything.

vdemeester · 2017-12-21T08:53:25Z

@thaJeztah @dnephin Updated with comments taken into account. Switch to enabled and disabled as value so we could, in the future, have more options for it 😉

thaJeztah · 2017-12-21T09:30:03Z

enabled|disabled LGTM, but still would love to see auto, as it feels natural to me to (if no configuration is set) have the CLI follow the daemon (and not require people to set the "experimental" option in two locations)

because a single client can connect to more than one server.

Different topic, but we could really use a way to set multiple configurations (per daemon/host), either by having a nodes: [{}] option in the configuration file, or (likely more flexible) having subdirectories with configuration files per-host (a-la docker-machine). We have --config already, but I can see there's configurations that are "global", and ones that are "per host"

vdemeester · 2017-12-21T09:37:00Z

@thaJeztah could we add auto on a follow-up PR (to discuss it) ? 👼

Different topic, but we could really use a way to set multiple configurations (per daemon/host), either by having a nodes: [{}] option in the configuration file, or (likely more flexible) having subdirectories with configuration files per-host (a-la docker-machine).

Yes ! This is in my TODOs 👼

thaJeztah · 2017-12-21T09:42:24Z

could we add auto on a follow-up PR (to discuss it) ?

Yes, definitely; current implementation allows expanding options (hence: not using a boolean true/false)

LGTM, but awaiting @dnephin before moving to "code review"

thaJeztah · 2017-12-21T09:47:14Z

cli/command/cli.go

+	case "", "disabled":
+		return false, nil
+	default:
+		return false, errors.Errorf("%q is not valid, should be either enabled or disabled", value)


Think we should mention the configuration option here, currently it shows:

$ docker version "Moby was here" is not valid, should be either enabled or disabled

Perhaps something like:

Invalid value for "experimentalCli": "Moby was here". Valid options are "enabled" and "disabled"

(better suggestions welcome)

thaJeztah · 2017-12-21T09:50:07Z

cli/config/configfile/file.go

 	NodesFormat          string                      `json:"nodesFormat,omitempty"`
 	PruneFilters         []string                    `json:"pruneFilters,omitempty"`
 	Proxies              map[string]ProxyConfig      `json:"proxies,omitempty"`
+	ExperimentalCli      string                      `json:"experimentalCli,omitempty"`


CLI is an acronym, so this should be ExperimentalCLI (and experimentalCLI for JSON)

Given that this configuration file is for the CLI; is CLI a bit redundant? Perhaps just Experimental?

dnephin

design LGTM

dnephin · 2017-12-21T18:32:38Z

cli/command/cli.go

 	}
+	hasExperimental, err := isEnabled(cli.configFile.ExperimentalCli)
+	if err != nil {
+		return err


I guess this should errors.Wrapf(err, "ExperimentalCLI field") so that the error message mentions which field has a problem

vdemeester · 2017-12-22T12:53:34Z

cc @AkihiroSuda @yongtang

tiborvass · 2017-12-22T13:44:12Z

cli/command/cli_test.go

 	}
 }

+func TestExperimentalCli(t *testing.T) {


small nit: TestExperimentalCLI to stay consistent with casing

tiborvass · 2017-12-22T13:45:14Z

LGTM

Allow to mark some commands and flags experimental on cli (i.e. not depending to the state of the daemon). This will allow more flexibility on experimentation with the cli. Marking `docker trust` as cli experimental as it is documented so. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

thaJeztah

LGTM, thanks!

bitsofinfo · 2018-01-02T21:41:28Z

thx

Add support for experimental Cli configuration Upstream-commit: 70db7cc Component: cli

vdemeester requested review from dnephin, thaJeztah and tiborvass December 20, 2017 14:10

GordonTheTurtle added the status/0-triage label Dec 20, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Dec 20, 2017

AkihiroSuda reviewed Dec 20, 2017

View reviewed changes

vdemeester force-pushed the experimental-cli branch from 43d8af1 to 42186c2 Compare December 20, 2017 14:16

silvin-lubecki reviewed Dec 20, 2017

View reviewed changes

vdemeester force-pushed the experimental-cli branch from 42186c2 to f821c37 Compare December 20, 2017 14:36

thaJeztah reviewed Dec 20, 2017

View reviewed changes

vdemeester force-pushed the experimental-cli branch from f821c37 to b9fd0aa Compare December 20, 2017 15:27

dnephin reviewed Dec 20, 2017

View reviewed changes

vdemeester force-pushed the experimental-cli branch from b9fd0aa to 1bd2b5f Compare December 21, 2017 08:52

silvin-lubecki mentioned this pull request Dec 21, 2017

Add kubernetes support to docker/cli #721

Merged

vdemeester force-pushed the experimental-cli branch from 1bd2b5f to fe81004 Compare December 21, 2017 09:25

thaJeztah reviewed Dec 21, 2017

View reviewed changes

vdemeester force-pushed the experimental-cli branch from fe81004 to da6a6e9 Compare December 21, 2017 16:29

dnephin reviewed Dec 21, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Dec 21, 2017

vdemeester force-pushed the experimental-cli branch from da6a6e9 to 19a3689 Compare December 22, 2017 12:52

vdemeester force-pushed the experimental-cli branch 2 times, most recently from deae842 to bae6e20 Compare December 22, 2017 13:33

tiborvass reviewed Dec 22, 2017

View reviewed changes

vdemeester force-pushed the experimental-cli branch from bae6e20 to 84fe1a1 Compare December 22, 2017 13:48

thaJeztah approved these changes Dec 22, 2017

View reviewed changes

thaJeztah merged commit 70db7cc into docker:master Dec 22, 2017

GordonTheTurtle added this to the 18.01.0 milestone Dec 22, 2017

vdemeester deleted the experimental-cli branch December 22, 2017 13:59

kvncrw mentioned this pull request Jan 6, 2018

[RFC] CLI Experimental Flag should resemble the interface of its daemon #790

Closed

ollypom mentioned this pull request Jan 11, 2018

Experimental CLI Configuration docker/docs#5718

Closed

thaJeztah added docs/revisit kind/experimental labels Jun 4, 2019

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#758 from vdemeester/experimental-cli

2ac4f6c

Add support for experimental Cli configuration Upstream-commit: 70db7cc Component: cli

	type clientVersion struct {
	Platform struct{ Name string } `json:",omitempty"`

	Version string
	APIVersion string `json:"ApiVersion"`
	DefaultAPIVersion string `json:"DefaultAPIVersion,omitempty"`
	GitCommit string
	GoVersion string
	Os string
	Arch string
	BuildTime string `json:",omitempty"`
	}

Comments

Conversation

vdemeester commented Dec 20, 2017 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Dec 20, 2017

Uh oh!

thaJeztah commented Dec 20, 2017

Uh oh!

dnephin commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented Dec 21, 2017

Uh oh!

thaJeztah commented Dec 21, 2017

Uh oh!

vdemeester commented Dec 21, 2017

Uh oh!

thaJeztah commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Dec 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Dec 22, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

bitsofinfo commented Jan 2, 2018

vdemeester commented Dec 20, 2017 •

edited by thaJeztah

Loading

codecov-io commented Dec 20, 2017 •

edited

Loading

vdemeester commented Dec 20, 2017 •

edited

Loading

dnephin commented Dec 20, 2017 •

edited

Loading