New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for experimental Cli configuration #758

Merged
merged 1 commit into from Dec 22, 2017

Conversation

@vdemeester
Member

vdemeester commented Dec 20, 2017

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)

jpeg image 271 x 186 pixels

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

Show outdated Hide outdated cli/command/cli.go Outdated
Show outdated Hide outdated cli/command/cli.go Outdated
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 20, 2017

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

codecov-io commented Dec 20, 2017

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.

Show outdated Hide outdated cli/command/cli.go Outdated
Show outdated Hide outdated cli/command/cli.go Outdated
@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Dec 20, 2017

Member

@thaJeztah Interesting, I like that approach 👼

Member

vdemeester commented Dec 20, 2017

@thaJeztah Interesting, I like that approach 👼

@dnephin

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

Show outdated Hide outdated cli/command/cli_test.go Outdated
Show outdated Hide outdated cli/command/cli_test.go Outdated
Show outdated Hide outdated cli/command/cli.go Outdated
Show outdated Hide outdated cmd/docker/docker.go Outdated
Show outdated Hide outdated cmd/docker/docker.go Outdated
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Dec 20, 2017

Collaborator

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).

Collaborator

dnephin commented Dec 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 20, 2017

Member

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)

Member

thaJeztah commented Dec 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Dec 20, 2017

Collaborator

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.

Collaborator

dnephin commented Dec 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Dec 21, 2017

Member

@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 😉

Member

vdemeester commented Dec 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 21, 2017

Member

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"

Member

thaJeztah commented Dec 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Dec 21, 2017

Member

@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 👼

Member

vdemeester commented Dec 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 21, 2017

Member

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"

Member

thaJeztah commented Dec 21, 2017

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"

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

This comment has been minimized.

@thaJeztah

thaJeztah Dec 21, 2017

Member

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

thaJeztah Dec 21, 2017

Member

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)

Show outdated Hide outdated cli/config/configfile/file.go Outdated
Show outdated Hide outdated cli/command/cli.go Outdated
@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester
Member

vdemeester commented Dec 22, 2017

Show outdated Hide outdated cli/command/cli_test.go Outdated
@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Dec 22, 2017

Contributor

LGTM

Contributor

tiborvass commented Dec 22, 2017

LGTM

Add support for experimental Cli configuration
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!

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

7 of 8 checks passed

codecov/patch 41.02% of diff hit (target 50%)
Details
ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/project 53.46% (+0.01%) compared to afcc78a
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 18.01.0 milestone Dec 22, 2017

@vdemeester vdemeester deleted the vdemeester:experimental-cli branch Dec 22, 2017

@bitsofinfo

This comment has been minimized.

Show comment
Hide comment
@bitsofinfo

bitsofinfo commented Jan 2, 2018

thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment