Skip to content

Commit 394482e

Browse files
simonferquelsimonferquel
committed
New round of thaJeztah review
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
1 parent 6f2d429 commit 394482e

File tree

19 files changed

+182
-169
lines changed

19 files changed

+182
-169
lines changed

‎cli/command/cli.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ func UserAgent() string {
441441
// - fallbacks to default HOST, uses TLS config from flags/env vars
442442
func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile, contextstore store.Store) (string, error) {
443443
if opts.Context != "" && len(opts.Hosts) > 0 {
444-
return "", errors.New("Conflicting options: either specify --host or --context, not bot")
444+
return "", errors.New("Conflicting options: either specify --host or --context, not both")
445445
}
446446
if opts.Context != "" {
447447
return opts.Context, nil

‎cli/command/context.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import (
88

99
// DockerContext is a typed representation of what we put in Context metadata
1010
type DockerContext struct {
11-
Description string `json:"description,omitempty"`
12-
StackOrchestrator Orchestrator `json:"stack_orchestrator,omitempty"`
11+
Description string `json:",omitempty"`
12+
StackOrchestrator Orchestrator `json:",omitempty"`
1313
}
1414

1515
// GetDockerContext extracts metadata from stored context metadata

‎cli/command/context/create.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func runCreate(cli command.Cli, o *createOptions) error {
121121
return err
122122
}
123123
fmt.Fprintln(cli.Out(), o.name)
124-
fmt.Fprintf(cli.Err(), "Context %q has been created\n", o.name)
124+
fmt.Fprintf(cli.Err(), "Successfully created context %q\n", o.name)
125125
return nil
126126
}
127127

‎cli/command/context/create_test.go‎

Lines changed: 71 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -41,33 +41,76 @@ func withCliConfig(configFile *configfile.ConfigFile) func(*test.FakeCli) {
4141
}
4242
}
4343

44-
func TestCreateNoName(t *testing.T) {
44+
func TestCreateInvalids(t *testing.T) {
4545
cli, cleanup := makeFakeCli(t)
4646
defer cleanup()
47-
err := runCreate(cli, &createOptions{})
48-
assert.ErrorContains(t, err, `context name cannot be empty`)
49-
}
50-
51-
func TestCreateExitingContext(t *testing.T) {
52-
cli, cleanup := makeFakeCli(t)
53-
defer cleanup()
54-
assert.NilError(t, cli.ContextStore().CreateOrUpdateContext(store.ContextMetadata{Name: "test"}))
55-
56-
err := runCreate(cli, &createOptions{
57-
name: "test",
58-
})
59-
assert.ErrorContains(t, err, `context "test" already exists`)
60-
}
61-
62-
func TestCreateInvalidOrchestrator(t *testing.T) {
63-
cli, cleanup := makeFakeCli(t)
64-
defer cleanup()
65-
66-
err := runCreate(cli, &createOptions{
67-
name: "test",
68-
defaultStackOrchestrator: "invalid",
69-
})
70-
assert.ErrorContains(t, err, `specified orchestrator "invalid" is invalid, please use either kubernetes, swarm or all`)
47+
assert.NilError(t, cli.ContextStore().CreateOrUpdateContext(store.ContextMetadata{Name: "existing-context"}))
48+
tests := []struct {
49+
options createOptions
50+
expecterErr string
51+
}{
52+
{
53+
expecterErr: `context name cannot be empty`,
54+
},
55+
{
56+
options: createOptions{
57+
name: " ",
58+
},
59+
expecterErr: `context name " " is invalid`,
60+
},
61+
{
62+
options: createOptions{
63+
name: "existing-context",
64+
},
65+
expecterErr: `context "existing-context" already exists`,
66+
},
67+
{
68+
options: createOptions{
69+
name: "invalid-docker-host",
70+
docker: map[string]string{
71+
keyHost: "some///invalid/host",
72+
},
73+
},
74+
expecterErr: `unable to parse docker host`,
75+
},
76+
{
77+
options: createOptions{
78+
name: "invalid-orchestrator",
79+
defaultStackOrchestrator: "invalid",
80+
},
81+
expecterErr: `specified orchestrator "invalid" is invalid, please use either kubernetes, swarm or all`,
82+
},
83+
{
84+
options: createOptions{
85+
name: "orchestrator-swarm-no-endpoint",
86+
defaultStackOrchestrator: "swarm",
87+
},
88+
expecterErr: `docker endpoint configuration is required`,
89+
},
90+
{
91+
options: createOptions{
92+
name: "orchestrator-kubernetes-no-endpoint",
93+
defaultStackOrchestrator: "kubernetes",
94+
docker: map[string]string{},
95+
},
96+
expecterErr: `cannot specify orchestrator "kubernetes" without configuring a Kubernetes endpoint`,
97+
},
98+
{
99+
options: createOptions{
100+
name: "orchestrator-all-no-endpoint",
101+
defaultStackOrchestrator: "all",
102+
docker: map[string]string{},
103+
},
104+
expecterErr: `cannot specify orchestrator "all" without configuring a Kubernetes endpoint`,
105+
},
106+
}
107+
for _, tc := range tests {
108+
tc := tc
109+
t.Run(tc.options.name, func(t *testing.T) {
110+
err := runCreate(cli, &tc.options)
111+
assert.ErrorContains(t, err, tc.expecterErr)
112+
})
113+
}
71114
}
72115

73116
func TestCreateOrchestratorSwarm(t *testing.T) {
@@ -81,7 +124,7 @@ func TestCreateOrchestratorSwarm(t *testing.T) {
81124
})
82125
assert.NilError(t, err)
83126
assert.Equal(t, "test\n", cli.OutBuffer().String())
84-
assert.Equal(t, "Context \"test\" has been created\n", cli.ErrBuffer().String())
127+
assert.Equal(t, "Successfully created context \"test\"\n", cli.ErrBuffer().String())
85128
}
86129

87130
func TestCreateOrchestratorEmpty(t *testing.T) {
@@ -95,31 +138,8 @@ func TestCreateOrchestratorEmpty(t *testing.T) {
95138
assert.NilError(t, err)
96139
}
97140

98-
func TestCreateOrchestratorKubernetesNoEndpoint(t *testing.T) {
99-
cli, cleanup := makeFakeCli(t)
100-
defer cleanup()
101-
102-
err := runCreate(cli, &createOptions{
103-
name: "test",
104-
defaultStackOrchestrator: "kubernetes",
105-
docker: map[string]string{},
106-
})
107-
assert.ErrorContains(t, err, `cannot specify orchestrator "kubernetes" without configuring a Kubernetes endpoint`)
108-
}
109-
110-
func TestCreateOrchestratorAllNoKubernetesEndpoint(t *testing.T) {
111-
cli, cleanup := makeFakeCli(t)
112-
defer cleanup()
113-
114-
err := runCreate(cli, &createOptions{
115-
name: "test",
116-
defaultStackOrchestrator: "all",
117-
docker: map[string]string{},
118-
})
119-
assert.ErrorContains(t, err, `cannot specify orchestrator "all" without configuring a Kubernetes endpoint`)
120-
}
121-
122141
func validateTestKubeEndpoint(t *testing.T, s store.Store, name string) {
142+
t.Helper()
123143
ctxMetadata, err := s.GetContextMetadata(name)
124144
assert.NilError(t, err)
125145
kubeMeta := ctxMetadata.Endpoints[kubernetes.KubernetesEndpoint].(kubernetes.EndpointMeta)
@@ -132,6 +152,7 @@ func validateTestKubeEndpoint(t *testing.T, s store.Store, name string) {
132152
}
133153

134154
func createTestContextWithKube(t *testing.T, cli command.Cli) {
155+
t.Helper()
135156
revert := env.Patch(t, "KUBECONFIG", "./testdata/test-kubeconfig")
136157
defer revert()
137158

@@ -152,15 +173,3 @@ func TestCreateOrchestratorAllKubernetesEndpointFromCurrent(t *testing.T) {
152173
createTestContextWithKube(t, cli)
153174
validateTestKubeEndpoint(t, cli.ContextStore(), "test")
154175
}
155-
156-
func TestCreateInvalidDockerHost(t *testing.T) {
157-
cli, cleanup := makeFakeCli(t)
158-
defer cleanup()
159-
err := runCreate(cli, &createOptions{
160-
name: "test",
161-
docker: map[string]string{
162-
keyHost: "some///invalid/host",
163-
},
164-
})
165-
assert.ErrorContains(t, err, "unable to parse docker host")
166-
}

‎cli/command/context/export-import_test.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func TestExportImportWithFile(t *testing.T) {
3939
assert.Equal(t, "test2", context2.Name)
4040

4141
assert.Equal(t, "test2\n", cli.OutBuffer().String())
42-
assert.Equal(t, "Context \"test2\" has been imported\n", cli.ErrBuffer().String())
42+
assert.Equal(t, "Successfully imported context \"test2\"\n", cli.ErrBuffer().String())
4343
}
4444

4545
func TestExportImportPipe(t *testing.T) {
@@ -67,7 +67,7 @@ func TestExportImportPipe(t *testing.T) {
6767
assert.Equal(t, "test2", context2.Name)
6868

6969
assert.Equal(t, "test2\n", cli.OutBuffer().String())
70-
assert.Equal(t, "Context \"test2\" has been imported\n", cli.ErrBuffer().String())
70+
assert.Equal(t, "Successfully imported context \"test2\"\n", cli.ErrBuffer().String())
7171
}
7272

7373
func TestExportKubeconfig(t *testing.T) {
@@ -105,9 +105,6 @@ func TestExportExistingFile(t *testing.T) {
105105
createTestContextWithKube(t, cli)
106106
cli.ErrBuffer().Reset()
107107
assert.NilError(t, ioutil.WriteFile(contextFile, []byte{}, 0644))
108-
assert.ErrorContains(t, runExport(cli, &exportOptions{
109-
contextName: "test",
110-
dest: contextFile,
111-
}), "exists")
112-
108+
err = runExport(cli, &exportOptions{contextName: "test", dest: contextFile})
109+
assert.Assert(t, os.IsExist(err))
113110
}

‎cli/command/context/import.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,6 @@ func runImport(dockerCli command.Cli, name string, source string) error {
4343
return err
4444
}
4545
fmt.Fprintln(dockerCli.Out(), name)
46-
fmt.Fprintf(dockerCli.Err(), "Context %q has been imported\n", name)
46+
fmt.Fprintf(dockerCli.Err(), "Successfully imported context %q\n", name)
4747
return nil
4848
}

‎cli/command/context/inspect.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ func newInspectCommand(dockerCli command.Cli) *cobra.Command {
4040

4141
func runInspect(dockerCli command.Cli, opts inspectOptions) error {
4242
getRefFunc := func(ref string) (interface{}, []byte, error) {
43+
if ref == "default" {
44+
return nil, nil, errors.New(`context "default" cannot be inspected`)
45+
}
4346
c, err := dockerCli.ContextStore().GetContextMetadata(ref)
4447
if err != nil {
4548
return nil, nil, err
@@ -59,6 +62,6 @@ func runInspect(dockerCli command.Cli, opts inspectOptions) error {
5962

6063
type contextWithTLSListing struct {
6164
store.ContextMetadata
62-
TLSMaterial map[string]store.EndpointFiles `json:"tls-material,omitempty"`
63-
Storage store.ContextStorageInfo `json:"storage,omitempty"`
65+
TLSMaterial map[string]store.EndpointFiles
66+
Storage store.ContextStorageInfo
6467
}

‎cli/command/context/inspect_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package context
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
"gotest.tools/assert"
8+
"gotest.tools/golden"
9+
)
10+
11+
func TestInspect(t *testing.T) {
12+
cli, cleanup := makeFakeCli(t)
13+
defer cleanup()
14+
createTestContextWithKubeAndSwarm(t, cli, "current", "all")
15+
cli.OutBuffer().Reset()
16+
assert.NilError(t, runInspect(cli, inspectOptions{
17+
refs: []string{"current"},
18+
}))
19+
expected := string(golden.Get(t, "inspect.golden"))
20+
si := cli.ContextStore().GetContextStorageInfo("current")
21+
expected = strings.Replace(expected, "<METADATA_PATH>", strings.Replace(si.MetadataPath, `\`, `\\`, -1), 1)
22+
expected = strings.Replace(expected, "<TLS_PATH>", strings.Replace(si.TLSPath, `\`, `\\`, -1), 1)
23+
assert.Equal(t, cli.OutBuffer().String(), expected)
24+
}

‎cli/command/context/list.go‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package context
22

33
import (
44
"fmt"
5+
"sort"
56

67
"github.com/docker/cli/cli"
78
"github.com/docker/cli/cli/command"
@@ -10,6 +11,7 @@ import (
1011
kubecontext "github.com/docker/cli/cli/context/kubernetes"
1112
"github.com/docker/cli/kubernetes"
1213
"github.com/spf13/cobra"
14+
"vbom.ml/util/sortorder"
1315
)
1416

1517
type listOptions struct {
@@ -30,12 +32,15 @@ func newListCommand(dockerCli command.Cli) *cobra.Command {
3032
}
3133

3234
flags := cmd.Flags()
33-
flags.StringVar(&opts.format, "format", formatter.TableFormatKey, "Pretty-print contexts using a Go template")
35+
flags.StringVar(&opts.format, "format", "", "Pretty-print contexts using a Go template")
3436
flags.BoolVarP(&opts.quiet, "quiet", "q", false, "Only show context names")
3537
return cmd
3638
}
3739

3840
func runList(dockerCli command.Cli, opts *listOptions) error {
41+
if opts.format == "" {
42+
opts.format = formatter.TableFormatKey
43+
}
3944
curContext := dockerCli.CurrentContext()
4045
contextMap, err := dockerCli.ContextStore().ListContexts()
4146
if err != nil {
@@ -66,28 +71,32 @@ func runList(dockerCli command.Cli, opts *listOptions) error {
6671
}
6772
contexts = append(contexts, &desc)
6873
}
69-
if dockerCli.CurrentContext() == "" && !opts.quiet {
70-
orchestrator, _ := dockerCli.StackOrchestrator("")
71-
kubEndpointText := ""
72-
kubeconfig := kubernetes.NewKubernetesConfig("")
73-
if cfg, err := kubeconfig.ClientConfig(); err == nil {
74-
ns, _, _ := kubeconfig.Namespace()
75-
if ns == "" {
76-
ns = "default"
77-
}
78-
kubEndpointText = fmt.Sprintf("%s (%s)", cfg.Host, ns)
79-
}
80-
// prepend a "virtual context"
74+
if !opts.quiet {
8175
desc := &formatter.ClientContext{
82-
Name: "default",
83-
Current: true,
84-
Description: "Current DOCKER_HOST based configuration",
85-
StackOrchestrator: string(orchestrator),
86-
DockerEndpoint: dockerCli.DockerEndpoint().Host,
87-
KubernetesEndpoint: kubEndpointText,
76+
Name: "default",
77+
Description: "Current DOCKER_HOST based configuration",
78+
}
79+
if dockerCli.CurrentContext() == "" {
80+
orchestrator, _ := dockerCli.StackOrchestrator("")
81+
kubEndpointText := ""
82+
kubeconfig := kubernetes.NewKubernetesConfig("")
83+
if cfg, err := kubeconfig.ClientConfig(); err == nil {
84+
ns, _, _ := kubeconfig.Namespace()
85+
if ns == "" {
86+
ns = "default"
87+
}
88+
kubEndpointText = fmt.Sprintf("%s (%s)", cfg.Host, ns)
89+
}
90+
desc.Current = true
91+
desc.StackOrchestrator = string(orchestrator)
92+
desc.DockerEndpoint = dockerCli.DockerEndpoint().Host
93+
desc.KubernetesEndpoint = kubEndpointText
8894
}
89-
contexts = append([]*formatter.ClientContext{desc}, contexts...)
95+
contexts = append(contexts, desc)
9096
}
97+
sort.Slice(contexts, func(i, j int) bool {
98+
return sortorder.NaturalLess(contexts[i].Name, contexts[j].Name)
99+
})
91100
return format(dockerCli, opts, contexts)
92101
}
93102

0 commit comments

Comments
 (0)