From 7f207f3f957ed3f5129aeb22bef2a429c14caf22 Mon Sep 17 00:00:00 2001 From: Silvin Lubecki Date: Mon, 28 Jan 2019 14:52:58 +0100 Subject: [PATCH] Introduce functional arguments to NewDockerCli for a more stable API. Signed-off-by: Silvin Lubecki --- cli/command/cli.go | 63 +++++++++++++++++++++------ cli/command/cli_options.go | 89 ++++++++++++++++++++++++++++++++++++++ cli/command/cli_test.go | 45 +++++++++++++++++++ cmd/docker/docker.go | 27 ++++-------- cmd/docker/docker_test.go | 22 ++++++---- docs/yaml/generate.go | 7 +-- man/generate.go | 7 +-- 7 files changed, 214 insertions(+), 46 deletions(-) create mode 100644 cli/command/cli_options.go diff --git a/cli/command/cli.go b/cli/command/cli.go index 5da0f01ddc..33eeb10949 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -19,12 +19,15 @@ import ( cliflags "github.com/docker/cli/cli/flags" manifeststore "github.com/docker/cli/cli/manifest/store" registryclient "github.com/docker/cli/cli/registry/client" + "github.com/docker/cli/cli/streams" "github.com/docker/cli/cli/trust" + "github.com/docker/cli/internal/containerizedengine" dopts "github.com/docker/cli/opts" clitypes "github.com/docker/cli/types" "github.com/docker/docker/api/types" registrytypes "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" + "github.com/docker/docker/pkg/term" "github.com/docker/go-connections/tlsconfig" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -35,18 +38,19 @@ import ( // Streams is an interface which exposes the standard input and output streams type Streams interface { - In() *InStream - Out() *OutStream + In() *streams.In + Out() *streams.Out Err() io.Writer } // Cli represents the docker command line client. type Cli interface { Client() client.APIClient - Out() *OutStream + Out() *streams.Out Err() io.Writer - In() *InStream - SetIn(in *InStream) + In() *streams.In + SetIn(in *streams.In) + Apply(ops ...DockerCliOption) error ConfigFile() *configfile.ConfigFile ServerInfo() ServerInfo ClientInfo() ClientInfo @@ -66,8 +70,8 @@ type Cli interface { // Instances of the client can be returned from NewDockerCli. type DockerCli struct { configFile *configfile.ConfigFile - in *InStream - out *OutStream + in *streams.In + out *streams.Out err io.Writer client client.APIClient serverInfo ServerInfo @@ -96,7 +100,7 @@ func (cli *DockerCli) Client() client.APIClient { } // Out returns the writer used for stdout -func (cli *DockerCli) Out() *OutStream { +func (cli *DockerCli) Out() *streams.Out { return cli.out } @@ -106,12 +110,12 @@ func (cli *DockerCli) Err() io.Writer { } // SetIn sets the reader used for stdin -func (cli *DockerCli) SetIn(in *InStream) { +func (cli *DockerCli) SetIn(in *streams.In) { cli.in = in } // In returns the reader used for stdin -func (cli *DockerCli) In() *InStream { +func (cli *DockerCli) In() *streams.In { return cli.in } @@ -393,6 +397,16 @@ func (cli *DockerCli) DockerEndpoint() docker.Endpoint { return cli.dockerEndpoint } +// Apply all the operation on the cli +func (cli *DockerCli) Apply(ops ...DockerCliOption) error { + for _, op := range ops { + if err := op(cli); err != nil { + return err + } + } + return nil +} + // ServerInfo stores details about the supported features and platform of the // server type ServerInfo struct { @@ -407,9 +421,32 @@ type ClientInfo struct { DefaultVersion string } -// NewDockerCli returns a DockerCli instance with IO output and error streams set by in, out and err. -func NewDockerCli(in io.ReadCloser, out, err io.Writer, isTrusted bool, containerizedFn func(string) (clitypes.ContainerizedClient, error)) *DockerCli { - return &DockerCli{in: NewInStream(in), out: NewOutStream(out), err: err, contentTrust: isTrusted, newContainerizeClient: containerizedFn} +// NewDockerCli returns a DockerCli instance with all operators applied on it. +// It applies by default the standard streams, the content trust from +// environment and the default containerized client constructor operations. +func NewDockerCli(ops ...DockerCliOption) (*DockerCli, error) { + cli := &DockerCli{} + defaultOps := []DockerCliOption{ + WithContentTrustFromEnv(), + WithContainerizedClient(containerizedengine.NewClient), + } + ops = append(defaultOps, ops...) + if err := cli.Apply(ops...); err != nil { + return nil, err + } + if cli.out == nil || cli.in == nil || cli.err == nil { + stdin, stdout, stderr := term.StdStreams() + if cli.in == nil { + cli.in = streams.NewIn(stdin) + } + if cli.out == nil { + cli.out = streams.NewOut(stdout) + } + if cli.err == nil { + cli.err = stderr + } + } + return cli, nil } func getServerHost(hosts []string, tlsOptions *tlsconfig.Options) (string, error) { diff --git a/cli/command/cli_options.go b/cli/command/cli_options.go new file mode 100644 index 0000000000..43db626211 --- /dev/null +++ b/cli/command/cli_options.go @@ -0,0 +1,89 @@ +package command + +import ( + "io" + "os" + "strconv" + + "github.com/docker/cli/cli/streams" + clitypes "github.com/docker/cli/types" + "github.com/docker/docker/pkg/term" +) + +// DockerCliOption applies a modification on a DockerCli. +type DockerCliOption func(cli *DockerCli) error + +// WithStandardStreams sets a cli in, out and err streams with the standard streams. +func WithStandardStreams() DockerCliOption { + return func(cli *DockerCli) error { + // Set terminal emulation based on platform as required. + stdin, stdout, stderr := term.StdStreams() + cli.in = streams.NewIn(stdin) + cli.out = streams.NewOut(stdout) + cli.err = stderr + return nil + } +} + +// WithCombinedStreams uses the same stream for the output and error streams. +func WithCombinedStreams(combined io.Writer) DockerCliOption { + return func(cli *DockerCli) error { + cli.out = streams.NewOut(combined) + cli.err = combined + return nil + } +} + +// WithInputStream sets a cli input stream. +func WithInputStream(in io.ReadCloser) DockerCliOption { + return func(cli *DockerCli) error { + cli.in = streams.NewIn(in) + return nil + } +} + +// WithOutputStream sets a cli output stream. +func WithOutputStream(out io.Writer) DockerCliOption { + return func(cli *DockerCli) error { + cli.out = streams.NewOut(out) + return nil + } +} + +// WithErrorStream sets a cli error stream. +func WithErrorStream(err io.Writer) DockerCliOption { + return func(cli *DockerCli) error { + cli.err = err + return nil + } +} + +// WithContentTrustFromEnv enables content trust on a cli from environment variable DOCKER_CONTENT_TRUST value. +func WithContentTrustFromEnv() DockerCliOption { + return func(cli *DockerCli) error { + cli.contentTrust = false + if e := os.Getenv("DOCKER_CONTENT_TRUST"); e != "" { + if t, err := strconv.ParseBool(e); t || err != nil { + // treat any other value as true + cli.contentTrust = true + } + } + return nil + } +} + +// WithContentTrust enables content trust on a cli. +func WithContentTrust(enabled bool) DockerCliOption { + return func(cli *DockerCli) error { + cli.contentTrust = enabled + return nil + } +} + +// WithContainerizedClient sets the containerized client constructor on a cli. +func WithContainerizedClient(containerizedFn func(string) (clitypes.ContainerizedClient, error)) DockerCliOption { + return func(cli *DockerCli) error { + cli.newContainerizeClient = containerizedFn + return nil + } +} diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go index 71029e9107..e83941ffc6 100644 --- a/cli/command/cli_test.go +++ b/cli/command/cli_test.go @@ -1,8 +1,11 @@ package command import ( + "bytes" "context" "crypto/x509" + "fmt" + "io/ioutil" "os" "runtime" "testing" @@ -10,6 +13,7 @@ import ( cliconfig "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/flags" + clitypes "github.com/docker/cli/types" "github.com/docker/docker/api" "github.com/docker/docker/api/types" "github.com/docker/docker/client" @@ -247,3 +251,44 @@ func TestGetClientWithPassword(t *testing.T) { }) } } + +func TestNewDockerCliAndOperators(t *testing.T) { + // Test default operations and also overriding default ones + cli, err := NewDockerCli( + WithContentTrust(true), + WithContainerizedClient(func(string) (clitypes.ContainerizedClient, error) { return nil, nil }), + ) + assert.NilError(t, err) + // Check streams are initialized + assert.Check(t, cli.In() != nil) + assert.Check(t, cli.Out() != nil) + assert.Check(t, cli.Err() != nil) + assert.Equal(t, cli.ContentTrustEnabled(), true) + client, err := cli.NewContainerizedEngineClient("") + assert.NilError(t, err) + assert.Equal(t, client, nil) + + // Apply can modify a dockerCli after construction + inbuf := bytes.NewBuffer([]byte("input")) + outbuf := bytes.NewBuffer(nil) + errbuf := bytes.NewBuffer(nil) + cli.Apply( + WithInputStream(ioutil.NopCloser(inbuf)), + WithOutputStream(outbuf), + WithErrorStream(errbuf), + ) + // Check input stream + inputStream, err := ioutil.ReadAll(cli.In()) + assert.NilError(t, err) + assert.Equal(t, string(inputStream), "input") + // Check output stream + fmt.Fprintf(cli.Out(), "output") + outputStream, err := ioutil.ReadAll(outbuf) + assert.NilError(t, err) + assert.Equal(t, string(outputStream), "output") + // Check error stream + fmt.Fprintf(cli.Err(), "error") + errStream, err := ioutil.ReadAll(errbuf) + assert.NilError(t, err) + assert.Equal(t, string(errStream), "error") +} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index ab31ad499e..5909953be3 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "os" - "strconv" "strings" "github.com/docker/cli/cli" @@ -13,10 +12,8 @@ import ( cliconfig "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/debug" cliflags "github.com/docker/cli/cli/flags" - "github.com/docker/cli/internal/containerizedengine" "github.com/docker/docker/api/types/versions" "github.com/docker/docker/client" - "github.com/docker/docker/pkg/term" "github.com/sirupsen/logrus" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -170,17 +167,19 @@ func noArgs(cmd *cobra.Command, args []string) error { } func main() { - // Set terminal emulation based on platform as required. - stdin, stdout, stderr := term.StdStreams() - logrus.SetOutput(stderr) + dockerCli, err := command.NewDockerCli() + if err != nil { + fmt.Fprintln(dockerCli.Err(), err) + os.Exit(1) + } + logrus.SetOutput(dockerCli.Err()) - dockerCli := command.NewDockerCli(stdin, stdout, stderr, contentTrustEnabled(), containerizedengine.NewClient) cmd := newDockerCommand(dockerCli) if err := cmd.Execute(); err != nil { if sterr, ok := err.(cli.StatusError); ok { if sterr.Status != "" { - fmt.Fprintln(stderr, sterr.Status) + fmt.Fprintln(dockerCli.Err(), sterr.Status) } // StatusError should only be used for errors, and all errors should // have a non-zero exit status, so never exit with 0 @@ -189,21 +188,11 @@ func main() { } os.Exit(sterr.StatusCode) } - fmt.Fprintln(stderr, err) + fmt.Fprintln(dockerCli.Err(), err) os.Exit(1) } } -func contentTrustEnabled() bool { - if e := os.Getenv("DOCKER_CONTENT_TRUST"); e != "" { - if t, err := strconv.ParseBool(e); t || err != nil { - // treat any other value as true - return true - } - } - return false -} - func dockerPreRun(opts *cliflags.ClientOptions) { cliflags.SetLogLevel(opts.Common.LogLevel) diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index 7c6e4526f4..e0f23747c0 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -25,27 +25,33 @@ func TestClientDebugEnabled(t *testing.T) { assert.Check(t, is.Equal(logrus.DebugLevel, logrus.GetLevel())) } +var discard = ioutil.NopCloser(bytes.NewBuffer(nil)) + func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) { - discard := ioutil.Discard - cmd := newDockerCommand(command.NewDockerCli(os.Stdin, discard, discard, false, nil)) + cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(ioutil.Discard)) + assert.NilError(t, err) + cmd := newDockerCommand(cli) cmd.SetArgs([]string{"help", "invalid"}) - err := cmd.Execute() + err = cmd.Execute() assert.Error(t, err, "unknown help topic: invalid") } func TestExitStatusForInvalidSubcommand(t *testing.T) { - discard := ioutil.Discard - cmd := newDockerCommand(command.NewDockerCli(os.Stdin, discard, discard, false, nil)) + cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(ioutil.Discard)) + assert.NilError(t, err) + cmd := newDockerCommand(cli) cmd.SetArgs([]string{"invalid"}) - err := cmd.Execute() + err = cmd.Execute() assert.Check(t, is.ErrorContains(err, "docker: 'invalid' is not a docker command.")) } func TestVersion(t *testing.T) { var b bytes.Buffer - cmd := newDockerCommand(command.NewDockerCli(os.Stdin, &b, &b, false, nil)) + cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(&b)) + assert.NilError(t, err) + cmd := newDockerCommand(cli) cmd.SetArgs([]string{"--version"}) - err := cmd.Execute() + err = cmd.Execute() assert.NilError(t, err) assert.Check(t, is.Contains(b.String(), "Docker version")) } diff --git a/docs/yaml/generate.go b/docs/yaml/generate.go index 09550b2fad..fdc5a2522f 100644 --- a/docs/yaml/generate.go +++ b/docs/yaml/generate.go @@ -10,7 +10,6 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/commands" - "github.com/docker/docker/pkg/term" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -18,8 +17,10 @@ import ( const descriptionSourcePath = "docs/reference/commandline/" func generateCliYaml(opts *options) error { - stdin, stdout, stderr := term.StdStreams() - dockerCli := command.NewDockerCli(stdin, stdout, stderr, false, nil) + dockerCli, err := command.NewDockerCli() + if err != nil { + return err + } cmd := &cobra.Command{Use: "docker"} commands.AddCommands(cmd, dockerCli) disableFlagsInUseLine(cmd) diff --git a/man/generate.go b/man/generate.go index e5e480be3f..63c4219d79 100644 --- a/man/generate.go +++ b/man/generate.go @@ -11,7 +11,6 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/commands" - "github.com/docker/docker/pkg/term" "github.com/spf13/cobra" "github.com/spf13/cobra/doc" "github.com/spf13/pflag" @@ -37,8 +36,10 @@ func generateManPages(opts *options) error { header.Date = &now } - stdin, stdout, stderr := term.StdStreams() - dockerCli := command.NewDockerCli(stdin, stdout, stderr, false, nil) + dockerCli, err := command.NewDockerCli() + if err != nil { + return err + } cmd := &cobra.Command{Use: "docker"} commands.AddCommands(cmd, dockerCli) source := filepath.Join(opts.source, descriptionSourcePath)