From ad409767bfced6b341c659297a5df464cf75d5a2 Mon Sep 17 00:00:00 2001 From: Silvin Lubecki Date: Tue, 26 Dec 2017 15:40:17 +0100 Subject: [PATCH] Activate kubernetes only when experimental cli is enabled * Refactor tests on version and kubernetes switch * Fix rebase errors * Refactor for gocyclo linter Signed-off-by: Silvin Lubecki --- cli/command/cli.go | 4 +- cli/command/cli_test.go | 104 +++++++++++++++++++++++++++++ cli/command/orchestrator.go | 6 +- cli/command/stack/deploy.go | 2 +- cli/command/stack/list.go | 2 +- cli/command/stack/ps.go | 2 +- cli/command/stack/remove.go | 2 +- cli/command/stack/services.go | 2 +- cli/command/system/version_test.go | 16 ++--- cmd/docker/docker.go | 73 ++++++++------------ internal/test/cli.go | 15 +++++ 11 files changed, 163 insertions(+), 65 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index bdd3b18f24..57379ce749 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -136,11 +136,11 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { if err != nil { return errors.Wrap(err, "Experimental field") } - orchestrator := GetOrchestrator(opts.Common.Orchestrator, cli.configFile.Orchestrator) + orchestrator := GetOrchestrator(hasExperimental, opts.Common.Orchestrator, cli.configFile.Orchestrator) cli.clientInfo = ClientInfo{ DefaultVersion: cli.client.ClientVersion(), HasExperimental: hasExperimental, - HasKubernetes: orchestrator == OrchestratorKubernetes, + HasKubernetes: hasExperimental && orchestrator == OrchestratorKubernetes, Orchestrator: orchestrator, } cli.initializeFromClient() diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go index d2cf1352d6..37047551e5 100644 --- a/cli/command/cli_test.go +++ b/cli/command/cli_test.go @@ -170,6 +170,110 @@ func TestExperimentalCLI(t *testing.T) { } } +func TestOrchestratorSwitch(t *testing.T) { + defaultVersion := "v1.55" + + var testcases = []struct { + doc string + configfile string + envOrchestrator string + flagOrchestrator string + expectedOrchestrator string + expectedKubernetes bool + }{ + { + doc: "default", + configfile: `{ + "experimental": "enabled" + }`, + expectedOrchestrator: "swarm", + expectedKubernetes: false, + }, + { + doc: "kubernetesIsExperimental", + configfile: `{ + "experimental": "disabled", + "orchestrator": "kubernetes" + }`, + envOrchestrator: "kubernetes", + flagOrchestrator: "kubernetes", + expectedOrchestrator: "swarm", + expectedKubernetes: false, + }, + { + doc: "kubernetesConfigFile", + configfile: `{ + "experimental": "enabled", + "orchestrator": "kubernetes" + }`, + expectedOrchestrator: "kubernetes", + expectedKubernetes: true, + }, + { + doc: "kubernetesEnv", + configfile: `{ + "experimental": "enabled" + }`, + envOrchestrator: "kubernetes", + expectedOrchestrator: "kubernetes", + expectedKubernetes: true, + }, + { + doc: "kubernetesFlag", + configfile: `{ + "experimental": "enabled" + }`, + flagOrchestrator: "kubernetes", + expectedOrchestrator: "kubernetes", + expectedKubernetes: true, + }, + { + doc: "envOverridesConfigFile", + configfile: `{ + "experimental": "enabled", + "orchestrator": "kubernetes" + }`, + envOrchestrator: "swarm", + expectedOrchestrator: "swarm", + expectedKubernetes: false, + }, + { + doc: "flagOverridesEnv", + configfile: `{ + "experimental": "enabled" + }`, + envOrchestrator: "kubernetes", + flagOrchestrator: "swarm", + expectedOrchestrator: "swarm", + expectedKubernetes: false, + }, + } + + for _, testcase := range testcases { + t.Run(testcase.doc, func(t *testing.T) { + dir := fs.NewDir(t, testcase.doc, fs.WithFile("config.json", testcase.configfile)) + defer dir.Remove() + apiclient := &fakeClient{ + version: defaultVersion, + } + if testcase.envOrchestrator != "" { + defer patchEnvVariable(t, "DOCKER_ORCHESTRATOR", testcase.envOrchestrator)() + } + + cli := &DockerCli{client: apiclient, err: os.Stderr} + cliconfig.SetDir(dir.Path()) + options := flags.NewClientOptions() + if testcase.flagOrchestrator != "" { + options.Common.Orchestrator = testcase.flagOrchestrator + } + err := cli.Initialize(options) + assert.NoError(t, err) + assert.Equal(t, testcase.expectedKubernetes, cli.ClientInfo().HasKubernetes) + assert.Equal(t, testcase.expectedOrchestrator, string(cli.ClientInfo().Orchestrator)) + }) + } +} + func TestGetClientWithPassword(t *testing.T) { expected := "password" diff --git a/cli/command/orchestrator.go b/cli/command/orchestrator.go index 6cf0e4603b..520f030196 100644 --- a/cli/command/orchestrator.go +++ b/cli/command/orchestrator.go @@ -33,7 +33,11 @@ func normalize(flag string) Orchestrator { // GetOrchestrator checks DOCKER_ORCHESTRATOR environment variable and configuration file // orchestrator value and returns user defined Orchestrator. -func GetOrchestrator(flagValue, value string) Orchestrator { +func GetOrchestrator(isExperimental bool, flagValue, value string) Orchestrator { + // Non experimental CLI has kubernetes disabled + if !isExperimental { + return defaultOrchestrator + } // Check flag if o := normalize(flagValue); o != orchestratorUnset { return o diff --git a/cli/command/stack/deploy.go b/cli/command/stack/deploy.go index d01add7b98..f9866716a8 100644 --- a/cli/command/stack/deploy.go +++ b/cli/command/stack/deploy.go @@ -19,7 +19,7 @@ func newDeployCommand(dockerCli command.Cli) *cobra.Command { Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespace = args[0] - if dockerCli.ClientInfo().HasKubernetes() { + if dockerCli.ClientInfo().HasKubernetes { kli, err := kubernetes.WrapCli(dockerCli, cmd) if err != nil { return err diff --git a/cli/command/stack/list.go b/cli/command/stack/list.go index 2cb10c1716..4db83765fe 100644 --- a/cli/command/stack/list.go +++ b/cli/command/stack/list.go @@ -18,7 +18,7 @@ func newListCommand(dockerCli command.Cli) *cobra.Command { Short: "List stacks", Args: cli.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - if dockerCli.ClientInfo().HasKubernetes() { + if dockerCli.ClientInfo().HasKubernetes { kli, err := kubernetes.WrapCli(dockerCli, cmd) if err != nil { return err diff --git a/cli/command/stack/ps.go b/cli/command/stack/ps.go index 8e835201a2..cd8c32a28e 100644 --- a/cli/command/stack/ps.go +++ b/cli/command/stack/ps.go @@ -19,7 +19,7 @@ func newPsCommand(dockerCli command.Cli) *cobra.Command { Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespace = args[0] - if dockerCli.ClientInfo().HasKubernetes() { + if dockerCli.ClientInfo().HasKubernetes { kli, err := kubernetes.WrapCli(dockerCli, cmd) if err != nil { return err diff --git a/cli/command/stack/remove.go b/cli/command/stack/remove.go index e4dd913dbd..8324ba09a2 100644 --- a/cli/command/stack/remove.go +++ b/cli/command/stack/remove.go @@ -19,7 +19,7 @@ func newRemoveCommand(dockerCli command.Cli) *cobra.Command { Args: cli.RequiresMinArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespaces = args - if dockerCli.ClientInfo().HasKubernetes() { + if dockerCli.ClientInfo().HasKubernetes { kli, err := kubernetes.WrapCli(dockerCli, cmd) if err != nil { return err diff --git a/cli/command/stack/services.go b/cli/command/stack/services.go index 7f427c170e..b3331c3df4 100644 --- a/cli/command/stack/services.go +++ b/cli/command/stack/services.go @@ -19,7 +19,7 @@ func newServicesCommand(dockerCli command.Cli) *cobra.Command { Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespace = args[0] - if dockerCli.ClientInfo().HasKubernetes() { + if dockerCli.ClientInfo().HasKubernetes { kli, err := kubernetes.WrapCli(dockerCli, cmd) if err != nil { return err diff --git a/cli/command/system/version_test.go b/cli/command/system/version_test.go index 32b5452c96..98c0c1167d 100644 --- a/cli/command/system/version_test.go +++ b/cli/command/system/version_test.go @@ -5,7 +5,8 @@ import ( "strings" "testing" - "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/cli/command" + "github.com/docker/cli/internal/test" "github.com/docker/docker/api" "github.com/docker/docker/api/types" @@ -33,23 +34,14 @@ func fakeServerVersion(ctx context.Context) (types.Version, error) { }, nil } -func TestVersionWithDefaultOrchestrator(t *testing.T) { +func TestVersionWithOrchestrator(t *testing.T) { cli := test.NewFakeCli(&fakeClient{serverVersion: fakeServerVersion}) + cli.SetClientInfo(func() command.ClientInfo { return command.ClientInfo{Orchestrator: "swarm"} }) cmd := NewVersionCommand(cli) assert.NoError(t, cmd.Execute()) assert.Contains(t, cleanTabs(cli.OutBuffer().String()), "Orchestrator: swarm") } -func TestVersionWithOverridenOrchestrator(t *testing.T) { - cli := test.NewFakeCli(&fakeClient{serverVersion: fakeServerVersion}) - config := configfile.New("configfile") - config.Orchestrator = "Kubernetes" - cli.SetConfigFile(config) - cmd := NewVersionCommand(cli) - assert.NoError(t, cmd.Execute()) - assert.Contains(t, cleanTabs(cli.OutBuffer().String()), "Orchestrator: kubernetes") -} - func cleanTabs(line string) string { return strings.Join(strings.Fields(line), " ") } diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 0b48f464cc..20bcf393fa 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -195,7 +195,24 @@ type versionDetails interface { Client() client.APIClient ClientInfo() command.ClientInfo ServerInfo() command.ServerInfo - ClientInfo() command.ClientInfo +} + +func hideFeatureFlag(f *pflag.Flag, hasFeature bool, annotation string) { + if hasFeature { + return + } + if _, ok := f.Annotations[annotation]; ok { + f.Hidden = true + } +} + +func hideFeatureSubCommand(subcmd *cobra.Command, hasFeature bool, annotation string) { + if hasFeature { + return + } + if _, ok := subcmd.Annotations[annotation]; ok { + subcmd.Hidden = true + } } func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) { @@ -206,27 +223,10 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) { hasKubernetes := details.ClientInfo().HasKubernetes cmd.Flags().VisitAll(func(f *pflag.Flag) { - // hide experimental flags - if !hasExperimental { - if _, ok := f.Annotations["experimental"]; ok { - f.Hidden = true - } - } - if !hasExperimentalCLI { - if _, ok := f.Annotations["experimentalCLI"]; ok { - f.Hidden = true - } - } - if !hasKubernetes { - if _, ok := f.Annotations["kubernetes"]; ok { - f.Hidden = true - } - } else { - if _, ok := f.Annotations["swarm"]; ok { - f.Hidden = true - } - } - + hideFeatureFlag(f, hasExperimental, "experimental") + hideFeatureFlag(f, hasExperimentalCLI, "experimentalCLI") + hideFeatureFlag(f, hasKubernetes, "kubernetes") + hideFeatureFlag(f, !hasKubernetes, "swarm") // hide flags not supported by the server if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) { f.Hidden = true @@ -234,28 +234,10 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) { }) for _, subcmd := range cmd.Commands() { - // hide experimental subcommands - if !hasExperimental { - if _, ok := subcmd.Annotations["experimental"]; ok { - subcmd.Hidden = true - } - } - if !hasExperimentalCLI { - if _, ok := subcmd.Annotations["experimentalCLI"]; ok { - subcmd.Hidden = true - } - } - - if !hasKubernetes { - if _, ok := subcmd.Annotations["kubernetes"]; ok { - subcmd.Hidden = true - } - } else { - if _, ok := subcmd.Annotations["swarm"]; ok { - subcmd.Hidden = true - } - } - + hideFeatureSubCommand(subcmd, hasExperimental, "experimental") + hideFeatureSubCommand(subcmd, hasExperimentalCLI, "experimentalCLI") + hideFeatureSubCommand(subcmd, hasKubernetes, "kubernetes") + hideFeatureSubCommand(subcmd, !hasKubernetes, "swarm") // hide subcommands not supported by the server if subcmdVersion, ok := subcmd.Annotations["version"]; ok && versions.LessThan(clientVersion, subcmdVersion) { subcmd.Hidden = true @@ -279,7 +261,8 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error { clientVersion := details.Client().ClientVersion() osType := details.ServerInfo().OSType hasExperimental := details.ServerInfo().HasExperimental - hasKubernetes := details.ClientInfo().HasKubernetes() + hasKubernetes := details.ClientInfo().HasKubernetes + hasExperimentalCLI := details.ClientInfo().HasExperimental errs := []string{} diff --git a/internal/test/cli.go b/internal/test/cli.go index 3133d68960..5fffde64ee 100644 --- a/internal/test/cli.go +++ b/internal/test/cli.go @@ -15,6 +15,7 @@ import ( ) type notaryClientFuncType func(imgRefAndAuth trust.ImageRefAndAuth, actions []string) (notaryclient.Repository, error) +type clientInfoFuncType func() command.ClientInfo // FakeCli emulates the default DockerCli type FakeCli struct { @@ -26,6 +27,7 @@ type FakeCli struct { err *bytes.Buffer in *command.InStream server command.ServerInfo + clientInfoFunc clientInfoFuncType notaryClientFunc notaryClientFuncType } @@ -88,6 +90,19 @@ func (c *FakeCli) ServerInfo() command.ServerInfo { return c.server } +// ClientInfo returns client information +func (c *FakeCli) ClientInfo() command.ClientInfo { + if c.clientInfoFunc != nil { + return c.clientInfoFunc() + } + return c.DockerCli.ClientInfo() +} + +// SetClientInfo sets the internal getter for retrieving a ClientInfo +func (c *FakeCli) SetClientInfo(clientInfoFunc clientInfoFuncType) { + c.clientInfoFunc = clientInfoFunc +} + // OutBuffer returns the stdout buffer func (c *FakeCli) OutBuffer() *bytes.Buffer { return c.outBuffer