Makes the default docker context behavior consistent #1719

jcsirot · 2019-03-06T14:11:08Z

- What I did
Make default context behaves like a real context

when using "--context default" parameter
when printing the list of contexts
when exporting the default context to a tarball

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

codecov-io · 2019-03-06T14:24:37Z

Codecov Report

Merging #1719 into master will increase coverage by 0.12%.
The diff coverage is 64.92%.

@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
+ Coverage    56.1%   56.23%   +0.12%     
==========================================
  Files         306      307       +1     
  Lines       21049    21125      +76     
==========================================
+ Hits        11810    11880      +70     
  Misses       8382     8382              
- Partials      857      863       +6

cli/command/context/testdata/list.golden

cli/context/store/store.go

simonferquel

There is a foundational aspect I dislike about the implementation: the context store package is all about storing (and retrieving) contexts to disk. The 'default' context is not stored, so the store should have no knowledge about it.

Something you might want to do though, is to implement a separate "defaultContextStore" of the store.Store interface for allowing to dynamically resolve the "default" context when needed by operations like list, inspect and export (which would consume both the on-disk context store and this "defaultContextStore". If it makes sense, feel free to introduce an interface that is a subset of store.Store that satisfies the requirements of list/inspect/export only to make it easier to implement that.

jcsirot · 2019-03-07T14:54:28Z

@simonferquel understood. Actually I was more considering the metadatastore and tlsstore as the real storage backend and the store as a proxy in front of these implementations. I also considered adding another implementation of Store for the default context and create a sort of multiplexer but I was wondering if it wasn't over-engineering for little gains. But ok, I'm going to refactor that.

simonferquel

A few things to fix, but looks good overall
Really like that you managed to make a "virtual store exposing the default context" available for all client code, including plugins.

simonferquel · 2019-03-12T10:18:44Z

cli/command/cli.go


 func resolveDockerEndpoint(s store.Store, contextName string, opts *cliflags.CommonOptions) (docker.Endpoint, error) {
-	if contextName != "" {
+	if contextName != "" || contextName == DefaultContextName {


feels like the comparison here is wrong. should be:

Suggested change

if contextName != "" || contextName == DefaultContextName {

if contextName != "" && contextName != DefaultContextName {

simonferquel · 2019-03-12T10:23:06Z

cli/command/context/list.go

 		desc := formatter.ClientContext{
 			Name:               rawMeta.Name,
-			Current:            rawMeta.Name == curContext,
+			Current:            rawMeta.Name == curContext || (curContext == "" && rawMeta.Name == command.DefaultContextName),


Could be simplified by putting a simpler

if curContext == ""{ curContext = command.DefaultContextName }

on top of the loop

cli/command/context/list_test.go

simonferquel · 2019-03-12T10:26:22Z

cli/command/defaultcontextstore.go

+		kubeconfig = filepath.Join(homedir.Get(), ".kube/config")
+	}
+	kubeEP, err := kubernetes.FromKubeConfig(kubeconfig, "", "")
+	if stackOrchestrator == OrchestratorKubernetes && err != nil {


Should we also handle OrchestratorAll ?

You can use stackOrchestrator.HasKubernetes then (see code here)

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

cli/command/defaultcontextstore.go

+			return nil, err
+		}
+		if defaultContext.TLS.Endpoints[endpointName].Files[fileName] == nil {
+			return nil, errors.Errorf("tls data for %s/%s/%s does not exist", DefaultContextName, endpointName, fileName)


I think there is a typed error interface for not-found cases in the store package. It should return an error satisfying this interface

So just checked, there is no error interface but a specific error type (see here: https://github.com/docker/cli/blob/master/cli/context/store/store.go#L324). I suggest you introduce a tlsDataNotExistError interface in store package, and modify IsErrTLSDataDoesNotExist to check this interface, and makes this code return an error satisfying this interface.

cli/command/defaultcontextstore.go

thaJeztah

left some suggestions/questions; I haven't checked all the cases, but I think some of the special cases would not be needed if ContextStoreWithDefault indeed implements the interface (and abstracts the "default" context to act as a "real" context)

thaJeztah · 2019-03-12T10:02:03Z

cli/command/cli.go


 func resolveDockerEndpoint(s store.Store, contextName string, opts *cliflags.CommonOptions) (docker.Endpoint, error) {
-	if contextName != "" {
+	if contextName != "" || contextName == DefaultContextName {


Is this logic flipped? i.e. should this be

Suggested change

if contextName != "" || contextName == DefaultContextName {

if contextName != "" && contextName != DefaultContextName {

But perhaps better to return early for the special case;

Suggested change

if contextName != "" || contextName == DefaultContextName {

if contextName == "" || contextName == DefaultContextName {

return resolveDefaultDockerEndpoint(opts)

}

cli/command/context/list.go

thaJeztah · 2019-03-12T10:25:41Z

cli/command/cli.go

 		currentContext = configFile.CurrentContext
 	}
-	if currentContext != "" {
+	if currentContext != "" && currentContext != DefaultContextName {


doesn't ContextStoreWithDefault now also implement GetContextMetadata()? IOW; do we need to still have this special case?

cli/command/context/export.go

thaJeztah · 2019-03-12T10:36:15Z

cli/command/cli.go

 	if cli.client == nil {
-		cli.contextStore = store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig)
+		baseContextSore := store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig)
+		cli.contextStore = &ContextStoreWithDefault{


Should ContextStoreWithDefault be in its own package? So that we could do something like;

storewithdefault.New(...)

Perhaps even integrate with the default store (but not sure if there's situations where we want that to be used separate)

ContextStoreWithDefault needs things defined in package command (related to resolving the docker context based on "legacy" cli flags and env var). That would:

either introduce a cycling dependency (dockerCli.Initialize needs this ContextStoreWithDefault)

or move much of the command package code in a separate package

implementing that in store package might introduce many new dependencies to the store package (and I don't feel like it is a good idea)

Could we pass those options in the constructor?

thaJeztah · 2019-03-12T10:39:15Z

cli/command/context/list.go

 		if kubernetesEndpoint != nil {
 			kubEndpointText = fmt.Sprintf("%s (%s)", kubernetesEndpoint.Host, kubernetesEndpoint.DefaultNamespace)
 		}
+		if rawMeta.Name == command.DefaultContextName {


If dockerCli.ContextStore().ListContexts() is handled by ContextStoreWithDefault.ListContexts (), is this special case still needed, or would it already return the right information?

@thaJeztah I didn't want to add specific code for import/export command to avoid, when importing an exported default context, having a second context with the "Current DOCKER_HOST based configuration" description. Therefore I decided to leave the description empty for the default context and add handle that case here.

Still a bit on the fence on this one (i.e., the same applies to other contexts, which also by default copy the description), but not a blocker

Indeed. I think it is less confusing for other contexts. Maybe it would be nice to add a --description flag to import command in order to override the imported context description

simonferquel · 2019-03-12T11:10:43Z

After rebasing #1690, can you rebase on this PR (we have pending work on docker-app requiring both)

cli/command/context/remove_test.go

cli/context/store/store.go

cli/command/defaultcontextstore.go

silvin-lubecki · 2019-03-12T15:36:57Z

cli/command/defaultcontextstore_test.go

+	srcFileList, err := s.ListContextTLSFiles("default")
+	assert.NilError(t, err)
+	destFileList, err := s.ListContextTLSFiles("dest")
+	assert.NilError(t, err)


nit: can you add some space in this block? I mean divide into logical sub-blocks, add an empty line between them and/or a comment explaining what you assert?

cli/context/store/store_test.go

cli/command/defaultcontextstore_test.go

silvin-lubecki · 2019-03-12T15:40:03Z

I like this default context store proxy on top of the context store 👍

jcsirot · 2019-03-13T13:26:28Z

PR #1690 have been rebased on this branch

simonferquel

One small change to do (to make sure IsTLSDataNotFound works for default context as well), and I think we are good.

simonferquel · 2019-03-15T14:39:11Z

cli/command/defaultcontextstore.go

+			return nil, err
+		}
+		if defaultContext.TLS.Endpoints[endpointName].Files[fileName] == nil {
+			return nil, errors.Errorf("tls data for %s/%s/%s does not exist", DefaultContextName, endpointName, fileName)


So just checked, there is no error interface but a specific error type (see here: https://github.com/docker/cli/blob/master/cli/context/store/store.go#L324). I suggest you introduce a tlsDataNotExistError interface in store package, and modify IsErrTLSDataDoesNotExist to check this interface, and makes this code return an error satisfying this interface.

simonferquel

lgtm
@thaJeztah can you PTAL alongside #1690 ? those are very important fixes for github.com/docker/app

- when using "--context default" parameter - when printing the list of contexts - when exporting the default context to a tarball Signed-off-by: Jean-Christophe Sirot <[email protected]> (+1 squashed commit) Squashed commits: [2067049] Fix CLI initialization for the `docker stack deploy --help` command and ensure that the dockerCli.CurrentContext() always returns a non empty context name (default as a fallback) Remove now obsolete code handling empty string context name Minor code cleanup Signed-off-by: Jean-Christophe Sirot <[email protected]>

thaJeztah

LGTM, thanks!

thaJeztah · 2019-03-18T11:35:19Z

cli/command/cli.go

-		currentContext = configFile.CurrentContext
+	ctxRaw, err := cli.ContextStore().GetContextMetadata(currentContext)
+	if store.IsErrContextDoesNotExist(err) {
+		// case where the currentContext has been removed (CLI behavior is to fallback to using DOCKER_HOST based resolution)


Not for this PR, but something similar came up in another discussion with @simonferquel - I think this should be an error; we should not silently fall back to DOCKER_HOST (because then I may be running commands against the wrong environment, which is likely not what you want)

thaJeztah · 2019-03-18T11:37:10Z

cli/command/context/list.go

 		if kubernetesEndpoint != nil {
 			kubEndpointText = fmt.Sprintf("%s (%s)", kubernetesEndpoint.Host, kubernetesEndpoint.DefaultNamespace)
 		}
+		if rawMeta.Name == command.DefaultContextName {


Still a bit on the fence on this one (i.e., the same applies to other contexts, which also by default copy the description), but not a blocker

silvin-lubecki

LGTM

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

jcsirot force-pushed the fix-default-context branch from cd25dd7 to 396d333 Compare March 6, 2019 14:24

jcsirot force-pushed the fix-default-context branch from 396d333 to 955bcb3 Compare March 6, 2019 14:36

thaJeztah added status/2-code-review area/plugins labels Mar 6, 2019

simonferquel reviewed Mar 7, 2019

View reviewed changes

cli/command/context/testdata/list.golden Outdated Show resolved Hide resolved

simonferquel reviewed Mar 7, 2019

View reviewed changes

cli/context/store/store.go Outdated Show resolved Hide resolved

simonferquel suggested changes Mar 7, 2019

View reviewed changes

jcsirot force-pushed the fix-default-context branch 4 times, most recently from 93d455c to 541d780 Compare March 12, 2019 09:22

thaJeztah removed the status/0-triage label Mar 12, 2019

simonferquel suggested changes Mar 12, 2019

View reviewed changes

simonferquel reviewed Mar 12, 2019

View reviewed changes

cli/command/defaultcontextstore.go Show resolved Hide resolved

simonferquel reviewed Mar 12, 2019

View reviewed changes

cli/command/defaultcontextstore.go Show resolved Hide resolved

simonferquel reviewed Mar 12, 2019

View reviewed changes

cli/command/defaultcontextstore.go Show resolved Hide resolved

simonferquel reviewed Mar 12, 2019

View reviewed changes

cli/command/defaultcontextstore.go Show resolved Hide resolved

thaJeztah requested changes Mar 12, 2019

View reviewed changes