From 3499669e1860214f23c1d77af8b5cd7663ee1c5f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 4 Nov 2022 11:58:11 +0100 Subject: [PATCH] cli/flags: merge CommonOptions into ClientOptions CommonOptions was inherited from when the cli and daemon were in the same repository, and some options would be shared between them. That's no longer the case, and some options are even "incorrect" (for example, while the daemon can be configured to run on multiple hosts, the CLI can only connect with a single host / connection). This patch does not (yet) address that, but merges the CommonOptions into the ClientOptions. An alias is created for the old type, although it doesn't appear there's any external consumers using the CommonOptions type (or its constructor). Signed-off-by: Sebastiaan van Stijn --- cli/cobra.go | 4 +- cli/command/cli.go | 14 +++--- cli/command/cli_test.go | 10 ++--- cli/command/defaultcontextstore.go | 2 +- cli/command/defaultcontextstore_test.go | 2 +- cli/flags/client.go | 12 ------ cli/flags/{common.go => options.go} | 43 ++++++++++--------- cli/flags/options_deprecated.go | 11 +++++ cli/flags/{common_test.go => options_test.go} | 8 ++-- cmd/docker/builder_test.go | 2 +- 10 files changed, 54 insertions(+), 54 deletions(-) delete mode 100644 cli/flags/client.go rename cli/flags/{common.go => options.go} (76%) create mode 100644 cli/flags/options_deprecated.go rename cli/flags/{common_test.go => options_test.go} (86%) diff --git a/cli/cobra.go b/cli/cobra.go index f7d38f8500..7cf7e810d9 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -28,7 +28,7 @@ func setupCommonRootCommand(rootCmd *cobra.Command) (*cliflags.ClientOptions, *p flags := rootCmd.Flags() flags.StringVar(&opts.ConfigDir, "config", config.Dir(), "Location of client config files") - opts.Common.InstallFlags(flags) + opts.InstallFlags(flags) cobra.AddTemplateFunc("add", func(a, b int) int { return a + b }) cobra.AddTemplateFunc("hasAliases", hasAliases) @@ -172,7 +172,7 @@ func (tcmd *TopLevelCommand) HandleGlobalFlags() (*cobra.Command, []string, erro // Initialize finalises global option parsing and initializes the docker client. func (tcmd *TopLevelCommand) Initialize(ops ...command.InitializeOpt) error { - tcmd.opts.Common.SetDefaultOptions(tcmd.flags) + tcmd.opts.SetDefaultOptions(tcmd.flags) return tcmd.dockerCli.Initialize(tcmd.opts, ops...) } diff --git a/cli/command/cli.go b/cli/command/cli.go index b31c35785d..b5c1ccd996 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -203,13 +203,13 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize return err } } - cliflags.SetLogLevel(opts.Common.LogLevel) + cliflags.SetLogLevel(opts.LogLevel) if opts.ConfigDir != "" { config.SetDir(opts.ConfigDir) } - if opts.Common.Debug { + if opts.Debug { debug.Enable() } @@ -219,10 +219,10 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize cli.contextStore = &ContextStoreWithDefault{ Store: baseContextStore, Resolver: func() (*DefaultContext, error) { - return ResolveDefaultContext(opts.Common, cli.contextStoreConfig) + return ResolveDefaultContext(opts, cli.contextStoreConfig) }, } - cli.currentContext, err = resolveContextName(opts.Common, cli.configFile, cli.contextStore) + cli.currentContext, err = resolveContextName(opts, cli.configFile, cli.contextStore) if err != nil { return err } @@ -242,7 +242,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize } // NewAPIClientFromFlags creates a new APIClient from command line flags -func NewAPIClientFromFlags(opts *cliflags.CommonOptions, configFile *configfile.ConfigFile) (client.APIClient, error) { +func NewAPIClientFromFlags(opts *cliflags.ClientOptions, configFile *configfile.ConfigFile) (client.APIClient, error) { storeConfig := DefaultContextStoreConfig() contextStore := &ContextStoreWithDefault{ Store: store.New(config.ContextStoreDir(), storeConfig), @@ -288,7 +288,7 @@ func resolveDockerEndpoint(s store.Reader, contextName string) (docker.Endpoint, } // Resolve the Docker endpoint for the default context (based on config, env vars and CLI flags) -func resolveDefaultDockerEndpoint(opts *cliflags.CommonOptions) (docker.Endpoint, error) { +func resolveDefaultDockerEndpoint(opts *cliflags.ClientOptions) (docker.Endpoint, error) { host, err := getServerHost(opts.Hosts, opts.TLSOptions) if err != nil { return docker.Endpoint{}, err @@ -445,7 +445,7 @@ func UserAgent() string { // - if DOCKER_CONTEXT is set, use this value // - if Config file has a globally set "CurrentContext", use this value // - fallbacks to default HOST, uses TLS config from flags/env vars -func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile, contextstore store.Reader) (string, error) { +func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigFile, contextstore store.Reader) (string, error) { if opts.Context != "" && len(opts.Hosts) > 0 { return "", errors.New("Conflicting options: either specify --host or --context, not both") } diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go index c6d106f94e..9f4cc99ae3 100644 --- a/cli/command/cli_test.go +++ b/cli/command/cli_test.go @@ -31,7 +31,7 @@ func TestNewAPIClientFromFlags(t *testing.T) { if runtime.GOOS == "windows" { host = "npipe://./" } - opts := &flags.CommonOptions{Hosts: []string{host}} + opts := &flags.ClientOptions{Hosts: []string{host}} apiClient, err := NewAPIClientFromFlags(opts, &configfile.ConfigFile{}) assert.NilError(t, err) assert.Equal(t, apiClient.DaemonHost(), host) @@ -44,7 +44,7 @@ func TestNewAPIClientFromFlagsForDefaultSchema(t *testing.T) { if runtime.GOOS == "windows" { slug = "tcp://127.0.0.1" } - opts := &flags.CommonOptions{Hosts: []string{host}} + opts := &flags.ClientOptions{Hosts: []string{host}} apiClient, err := NewAPIClientFromFlags(opts, &configfile.ConfigFile{}) assert.NilError(t, err) assert.Equal(t, apiClient.DaemonHost(), slug+host) @@ -62,7 +62,7 @@ func TestNewAPIClientFromFlagsWithCustomHeaders(t *testing.T) { })) defer ts.Close() host := strings.Replace(ts.URL, "http://", "tcp://", 1) - opts := &flags.CommonOptions{Hosts: []string{host}} + opts := &flags.ClientOptions{Hosts: []string{host}} configFile := &configfile.ConfigFile{ HTTPHeaders: map[string]string{ "My-Header": "Custom-Value", @@ -91,7 +91,7 @@ func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) { t.Setenv("DOCKER_API_VERSION", customVersion) t.Setenv("DOCKER_HOST", ":2375") - opts := &flags.CommonOptions{} + opts := &flags.ClientOptions{} configFile := &configfile.ConfigFile{} apiclient, err := NewAPIClientFromFlags(opts, configFile) assert.NilError(t, err) @@ -191,7 +191,7 @@ func TestInitializeFromClientHangs(t *testing.T) { ts.Start() defer ts.Close() - opts := &flags.CommonOptions{Hosts: []string{fmt.Sprintf("unix://%s", socket)}} + opts := &flags.ClientOptions{Hosts: []string{fmt.Sprintf("unix://%s", socket)}} configFile := &configfile.ConfigFile{} apiClient, err := NewAPIClientFromFlags(opts, configFile) assert.NilError(t, err) diff --git a/cli/command/defaultcontextstore.go b/cli/command/defaultcontextstore.go index 3677f711e5..95f89fc6ff 100644 --- a/cli/command/defaultcontextstore.go +++ b/cli/command/defaultcontextstore.go @@ -42,7 +42,7 @@ type EndpointDefaultResolver interface { } // ResolveDefaultContext creates a Metadata for the current CLI invocation parameters -func ResolveDefaultContext(opts *cliflags.CommonOptions, config store.Config) (*DefaultContext, error) { +func ResolveDefaultContext(opts *cliflags.ClientOptions, config store.Config) (*DefaultContext, error) { contextTLSData := store.ContextTLSData{ Endpoints: make(map[string]store.EndpointTLSData), } diff --git a/cli/command/defaultcontextstore_test.go b/cli/command/defaultcontextstore_test.go index a3b1bff583..a368135ff7 100644 --- a/cli/command/defaultcontextstore_test.go +++ b/cli/command/defaultcontextstore_test.go @@ -56,7 +56,7 @@ func TestDefaultContextInitializer(t *testing.T) { assert.NilError(t, err) t.Setenv("DOCKER_HOST", "ssh://someswarmserver") cli.configFile = &configfile.ConfigFile{} - ctx, err := ResolveDefaultContext(&cliflags.CommonOptions{ + ctx, err := ResolveDefaultContext(&cliflags.ClientOptions{ TLS: true, TLSOptions: &tlsconfig.Options{ CAFile: "./testdata/ca.pem", diff --git a/cli/flags/client.go b/cli/flags/client.go deleted file mode 100644 index c57879e6a7..0000000000 --- a/cli/flags/client.go +++ /dev/null @@ -1,12 +0,0 @@ -package flags - -// ClientOptions are the options used to configure the client cli -type ClientOptions struct { - Common *CommonOptions - ConfigDir string -} - -// NewClientOptions returns a new ClientOptions -func NewClientOptions() *ClientOptions { - return &ClientOptions{Common: NewCommonOptions()} -} diff --git a/cli/flags/common.go b/cli/flags/options.go similarity index 76% rename from cli/flags/common.go rename to cli/flags/options.go index fbe686c5d2..3d318cf1f6 100644 --- a/cli/flags/common.go +++ b/cli/flags/options.go @@ -43,8 +43,8 @@ var ( dockerTLS = os.Getenv("DOCKER_TLS") != "" ) -// CommonOptions are options common to both the client and the daemon. -type CommonOptions struct { +// ClientOptions are the options used to configure the client cli. +type ClientOptions struct { Debug bool Hosts []string LogLevel string @@ -52,59 +52,60 @@ type CommonOptions struct { TLSVerify bool TLSOptions *tlsconfig.Options Context string + ConfigDir string } -// NewCommonOptions returns a new CommonOptions -func NewCommonOptions() *CommonOptions { - return &CommonOptions{} +// NewClientOptions returns a new ClientOptions. +func NewClientOptions() *ClientOptions { + return &ClientOptions{} } // InstallFlags adds flags for the common options on the FlagSet -func (commonOpts *CommonOptions) InstallFlags(flags *pflag.FlagSet) { +func (o *ClientOptions) InstallFlags(flags *pflag.FlagSet) { if dockerCertPath == "" { dockerCertPath = config.Dir() } - flags.BoolVarP(&commonOpts.Debug, "debug", "D", false, "Enable debug mode") - flags.StringVarP(&commonOpts.LogLevel, "log-level", "l", "info", `Set the logging level ("debug"|"info"|"warn"|"error"|"fatal")`) - flags.BoolVar(&commonOpts.TLS, "tls", dockerTLS, "Use TLS; implied by --tlsverify") - flags.BoolVar(&commonOpts.TLSVerify, FlagTLSVerify, dockerTLSVerify, "Use TLS and verify the remote") + flags.BoolVarP(&o.Debug, "debug", "D", false, "Enable debug mode") + flags.StringVarP(&o.LogLevel, "log-level", "l", "info", `Set the logging level ("debug"|"info"|"warn"|"error"|"fatal")`) + flags.BoolVar(&o.TLS, "tls", dockerTLS, "Use TLS; implied by --tlsverify") + flags.BoolVar(&o.TLSVerify, FlagTLSVerify, dockerTLSVerify, "Use TLS and verify the remote") // TODO use flag flags.String("identity"}, "i", "", "Path to libtrust key file") - commonOpts.TLSOptions = &tlsconfig.Options{ + o.TLSOptions = &tlsconfig.Options{ CAFile: filepath.Join(dockerCertPath, DefaultCaFile), CertFile: filepath.Join(dockerCertPath, DefaultCertFile), KeyFile: filepath.Join(dockerCertPath, DefaultKeyFile), } - tlsOptions := commonOpts.TLSOptions + tlsOptions := o.TLSOptions flags.Var(opts.NewQuotedString(&tlsOptions.CAFile), "tlscacert", "Trust certs signed only by this CA") flags.Var(opts.NewQuotedString(&tlsOptions.CertFile), "tlscert", "Path to TLS certificate file") flags.Var(opts.NewQuotedString(&tlsOptions.KeyFile), "tlskey", "Path to TLS key file") // opts.ValidateHost is not used here, so as to allow connection helpers - hostOpt := opts.NewNamedListOptsRef("hosts", &commonOpts.Hosts, nil) + hostOpt := opts.NewNamedListOptsRef("hosts", &o.Hosts, nil) flags.VarP(hostOpt, "host", "H", "Daemon socket(s) to connect to") - flags.StringVarP(&commonOpts.Context, "context", "c", "", + flags.StringVarP(&o.Context, "context", "c", "", `Name of the context to use to connect to the daemon (overrides `+client.EnvOverrideHost+` env var and default context set with "docker context use")`) } // SetDefaultOptions sets default values for options after flag parsing is // complete -func (commonOpts *CommonOptions) SetDefaultOptions(flags *pflag.FlagSet) { +func (o *ClientOptions) SetDefaultOptions(flags *pflag.FlagSet) { // Regardless of whether the user sets it to true or false, if they // specify --tlsverify at all then we need to turn on TLS // TLSVerify can be true even if not set due to DOCKER_TLS_VERIFY env var, so we need // to check that here as well - if flags.Changed(FlagTLSVerify) || commonOpts.TLSVerify { - commonOpts.TLS = true + if flags.Changed(FlagTLSVerify) || o.TLSVerify { + o.TLS = true } - if !commonOpts.TLS { - commonOpts.TLSOptions = nil + if !o.TLS { + o.TLSOptions = nil } else { - tlsOptions := commonOpts.TLSOptions - tlsOptions.InsecureSkipVerify = !commonOpts.TLSVerify + tlsOptions := o.TLSOptions + tlsOptions.InsecureSkipVerify = !o.TLSVerify // Reset CertFile and KeyFile to empty string if the user did not specify // the respective flags and the respective default files were not found. diff --git a/cli/flags/options_deprecated.go b/cli/flags/options_deprecated.go new file mode 100644 index 0000000000..97fb7e4e6a --- /dev/null +++ b/cli/flags/options_deprecated.go @@ -0,0 +1,11 @@ +package flags + +// CommonOptions are options common to both the client and the daemon. +// +// Deprecated: use [ClientOptions]. +type CommonOptions = ClientOptions + +// NewCommonOptions returns a new CommonOptions +// +// Deprecated: use [NewClientOptions]. +var NewCommonOptions = NewClientOptions diff --git a/cli/flags/common_test.go b/cli/flags/options_test.go similarity index 86% rename from cli/flags/common_test.go rename to cli/flags/options_test.go index f248f8e9fc..1fef741757 100644 --- a/cli/flags/common_test.go +++ b/cli/flags/options_test.go @@ -10,9 +10,9 @@ import ( is "gotest.tools/v3/assert/cmp" ) -func TestCommonOptionsInstallFlags(t *testing.T) { +func TestClientOptionsInstallFlags(t *testing.T) { flags := pflag.NewFlagSet("testing", pflag.ContinueOnError) - opts := NewCommonOptions() + opts := NewClientOptions() opts.InstallFlags(flags) err := flags.Parse([]string{ @@ -30,9 +30,9 @@ func defaultPath(filename string) string { return filepath.Join(config.Dir(), filename) } -func TestCommonOptionsInstallFlagsWithDefaults(t *testing.T) { +func TestClientOptionsInstallFlagsWithDefaults(t *testing.T) { flags := pflag.NewFlagSet("testing", pflag.ContinueOnError) - opts := NewCommonOptions() + opts := NewClientOptions() opts.InstallFlags(flags) err := flags.Parse([]string{}) diff --git a/cmd/docker/builder_test.go b/cmd/docker/builder_test.go index 88ee253fe4..24745136b9 100644 --- a/cmd/docker/builder_test.go +++ b/cmd/docker/builder_test.go @@ -79,7 +79,7 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD })) } opts := flags.NewClientOptions() - opts.Common.Context = tt.context + opts.Context = tt.context assert.NilError(t, dockerCli.Initialize(opts)) }