add --from flag to context create #1773

zappy-shu · 2019-03-22T15:58:24Z

- What I did

Added --from option to context create command that simplifies creation of a context from a named
or the current context.

If --from is set then the context will be created using the existing context with the provided name. If --from, --docker, and --kubernetes is not set then the context will be created using the current context.

- How I did it
Uses the context store to export the current context, then updates the details (description and orchestrator), and uses the context store to import the new context.

- How to verify it
Tests have been added to check the new functionality and ensure the user cannot use the --from-current flag with other docker/kubernetes settings

codecov-io · 2019-03-22T16:12:34Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5bbb56b). Click here to learn what that means.
The diff coverage is 64.81%.

@@            Coverage Diff            @@
##             master    #1773   +/-   ##
=========================================
  Coverage          ?   56.28%           
=========================================
  Files             ?      308           
  Lines             ?    21323           
  Branches          ?        0           
=========================================
  Hits              ?    12001           
  Misses            ?     8445           
  Partials          ?      877

simonferquel · 2019-03-25T09:25:51Z

ping @dhiltgen

cli/command/context/create.go

silvin-lubecki · 2019-03-25T10:26:39Z

cli/command/context/create.go

@@ -63,6 +64,7 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
 		"Default orchestrator for stack operations to use with this context (swarm|kubernetes|all)")
 	flags.StringToStringVar(&opts.Docker, "docker", nil, "set the docker endpoint")
 	flags.StringToStringVar(&opts.Kubernetes, "kubernetes", nil, "set the kubernetes endpoint")
+	flags.BoolVar(&opts.FromCurrent, "from-current", false, "get docker endpoint from current configuration")


I think from-current flag is misleading, we don't know from what it is: from current context, from current environment? Reading the code it's clear it's from the current context, so I think we should be explicit here and rename it to from-current-context.
The description should also be amended with something more explicit like: create a new context from the current context

In context (no pun intended) it'll be docker context --from-current. With the proposed change it'll be docker context --from-current-context which seems stuttery to me. Is the current naming not clear enough when you see it used in context?

When rewording the help we should be careful not to obscure too much the fact that if your current context is the automatic one derived from envvars then that is what you will be importing -- that's the 80% use case for this flag I think (otherwise we'd call this context copy --from=xxx or context create --from=xxx etc I think).

We already have a "flag" (to be fair it's a CSV key) which is from-current, but creates a context with docker configuration from the current environment. Same for kubernetes.
That's a lot of from-current here, with different semantic. That's why I think we should be explicit here.

After off-line discussion with @ijc, I agree with docker context create --from=my-current-context 👍
@thaJeztah any thought about that?

from-current either as a flag or as the csv config key, always refers to the current context (being a real one, or default. It just applies at a different scope (e.g. either copy both, or only one of the endpoints as is).
I agree that naming might be confusing though. We should make sure it is clean before 19.03 is out, but I think we should keep both the flag and the setting names consistent.

I'm good with this 👍

My question would be what is the state of --kubernetes in the final case -- is it blank or is it inherited from the current context. IOW does passing any single option completely disable the copying behaviour of the first case for all options?

maybe if --docker is not set and --kubernetes is then it acts as an alias for --docker from-current=true --kubernetes blah=blah

@ijc I guess it is a matter of opinion. My guess is if you want a custom endpoint configuration, you are in fully customized way, but that does not disallow things like:
docker context create my-ctx --docker host=tcp://42.42.42.42:42 --kubernetes from=default ...

I wasn't expressing (and don't really have) an opinion, but we should make a decision for one or the other.

silvin-lubecki · 2019-03-27T14:30:39Z

@zappy-shu there is a linter error:

cli/command/context/create.go:72::warning: cyclomatic complexity 19 of function RunCreate() is high (> 16) (gocyclo)

silvin-lubecki

LGTM 👍

simonferquel · 2019-03-27T17:30:24Z

Reference documentation must be updated then :)
Can you deal with that @zappy-shu ? (I can give you pointers tomorrow on where to look at)

simonferquel · 2019-03-28T08:57:50Z

For symmetry, we need to remove from-current config key in docker and kubernetes endpoints, with a from config key accepting a context name (which can be default)

thaJeztah · 2019-03-28T09:46:34Z

For symmetry, we need to remove from-current config key in docker and kubernetes endpoints, with a from config key accepting a context name (which can be default)

I agree, I think the functionality overlaps

Would this be a common scenario? As this is already possible (and would be symmetrical with, e.g. docker image save hello-world | docker image load)

docker context export default - | docker context import mynewcontext -

thaJeztah · 2019-03-28T09:50:32Z

(Was also thinking of a docker context copy subcommand)

simonferquel · 2019-03-28T09:59:49Z

@thaJeztah it is actually how --from is implemented. It allows to override original description / default stack orchestrator though.
Export/Import + Update would probably do the same, but the idea here is to polish the UI for common context creation tasks.
Our nominal scenario is when working with a UCP bundle:
We can easily do something like

$ eval "$(<env.sh)" # or env.ps1
$ docker context create ucp

and be done with it.

thaJeztah · 2019-03-28T09:47:55Z

cli/command/context/create.go

@@ -63,6 +64,7 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
 		"Default orchestrator for stack operations to use with this context (swarm|kubernetes|all)")
 	flags.StringToStringVar(&opts.Docker, "docker", nil, "set the docker endpoint")
 	flags.StringToStringVar(&opts.Kubernetes, "kubernetes", nil, "set the kubernetes endpoint")
+	flags.StringVar(&opts.From, "from", "", "create context from a named context")


Perhaps the flag description should mention what the default is

The default depends on whether you use --docker/--kubernetes or not. With then it will use the options given to those arguments, without it will create from the current context. Seems a bit finicky to me I'll admit.
To make it less so we could remove the defaulting entirely but then creating from the current/environment is a bit more of a pain.

thaJeztah · 2019-03-28T09:49:55Z

cli/command/context/create.go

+
+func createFromExistingContext(s store.Store, fromContextName string, stackOrchestrator command.Orchestrator, o *CreateOptions) error {
+	if len(o.Docker) != 0 || len(o.Kubernetes) != 0 {
+		return errors.New("cannot use --docker or --kubernetes flags when --from is set")


Do we allow --default-stack-orchestrator and --description ? Perhaps that's a bit inconsistent (i.e., allowing some options to be overridden when creating --from, but not others)

Being able to set the description makes sense to me as it is a new context. The default stack orchestrator I don't know much about. From what I understand the main purpose of --from is to make it easier to copy the connection details for docker/kubernetes from a context or environment variables

thaJeztah · 2019-03-28T09:58:17Z

docs/reference/commandline/context_create.md

@@ -72,4 +73,14 @@ $ export KUBECONFIG=/path/to/my/kubeconfig
 $ docker context create my-context --kubernetes "from-current=true" --docker "host=/var/run/docker.sock"
 ```

+By default the `context` will be created from the current context. This will include both docker and, if set, kubernetes endpoint configuration. This is equivalent to `--docker "from-current=true"` or, if the current context has a kubernetes contfiguration, `--docker "from-current=true" --kubernetes "from-current=true"`:


Can you wrap this to ~80 chars? Also try to avoid future tense;

Suggested change

By default the `context` will be created from the current context. This will include both docker and, if set, kubernetes endpoint configuration. This is equivalent to `--docker "from-current=true"` or, if the current context has a kubernetes contfiguration, `--docker "from-current=true" --kubernetes "from-current=true"`:

By default the `context` is created from the current context. This includes

both `docker` and, if set, `kubernetes` endpoint configuration. This is

equivalent to `--docker "from-current=true"` or, if the current context has

a kubernetes contfiguration, `--docker "from-current=true" --kubernetes

"from-current=true"`:

If we agree on removing the from-current;

Suggested change

By default the `context` will be created from the current context. This will include both docker and, if set, kubernetes endpoint configuration. This is equivalent to `--docker "from-current=true"` or, if the current context has a kubernetes contfiguration, `--docker "from-current=true" --kubernetes "from-current=true"`:

By default the `context` is created from the current context. This includes

both `docker` and, if set, `kubernetes` endpoint configuration:

Or (explaining what "by default" means);

Suggested change

By default the `context` will be created from the current context. This will include both docker and, if set, kubernetes endpoint configuration. This is equivalent to `--docker "from-current=true"` or, if the current context has a kubernetes contfiguration, `--docker "from-current=true" --kubernetes "from-current=true"`:

If the `--from` option is not set, the `context` is created from the current

context, and copies the `docker` and, if set, `kubernetes` endpoint configuration:

Wondering though it the part about docker and kubernetes is a bit redundant (and if just mentioning that it's copied from the current context is sufficient?)

docs/reference/commandline/context_create.md

thaJeztah · 2019-03-28T10:02:11Z

docs/reference/commandline/context_create.md

+$ docker context create my-context
+```
+
+You can select a different `context` to create from by using the `--from` option:


Suggested change

You can select a different `context` to create from by using the `--from` option:

Use the `--from=<context name>` option to create a new context from an

existing context. The example below creates a new context named

`my-context` from the existing `existing-context` context.

thaJeztah · 2019-03-28T10:26:29Z

Our nominal scenario is when working with a UCP bundle

That feels like a band-aid solution; can we include an actual context in the UCP bundle, so that we can docker context import my-ucp-context ./ucp-context.context ?

simonferquel · 2019-03-28T10:41:12Z

@thaJeztah we will in the next release. But users that don't upgrade their clusters - and whose bundles don't have the dockercontext file in it - should have something better that what we have now.

thaJeztah · 2019-03-28T14:34:39Z

But users that don't upgrade their clusters - and whose bundles don't have the dockercontext file in it - should have something better that what we have now.

Well, if they don't update they would still use an old client 😉, but I get the idea; as long as we make sure to document the docker context import as the recommended/standard approach

thaJeztah · 2019-03-28T14:35:23Z

Also wondering if we can backport the "include context in UCP bundle" to older UCP releases (as part of a patch release)

simonferquel · 2019-03-28T14:50:28Z

Not likely because of golang vendoring hell (vendoring a new CLI requires vendoring a new docker engine... which is not possible for patch releases in UCP - except bumping a patch release of the currently supported engine)

--from creates a context from a named context. By default `context create` will create a context from the current context. Replaced "from-current=" docker/kubernetes option with "from=" to allow specifying which context to copy the settings from. Signed-off-by: Nick Adcock <nick.adcock@docker.com>

thaJeztah · 2019-04-02T14:51:34Z

docs/reference/commandline/context_create.md

@@ -23,7 +23,7 @@ Create a context
 Docker endpoint config:

 NAME                DESCRIPTION
-from-current        Copy current Docker endpoint configuration
+from                Copy Docker endpoint configuration from an existing context


Do we still want the from for docker and kubernetes, now that we have the top-level --from? Basically; wondering if we want context create and context update to allow mish-mashing two unrelated contexts.

I can slightly see this being something when creating a new context, but not entirely sure for updating an existing one.

I think it is not about "usefulness" but more about symetry: endpoint configuration options would be easier to learn if they are the same for both create and update.

thaJeztah

LGTM

thaJeztah · 2019-04-09T14:37:44Z

/cc @nigelpoulton

thaJeztah · 2019-04-09T14:39:48Z

oh, and /cc @albers (for completion scripts; forgot that those might need updating)

GordonTheTurtle added the status/0-triage label Mar 22, 2019

zappy-shu force-pushed the create-context-from-current branch from 9bd25c8 to a1b6398 Compare March 22, 2019 16:12

silvin-lubecki reviewed Mar 25, 2019

View reviewed changes

cli/command/context/create.go Outdated Show resolved Hide resolved

zappy-shu force-pushed the create-context-from-current branch from a1b6398 to 62b46d6 Compare March 25, 2019 09:56

silvin-lubecki requested changes Mar 25, 2019

View reviewed changes

zappy-shu force-pushed the create-context-from-current branch from 62b46d6 to 1e01375 Compare March 26, 2019 14:53

ijc approved these changes Mar 27, 2019

View reviewed changes

zappy-shu force-pushed the create-context-from-current branch 2 times, most recently from a962b05 to 0cb1a47 Compare March 27, 2019 15:10

silvin-lubecki approved these changes Mar 27, 2019

View reviewed changes

zappy-shu requested a review from thaJeztah as a code owner March 28, 2019 08:37

zappy-shu changed the title ~~add --from-current flag to context create~~ add --from flag to context create Mar 28, 2019

zappy-shu changed the title ~~add --from flag to context create~~ WIP add --from flag to context create Mar 28, 2019

thaJeztah requested changes Mar 28, 2019

View reviewed changes

thaJeztah added status/1-design-review and removed status/0-triage labels Mar 28, 2019

zappy-shu force-pushed the create-context-from-current branch from cbe62a9 to 1e0dce0 Compare March 28, 2019 15:16

zappy-shu changed the title ~~WIP add --from flag to context create~~ add --from flag to context create Mar 28, 2019

zappy-shu force-pushed the create-context-from-current branch from 1e0dce0 to 8bb152d Compare April 2, 2019 12:42

thaJeztah reviewed Apr 2, 2019

View reviewed changes

thaJeztah approved these changes Apr 9, 2019

View reviewed changes

thaJeztah merged commit d6af3e1 into docker:master Apr 9, 2019

GordonTheTurtle added this to the 19.03.0 milestone Apr 9, 2019

thaJeztah added the impact/changelog label Apr 9, 2019

albers mentioned this pull request May 10, 2019

Add bash completion for context create --from #1868

Merged

	By default the `context` will be created from the current context. This will include both docker and, if set, kubernetes endpoint configuration. This is equivalent to `--docker "from-current=true"` or, if the current context has a kubernetes contfiguration, `--docker "from-current=true" --kubernetes "from-current=true"`:
	By default the `context` is created from the current context. This includes
	both `docker` and, if set, `kubernetes` endpoint configuration:

	By default the `context` will be created from the current context. This will include both docker and, if set, kubernetes endpoint configuration. This is equivalent to `--docker "from-current=true"` or, if the current context has a kubernetes contfiguration, `--docker "from-current=true" --kubernetes "from-current=true"`:
	If the `--from` option is not set, the `context` is created from the current
	context, and copies the `docker` and, if set, `kubernetes` endpoint configuration:

-You can select a different `context` to create from by using the `--from` option:
+Use the `--from=<context name>` option to create a new context from an
+existing context. The example below creates a new context named
+`my-context` from the existing `existing-context` context.

add --from flag to context create #1773

add --from flag to context create #1773

Conversation

zappy-shu commented Mar 22, 2019 • edited Loading

codecov-io commented Mar 22, 2019 • edited Loading

Codecov Report

simonferquel commented Mar 25, 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

silvin-lubecki commented Mar 27, 2019

silvin-lubecki left a comment

Choose a reason for hiding this comment

simonferquel commented Mar 27, 2019

simonferquel commented Mar 28, 2019

thaJeztah commented Mar 28, 2019

thaJeztah commented Mar 28, 2019

simonferquel commented Mar 28, 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

thaJeztah commented Mar 28, 2019 • edited Loading

simonferquel commented Mar 28, 2019

thaJeztah commented Mar 28, 2019

thaJeztah commented Mar 28, 2019

simonferquel commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 9, 2019

thaJeztah commented Apr 9, 2019

zappy-shu commented Mar 22, 2019 •

edited

Loading

codecov-io commented Mar 22, 2019 •

edited

Loading

thaJeztah commented Mar 28, 2019 •

edited

Loading