New round of thaJeztah review

Signed-off-by: Simon Ferquel <[email protected]>

cli/command/cli.go

@@ -441,7 +441,7 @@ func UserAgent() string {
 // - fallbacks to default HOST, uses TLS config from flags/env vars
 func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile, contextstore store.Store) (string, error) {
 	if opts.Context != "" && len(opts.Hosts) > 0 {
-		return "", errors.New("Conflicting options: either specify --host or --context, not bot")
+		return "", errors.New("Conflicting options: either specify --host or --context, not both")
 	}
 	if opts.Context != "" {
 		return opts.Context, nil

cli/command/context.go

@@ -8,8 +8,8 @@ import (
 
 // DockerContext is a typed representation of what we put in Context metadata
 type DockerContext struct {
-	Description       string       `json:"description,omitempty"`
-	StackOrchestrator Orchestrator `json:"stack_orchestrator,omitempty"`
+	Description       string       `json:",omitempty"`
+	StackOrchestrator Orchestrator `json:",omitempty"`
 }
 
 // GetDockerContext extracts metadata from stored context metadata

cli/command/context/create.go

@@ -121,7 +121,7 @@ func runCreate(cli command.Cli, o *createOptions) error {
 		return err
 	}
 	fmt.Fprintln(cli.Out(), o.name)
-	fmt.Fprintf(cli.Err(), "Context %q has been created\n", o.name)
+	fmt.Fprintf(cli.Err(), "Successfully created context %q\n", o.name)
 	return nil
 }
 

cli/command/context/create_test.go

@@ -41,33 +41,76 @@ func withCliConfig(configFile *configfile.ConfigFile) func(*test.FakeCli) {
 	}
 }
 
-func TestCreateNoName(t *testing.T) {
+func TestCreateInvalids(t *testing.T) {
 	cli, cleanup := makeFakeCli(t)
 	defer cleanup()
-	err := runCreate(cli, &createOptions{})
-	assert.ErrorContains(t, err, `context name cannot be empty`)
-}
-
-func TestCreateExitingContext(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-	assert.NilError(t, cli.ContextStore().CreateOrUpdateContext(store.ContextMetadata{Name: "test"}))
-
-	err := runCreate(cli, &createOptions{
-		name: "test",
-	})
-	assert.ErrorContains(t, err, `context "test" already exists`)
-}
-
-func TestCreateInvalidOrchestrator(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-
-	err := runCreate(cli, &createOptions{
-		name:                     "test",
-		defaultStackOrchestrator: "invalid",
-	})
-	assert.ErrorContains(t, err, `specified orchestrator "invalid" is invalid, please use either kubernetes, swarm or all`)
+	assert.NilError(t, cli.ContextStore().CreateOrUpdateContext(store.ContextMetadata{Name: "existing-context"}))
+	tests := []struct {
+		options     createOptions
+		expecterErr string
+	}{
+		{
+			expecterErr: `context name cannot be empty`,
+		},
+		{
+			options: createOptions{
+				name: " ",
+			},
+			expecterErr: `context name " " is invalid`,
+		},
+		{
+			options: createOptions{
+				name: "existing-context",
+			},
+			expecterErr: `context "existing-context" already exists`,
+		},
+		{
+			options: createOptions{
+				name: "invalid-docker-host",
+				docker: map[string]string{
+					keyHost: "some///invalid/host",
+				},
+			},
+			expecterErr: `unable to parse docker host`,
+		},
+		{
+			options: createOptions{
+				name:                     "invalid-orchestrator",
+				defaultStackOrchestrator: "invalid",
+			},
+			expecterErr: `specified orchestrator "invalid" is invalid, please use either kubernetes, swarm or all`,
+		},
+		{
+			options: createOptions{
+				name:                     "orchestrator-swarm-no-endpoint",
+				defaultStackOrchestrator: "swarm",
+			},
+			expecterErr: `docker endpoint configuration is required`,
+		},
+		{
+			options: createOptions{
+				name:                     "orchestrator-kubernetes-no-endpoint",
+				defaultStackOrchestrator: "kubernetes",
+				docker:                   map[string]string{},
+			},
+			expecterErr: `cannot specify orchestrator "kubernetes" without configuring a Kubernetes endpoint`,
+		},
+		{
+			options: createOptions{
+				name:                     "orchestrator-all-no-endpoint",
+				defaultStackOrchestrator: "all",
+				docker:                   map[string]string{},
+			},
+			expecterErr: `cannot specify orchestrator "all" without configuring a Kubernetes endpoint`,
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.options.name, func(t *testing.T) {
+			err := runCreate(cli, &tc.options)
+			assert.ErrorContains(t, err, tc.expecterErr)
+		})
+	}
 }
 
 func TestCreateOrchestratorSwarm(t *testing.T) {
@@ -81,7 +124,7 @@ func TestCreateOrchestratorSwarm(t *testing.T) {
 	})
 	assert.NilError(t, err)
 	assert.Equal(t, "test\n", cli.OutBuffer().String())
-	assert.Equal(t, "Context \"test\" has been created\n", cli.ErrBuffer().String())
+	assert.Equal(t, "Successfully created context \"test\"\n", cli.ErrBuffer().String())
 }
 
 func TestCreateOrchestratorEmpty(t *testing.T) {
@@ -95,31 +138,8 @@ func TestCreateOrchestratorEmpty(t *testing.T) {
 	assert.NilError(t, err)
 }
 
-func TestCreateOrchestratorKubernetesNoEndpoint(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-
-	err := runCreate(cli, &createOptions{
-		name:                     "test",
-		defaultStackOrchestrator: "kubernetes",
-		docker:                   map[string]string{},
-	})
-	assert.ErrorContains(t, err, `cannot specify orchestrator "kubernetes" without configuring a Kubernetes endpoint`)
-}
-
-func TestCreateOrchestratorAllNoKubernetesEndpoint(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-
-	err := runCreate(cli, &createOptions{
-		name:                     "test",
-		defaultStackOrchestrator: "all",
-		docker:                   map[string]string{},
-	})
-	assert.ErrorContains(t, err, `cannot specify orchestrator "all" without configuring a Kubernetes endpoint`)
-}
-
 func validateTestKubeEndpoint(t *testing.T, s store.Store, name string) {
+	t.Helper()
 	ctxMetadata, err := s.GetContextMetadata(name)
 	assert.NilError(t, err)
 	kubeMeta := ctxMetadata.Endpoints[kubernetes.KubernetesEndpoint].(kubernetes.EndpointMeta)
@@ -132,6 +152,7 @@ func validateTestKubeEndpoint(t *testing.T, s store.Store, name string) {
 }
 
 func createTestContextWithKube(t *testing.T, cli command.Cli) {
+	t.Helper()
 	revert := env.Patch(t, "KUBECONFIG", "./testdata/test-kubeconfig")
 	defer revert()
 
@@ -152,15 +173,3 @@ func TestCreateOrchestratorAllKubernetesEndpointFromCurrent(t *testing.T) {
 	createTestContextWithKube(t, cli)
 	validateTestKubeEndpoint(t, cli.ContextStore(), "test")
 }
-
-func TestCreateInvalidDockerHost(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-	err := runCreate(cli, &createOptions{
-		name: "test",
-		docker: map[string]string{
-			keyHost: "some///invalid/host",
-		},
-	})
-	assert.ErrorContains(t, err, "unable to parse docker host")
-}

cli/command/context/export-import_test.go

@@ -39,7 +39,7 @@ func TestExportImportWithFile(t *testing.T) {
 	assert.Equal(t, "test2", context2.Name)
 
 	assert.Equal(t, "test2\n", cli.OutBuffer().String())
-	assert.Equal(t, "Context \"test2\" has been imported\n", cli.ErrBuffer().String())
+	assert.Equal(t, "Successfully imported context \"test2\"\n", cli.ErrBuffer().String())
 }
 
 func TestExportImportPipe(t *testing.T) {
@@ -67,7 +67,7 @@ func TestExportImportPipe(t *testing.T) {
 	assert.Equal(t, "test2", context2.Name)
 
 	assert.Equal(t, "test2\n", cli.OutBuffer().String())
-	assert.Equal(t, "Context \"test2\" has been imported\n", cli.ErrBuffer().String())
+	assert.Equal(t, "Successfully imported context \"test2\"\n", cli.ErrBuffer().String())
 }
 
 func TestExportKubeconfig(t *testing.T) {
@@ -105,9 +105,6 @@ func TestExportExistingFile(t *testing.T) {
 	createTestContextWithKube(t, cli)
 	cli.ErrBuffer().Reset()
 	assert.NilError(t, ioutil.WriteFile(contextFile, []byte{}, 0644))
-	assert.ErrorContains(t, runExport(cli, &exportOptions{
-		contextName: "test",
-		dest:        contextFile,
-	}), "exists")
-
+	err = runExport(cli, &exportOptions{contextName: "test", dest: contextFile})
+	assert.Assert(t, os.IsExist(err))
 }

cli/command/context/import.go

@@ -43,6 +43,6 @@ func runImport(dockerCli command.Cli, name string, source string) error {
 		return err
 	}
 	fmt.Fprintln(dockerCli.Out(), name)
-	fmt.Fprintf(dockerCli.Err(), "Context %q has been imported\n", name)
+	fmt.Fprintf(dockerCli.Err(), "Successfully imported context %q\n", name)
 	return nil
 }

cli/command/context/inspect.go

@@ -40,6 +40,9 @@ func newInspectCommand(dockerCli command.Cli) *cobra.Command {
 
 func runInspect(dockerCli command.Cli, opts inspectOptions) error {
 	getRefFunc := func(ref string) (interface{}, []byte, error) {
+		if ref == "default" {
+			return nil, nil, errors.New(`context "default" cannot be inspected`)
+		}
 		c, err := dockerCli.ContextStore().GetContextMetadata(ref)
 		if err != nil {
 			return nil, nil, err
@@ -59,6 +62,6 @@ func runInspect(dockerCli command.Cli, opts inspectOptions) error {
 
 type contextWithTLSListing struct {
 	store.ContextMetadata
-	TLSMaterial map[string]store.EndpointFiles `json:"tls-material,omitempty"`
-	Storage     store.ContextStorageInfo       `json:"storage,omitempty"`
+	TLSMaterial map[string]store.EndpointFiles
+	Storage     store.ContextStorageInfo
 }

cli/command/context/inspect_test.go

@@ -0,0 +1,24 @@
+package context
+
+import (
+	"strings"
+	"testing"
+
+	"gotest.tools/assert"
+	"gotest.tools/golden"
+)
+
+func TestInspect(t *testing.T) {
+	cli, cleanup := makeFakeCli(t)
+	defer cleanup()
+	createTestContextWithKubeAndSwarm(t, cli, "current", "all")
+	cli.OutBuffer().Reset()
+	assert.NilError(t, runInspect(cli, inspectOptions{
+		refs: []string{"current"},
+	}))
+	expected := string(golden.Get(t, "inspect.golden"))
+	si := cli.ContextStore().GetContextStorageInfo("current")
+	expected = strings.Replace(expected, "<METADATA_PATH>", strings.Replace(si.MetadataPath, `\`, `\\`, -1), 1)
+	expected = strings.Replace(expected, "<TLS_PATH>", strings.Replace(si.TLSPath, `\`, `\\`, -1), 1)
+	assert.Equal(t, cli.OutBuffer().String(), expected)
+}

cli/command/context/list.go

@@ -2,6 +2,8 @@ package context
 
 import (
 	"fmt"
+	"sort"
+	"vbom.ml/util/sortorder"
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
@@ -66,28 +68,32 @@ func runList(dockerCli command.Cli, opts *listOptions) error {
 		}
 		contexts = append(contexts, &desc)
 	}
-	if dockerCli.CurrentContext() == "" && !opts.quiet {
-		orchestrator, _ := dockerCli.StackOrchestrator("")
-		kubEndpointText := ""
-		kubeconfig := kubernetes.NewKubernetesConfig("")
-		if cfg, err := kubeconfig.ClientConfig(); err == nil {
-			ns, _, _ := kubeconfig.Namespace()
-			if ns == "" {
-				ns = "default"
-			}
-			kubEndpointText = fmt.Sprintf("%s (%s)", cfg.Host, ns)
-		}
-		// prepend a "virtual context"
+	if !opts.quiet {
 		desc := &formatter.ClientContext{
-			Name:               "default",
-			Current:            true,
-			Description:        "Current DOCKER_HOST based configuration",
-			StackOrchestrator:  string(orchestrator),
-			DockerEndpoint:     dockerCli.DockerEndpoint().Host,
-			KubernetesEndpoint: kubEndpointText,
+			Name:        "default",
+			Description: "Current DOCKER_HOST based configuration",
+		}
+		if dockerCli.CurrentContext() == "" {
+			orchestrator, _ := dockerCli.StackOrchestrator("")
+			kubEndpointText := ""
+			kubeconfig := kubernetes.NewKubernetesConfig("")
+			if cfg, err := kubeconfig.ClientConfig(); err == nil {
+				ns, _, _ := kubeconfig.Namespace()
+				if ns == "" {
+					ns = "default"
+				}
+				kubEndpointText = fmt.Sprintf("%s (%s)", cfg.Host, ns)
+			}
+			desc.Current = true
+			desc.StackOrchestrator = string(orchestrator)
+			desc.DockerEndpoint = dockerCli.DockerEndpoint().Host
+			desc.KubernetesEndpoint = kubEndpointText
 		}
-		contexts = append([]*formatter.ClientContext{desc}, contexts...)
+		contexts = append(contexts, desc)
 	}
+	sort.Slice(contexts, func(i, j int) bool {
+		return sortorder.NaturalLess(contexts[i].Name, contexts[j].Name)
+	})
 	return format(dockerCli, opts, contexts)
 }
 

cli/command/context/list_test.go

@@ -1,7 +1,6 @@
 package context
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/docker/cli/cli/command"
@@ -31,6 +30,7 @@ func TestList(t *testing.T) {
 	defer cleanup()
 	createTestContextWithKubeAndSwarm(t, cli, "current", "all")
 	createTestContextWithKubeAndSwarm(t, cli, "other", "all")
+	createTestContextWithKubeAndSwarm(t, cli, "unset", "unset")
 	cli.SetCurrentContext("current")
 	cli.OutBuffer().Reset()
 	assert.NilError(t, runList(cli, &listOptions{
@@ -39,19 +39,6 @@ func TestList(t *testing.T) {
 	golden.Assert(t, cli.OutBuffer().String(), "list.golden")
 }
 
-func TestListUnset(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-	createTestContextWithKubeAndSwarm(t, cli, "current", "unset")
-	createTestContextWithKubeAndSwarm(t, cli, "other", "all")
-	cli.SetCurrentContext("current")
-	cli.OutBuffer().Reset()
-	assert.NilError(t, runList(cli, &listOptions{
-		format: formatter.TableFormatKey,
-	}))
-	golden.Assert(t, cli.OutBuffer().String(), "list.unset.golden")
-}
-
 func TestListNoContext(t *testing.T) {
 	cli, cleanup := makeFakeCli(t)
 	defer cleanup()
@@ -81,18 +68,3 @@ func TestListQuiet(t *testing.T) {
 	}))
 	golden.Assert(t, cli.OutBuffer().String(), "quiet-list.golden")
 }
-
-func TestInspect(t *testing.T) {
-	cli, cleanup := makeFakeCli(t)
-	defer cleanup()
-	createTestContextWithKubeAndSwarm(t, cli, "current", "all")
-	cli.OutBuffer().Reset()
-	assert.NilError(t, runInspect(cli, inspectOptions{
-		refs: []string{"current"},
-	}))
-	expected := string(golden.Get(t, "inspect.golden"))
-	si := cli.ContextStore().GetContextStorageInfo("current")
-	expected = strings.Replace(expected, "<METADATA_PATH>", strings.Replace(si.MetadataPath, `\`, `\\`, -1), 1)
-	expected = strings.Replace(expected, "<TLS_PATH>", strings.Replace(si.TLSPath, `\`, `\\`, -1), 1)
-	assert.Equal(t, cli.OutBuffer().String(), expected)
-}