From 9ff9a91ab7f964f4e5042f94fe22dd50b5c3d832 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Thu, 1 Jun 2017 13:34:31 -0700 Subject: [PATCH] Remove `cli/flags` package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Moving the `common*.go` files in `cmd/dockerd` directly (it's the only place it's getting used) - Rename `cli/flags` to `cli/config` because it's the only thing left in that package 👼 Now, `integration-cli` does *truly* not depend on `cobra` stuff. Signed-off-by: Vincent Demeester --- cli/{flags => config}/configdir.go | 6 +- cli/flags/client.go | 13 ---- cmd/dockerd/daemon.go | 29 ++++---- cmd/dockerd/daemon_test.go | 24 +++---- cmd/dockerd/daemon_unix_test.go | 4 +- cmd/dockerd/docker.go | 19 +----- cli/flags/common.go => cmd/dockerd/options.go | 67 ++++++++++--------- .../dockerd/options_test.go | 10 +-- integration-cli/check_test.go | 4 +- integration-cli/docker_cli_push_test.go | 10 +-- integration-cli/trust_server_test.go | 4 +- 11 files changed, 84 insertions(+), 106 deletions(-) rename cli/{flags => config}/configdir.go (68%) delete mode 100644 cli/flags/client.go rename cli/flags/common.go => cmd/dockerd/options.go (62%) rename cli/flags/common_test.go => cmd/dockerd/options_test.go (80%) diff --git a/cli/flags/configdir.go b/cli/config/configdir.go similarity index 68% rename from cli/flags/configdir.go rename to cli/config/configdir.go index 5ac7b4fbb9..de22577811 100644 --- a/cli/flags/configdir.go +++ b/cli/config/configdir.go @@ -1,4 +1,4 @@ -package flags +package config import ( "os" @@ -12,9 +12,9 @@ var ( configFileDir = ".docker" ) -// ConfigurationDir returns the path to the configuration directory as specified by the DOCKER_CONFIG environment variable. +// Dir returns the path to the configuration directory as specified by the DOCKER_CONFIG environment variable. // TODO: this was copied from cli/config/configfile and should be removed once cmd/dockerd moves -func ConfigurationDir() string { +func Dir() string { return configDir } diff --git a/cli/flags/client.go b/cli/flags/client.go deleted file mode 100644 index 9b6940f6bd..0000000000 --- a/cli/flags/client.go +++ /dev/null @@ -1,13 +0,0 @@ -package flags - -// ClientOptions are the options used to configure the client cli -type ClientOptions struct { - Common *CommonOptions - ConfigDir string - Version bool -} - -// NewClientOptions returns a new ClientOptions -func NewClientOptions() *ClientOptions { - return &ClientOptions{Common: NewCommonOptions()} -} diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index 5898df77f6..6fe92af210 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -27,7 +27,6 @@ import ( systemrouter "github.com/docker/docker/api/server/router/system" "github.com/docker/docker/api/server/router/volume" "github.com/docker/docker/cli/debug" - cliflags "github.com/docker/docker/cli/flags" "github.com/docker/docker/daemon" "github.com/docker/docker/daemon/cluster" "github.com/docker/docker/daemon/config" @@ -65,14 +64,14 @@ func NewDaemonCli() *DaemonCli { return &DaemonCli{} } -func (cli *DaemonCli) start(opts daemonOptions) (err error) { +func (cli *DaemonCli) start(opts *daemonOptions) (err error) { stopc := make(chan bool) defer close(stopc) // warn from uuid package when running the daemon uuid.Loggerf = logrus.Warnf - opts.common.SetDefaultOptions(opts.flags) + opts.SetDefaultOptions(opts.flags) if cli.Config, err = loadDaemonCliConfig(opts); err != nil { return err @@ -358,20 +357,20 @@ func shutdownDaemon(d *daemon.Daemon) { } } -func loadDaemonCliConfig(opts daemonOptions) (*config.Config, error) { +func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) { conf := opts.daemonConfig flags := opts.flags - conf.Debug = opts.common.Debug - conf.Hosts = opts.common.Hosts - conf.LogLevel = opts.common.LogLevel - conf.TLS = opts.common.TLS - conf.TLSVerify = opts.common.TLSVerify + conf.Debug = opts.Debug + conf.Hosts = opts.Hosts + conf.LogLevel = opts.LogLevel + conf.TLS = opts.TLS + conf.TLSVerify = opts.TLSVerify conf.CommonTLSOptions = config.CommonTLSOptions{} - if opts.common.TLSOptions != nil { - conf.CommonTLSOptions.CAFile = opts.common.TLSOptions.CAFile - conf.CommonTLSOptions.CertFile = opts.common.TLSOptions.CertFile - conf.CommonTLSOptions.KeyFile = opts.common.TLSOptions.KeyFile + if opts.TLSOptions != nil { + conf.CommonTLSOptions.CAFile = opts.TLSOptions.CAFile + conf.CommonTLSOptions.CertFile = opts.TLSOptions.CertFile + conf.CommonTLSOptions.KeyFile = opts.TLSOptions.KeyFile } if conf.TrustKeyPath == "" { @@ -425,12 +424,12 @@ func loadDaemonCliConfig(opts daemonOptions) (*config.Config, error) { // Regardless of whether the user sets it to true or false, if they // specify TLSVerify at all then we need to turn on TLS - if conf.IsValueSet(cliflags.FlagTLSVerify) { + if conf.IsValueSet(FlagTLSVerify) { conf.TLS = true } // ensure that the log level is the one set after merging configurations - cliflags.SetLogLevel(conf.LogLevel) + setLogLevel(conf.LogLevel) return conf, nil } diff --git a/cmd/dockerd/daemon_test.go b/cmd/dockerd/daemon_test.go index c3ad617c78..7ae91fe1a7 100644 --- a/cmd/dockerd/daemon_test.go +++ b/cmd/dockerd/daemon_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/Sirupsen/logrus" - cliflags "github.com/docker/docker/cli/flags" "github.com/docker/docker/daemon/config" "github.com/docker/docker/pkg/testutil" "github.com/docker/docker/pkg/testutil/tempfile" @@ -13,13 +12,10 @@ import ( "github.com/stretchr/testify/require" ) -func defaultOptions(configFile string) daemonOptions { - opts := daemonOptions{ - daemonConfig: &config.Config{}, - flags: &pflag.FlagSet{}, - common: cliflags.NewCommonOptions(), - } - opts.common.InstallFlags(opts.flags) +func defaultOptions(configFile string) *daemonOptions { + opts := newDaemonOptions(&config.Config{}) + opts.flags = &pflag.FlagSet{} + opts.InstallFlags(opts.flags) installConfigFlags(opts.daemonConfig, opts.flags) opts.flags.StringVar(&opts.configFile, "config-file", defaultDaemonConfigFile, "") opts.configFile = configFile @@ -28,7 +24,7 @@ func defaultOptions(configFile string) daemonOptions { func TestLoadDaemonCliConfigWithoutOverriding(t *testing.T) { opts := defaultOptions("") - opts.common.Debug = true + opts.Debug = true loadedConfig, err := loadDaemonCliConfig(opts) require.NoError(t, err) @@ -40,8 +36,8 @@ func TestLoadDaemonCliConfigWithoutOverriding(t *testing.T) { func TestLoadDaemonCliConfigWithTLS(t *testing.T) { opts := defaultOptions("") - opts.common.TLSOptions.CAFile = "/tmp/ca.pem" - opts.common.TLS = true + opts.TLSOptions.CAFile = "/tmp/ca.pem" + opts.TLS = true loadedConfig, err := loadDaemonCliConfig(opts) require.NoError(t, err) @@ -70,7 +66,7 @@ func TestLoadDaemonCliConfigWithTLSVerify(t *testing.T) { defer tempFile.Remove() opts := defaultOptions(tempFile.Name()) - opts.common.TLSOptions.CAFile = "/tmp/ca.pem" + opts.TLSOptions.CAFile = "/tmp/ca.pem" loadedConfig, err := loadDaemonCliConfig(opts) require.NoError(t, err) @@ -83,7 +79,7 @@ func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) { defer tempFile.Remove() opts := defaultOptions(tempFile.Name()) - opts.common.TLSOptions.CAFile = "/tmp/ca.pem" + opts.TLSOptions.CAFile = "/tmp/ca.pem" loadedConfig, err := loadDaemonCliConfig(opts) require.NoError(t, err) @@ -96,7 +92,7 @@ func TestLoadDaemonCliConfigWithoutTLSVerify(t *testing.T) { defer tempFile.Remove() opts := defaultOptions(tempFile.Name()) - opts.common.TLSOptions.CAFile = "/tmp/ca.pem" + opts.TLSOptions.CAFile = "/tmp/ca.pem" loadedConfig, err := loadDaemonCliConfig(opts) require.NoError(t, err) diff --git a/cmd/dockerd/daemon_unix_test.go b/cmd/dockerd/daemon_unix_test.go index 4aaa758d53..18761493d0 100644 --- a/cmd/dockerd/daemon_unix_test.go +++ b/cmd/dockerd/daemon_unix_test.go @@ -20,8 +20,8 @@ func TestLoadDaemonCliConfigWithDaemonFlags(t *testing.T) { defer tempFile.Remove() opts := defaultOptions(tempFile.Name()) - opts.common.Debug = true - opts.common.LogLevel = "info" + opts.Debug = true + opts.LogLevel = "info" assert.NoError(t, opts.flags.Set("selinux-enabled", "true")) loadedConfig, err := loadDaemonCliConfig(opts) diff --git a/cmd/dockerd/docker.go b/cmd/dockerd/docker.go index c0bc965674..8a5c8f5433 100644 --- a/cmd/dockerd/docker.go +++ b/cmd/dockerd/docker.go @@ -8,28 +8,15 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/docker/cli" - cliflags "github.com/docker/docker/cli/flags" "github.com/docker/docker/daemon/config" "github.com/docker/docker/dockerversion" "github.com/docker/docker/pkg/reexec" "github.com/docker/docker/pkg/term" "github.com/spf13/cobra" - "github.com/spf13/pflag" ) -type daemonOptions struct { - version bool - configFile string - daemonConfig *config.Config - common *cliflags.CommonOptions - flags *pflag.FlagSet -} - func newDaemonCommand() *cobra.Command { - opts := daemonOptions{ - daemonConfig: config.New(), - common: cliflags.NewCommonOptions(), - } + opts := newDaemonOptions(config.New()) cmd := &cobra.Command{ Use: "dockerd [OPTIONS]", @@ -47,14 +34,14 @@ func newDaemonCommand() *cobra.Command { flags := cmd.Flags() flags.BoolVarP(&opts.version, "version", "v", false, "Print version information and quit") flags.StringVar(&opts.configFile, "config-file", defaultDaemonConfigFile, "Daemon configuration file") - opts.common.InstallFlags(flags) + opts.InstallFlags(flags) installConfigFlags(opts.daemonConfig, flags) installServiceFlags(flags) return cmd } -func runDaemon(opts daemonOptions) error { +func runDaemon(opts *daemonOptions) error { if opts.version { showVersion() return nil diff --git a/cli/flags/common.go b/cmd/dockerd/options.go similarity index 62% rename from cli/flags/common.go rename to cmd/dockerd/options.go index 38a793c402..629b0223e9 100644 --- a/cli/flags/common.go +++ b/cmd/dockerd/options.go @@ -1,4 +1,4 @@ -package flags +package main import ( "fmt" @@ -6,6 +6,8 @@ import ( "path/filepath" "github.com/Sirupsen/logrus" + cliconfig "github.com/docker/docker/cli/config" + "github.com/docker/docker/daemon/config" "github.com/docker/docker/opts" "github.com/docker/go-connections/tlsconfig" "github.com/spf13/pflag" @@ -27,64 +29,69 @@ var ( dockerTLSVerify = os.Getenv("DOCKER_TLS_VERIFY") != "" ) -// CommonOptions are options common to both the client and the daemon. -type CommonOptions struct { - Debug bool - Hosts []string - LogLevel string - TLS bool - TLSVerify bool - TLSOptions *tlsconfig.Options +type daemonOptions struct { + version bool + configFile string + daemonConfig *config.Config + flags *pflag.FlagSet + Debug bool + Hosts []string + LogLevel string + TLS bool + TLSVerify bool + TLSOptions *tlsconfig.Options } -// NewCommonOptions returns a new CommonOptions -func NewCommonOptions() *CommonOptions { - return &CommonOptions{} +// newDaemonOptions returns a new daemonFlags +func newDaemonOptions(config *config.Config) *daemonOptions { + return &daemonOptions{ + daemonConfig: config, + } } // InstallFlags adds flags for the common options on the FlagSet -func (commonOpts *CommonOptions) InstallFlags(flags *pflag.FlagSet) { +func (o *daemonOptions) InstallFlags(flags *pflag.FlagSet) { if dockerCertPath == "" { - dockerCertPath = ConfigurationDir() + dockerCertPath = cliconfig.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", false, "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", false, "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") - hostOpt := opts.NewNamedListOptsRef("hosts", &commonOpts.Hosts, opts.ValidateHost) + hostOpt := opts.NewNamedListOptsRef("hosts", &o.Hosts, opts.ValidateHost) flags.VarP(hostOpt, "host", "H", "Daemon socket(s) to connect to") } // SetDefaultOptions sets default values for options after flag parsing is // complete -func (commonOpts *CommonOptions) SetDefaultOptions(flags *pflag.FlagSet) { +func (o *daemonOptions) 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. @@ -101,8 +108,8 @@ func (commonOpts *CommonOptions) SetDefaultOptions(flags *pflag.FlagSet) { } } -// SetLogLevel sets the logrus logging level -func SetLogLevel(logLevel string) { +// setLogLevel sets the logrus logging level +func setLogLevel(logLevel string) { if logLevel != "" { lvl, err := logrus.ParseLevel(logLevel) if err != nil { diff --git a/cli/flags/common_test.go b/cmd/dockerd/options_test.go similarity index 80% rename from cli/flags/common_test.go rename to cmd/dockerd/options_test.go index 52aa3d8b54..c3298a0ac6 100644 --- a/cli/flags/common_test.go +++ b/cmd/dockerd/options_test.go @@ -1,16 +1,18 @@ -package flags +package main import ( "path/filepath" "testing" + cliconfig "github.com/docker/docker/cli/config" + "github.com/docker/docker/daemon/config" "github.com/spf13/pflag" "github.com/stretchr/testify/assert" ) func TestCommonOptionsInstallFlags(t *testing.T) { flags := pflag.NewFlagSet("testing", pflag.ContinueOnError) - opts := NewCommonOptions() + opts := newDaemonOptions(&config.Config{}) opts.InstallFlags(flags) err := flags.Parse([]string{ @@ -25,12 +27,12 @@ func TestCommonOptionsInstallFlags(t *testing.T) { } func defaultPath(filename string) string { - return filepath.Join(ConfigurationDir(), filename) + return filepath.Join(cliconfig.Dir(), filename) } func TestCommonOptionsInstallFlagsWithDefaults(t *testing.T) { flags := pflag.NewFlagSet("testing", pflag.ContinueOnError) - opts := NewCommonOptions() + opts := newDaemonOptions(&config.Config{}) opts.InstallFlags(flags) err := flags.Parse([]string{}) diff --git a/integration-cli/check_test.go b/integration-cli/check_test.go index a1927e4c9a..cc3b80c94f 100644 --- a/integration-cli/check_test.go +++ b/integration-cli/check_test.go @@ -12,7 +12,7 @@ import ( "testing" "github.com/docker/docker/api/types/swarm" - "github.com/docker/docker/cli/flags" + "github.com/docker/docker/cli/config" "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/integration-cli/cli/build/fakestorage" "github.com/docker/docker/integration-cli/daemon" @@ -406,7 +406,7 @@ func (s *DockerTrustSuite) TearDownTest(c *check.C) { } // Remove trusted keys and metadata after test - os.RemoveAll(filepath.Join(flags.ConfigurationDir(), "trust")) + os.RemoveAll(filepath.Join(config.Dir(), "trust")) s.ds.TearDownTest(c) } diff --git a/integration-cli/docker_cli_push_test.go b/integration-cli/docker_cli_push_test.go index f0548e6907..2ae206df7d 100644 --- a/integration-cli/docker_cli_push_test.go +++ b/integration-cli/docker_cli_push_test.go @@ -12,7 +12,7 @@ import ( "sync" "github.com/docker/distribution/reference" - "github.com/docker/docker/cli/flags" + "github.com/docker/docker/cli/config" "github.com/docker/docker/integration-cli/checker" "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/integration-cli/cli/build" @@ -294,7 +294,7 @@ func (s *DockerTrustSuite) TestTrustedPush(c *check.C) { }) // Assert that we rotated the snapshot key to the server by checking our local keystore - contents, err := ioutil.ReadDir(filepath.Join(flags.ConfigurationDir(), "trust/private/tuf_keys", privateRegistryURL, "dockerclitrusted/pushtest")) + contents, err := ioutil.ReadDir(filepath.Join(config.Dir(), "trust/private/tuf_keys", privateRegistryURL, "dockerclitrusted/pushtest")) c.Assert(err, check.IsNil, check.Commentf("Unable to read local tuf key files")) // Check that we only have 1 key (targets key) c.Assert(contents, checker.HasLen, 1) @@ -399,7 +399,7 @@ func (s *DockerTrustSuite) TestTrustedPushWithReleasesDelegationOnly(c *check.C) s.assertTargetNotInRoles(c, repoName, "latest", "targets") // Try pull after push - os.RemoveAll(filepath.Join(flags.ConfigurationDir(), "trust")) + os.RemoveAll(filepath.Join(config.Dir(), "trust")) cli.Docker(cli.Args("pull", targetName), trustedCmd).Assert(c, icmd.Expected{ Out: "Status: Image is up to date", @@ -436,7 +436,7 @@ func (s *DockerTrustSuite) TestTrustedPushSignsAllFirstLevelRolesWeHaveKeysFor(c s.assertTargetNotInRoles(c, repoName, "latest", "targets") // Try pull after push - os.RemoveAll(filepath.Join(flags.ConfigurationDir(), "trust")) + os.RemoveAll(filepath.Join(config.Dir(), "trust")) // pull should fail because none of these are the releases role cli.Docker(cli.Args("pull", targetName), trustedCmd).Assert(c, icmd.Expected{ @@ -472,7 +472,7 @@ func (s *DockerTrustSuite) TestTrustedPushSignsForRolesWithKeysAndValidPaths(c * s.assertTargetNotInRoles(c, repoName, "latest", "targets") // Try pull after push - os.RemoveAll(filepath.Join(flags.ConfigurationDir(), "trust")) + os.RemoveAll(filepath.Join(config.Dir(), "trust")) // pull should fail because none of these are the releases role cli.Docker(cli.Args("pull", targetName), trustedCmd).Assert(c, icmd.Expected{ diff --git a/integration-cli/trust_server_test.go b/integration-cli/trust_server_test.go index d1eb62e794..e3f0674cf3 100644 --- a/integration-cli/trust_server_test.go +++ b/integration-cli/trust_server_test.go @@ -11,7 +11,7 @@ import ( "strings" "time" - "github.com/docker/docker/cli/flags" + cliconfig "github.com/docker/docker/cli/config" "github.com/docker/docker/integration-cli/checker" "github.com/docker/docker/integration-cli/cli" icmd "github.com/docker/docker/pkg/testutil/cmd" @@ -108,7 +108,7 @@ func newTestNotary(c *check.C) (*testNotary, error) { "skipTLSVerify": true } }` - if _, err = fmt.Fprintf(clientConfig, template, filepath.Join(flags.ConfigurationDir(), "trust"), notaryURL); err != nil { + if _, err = fmt.Fprintf(clientConfig, template, filepath.Join(cliconfig.Dir(), "trust"), notaryURL); err != nil { os.RemoveAll(tmp) return nil, err }