-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Introduce functional arguments to NewDockerCli for a more stable API. #1633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,19 @@ | ||
| package command | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "crypto/x509" | ||
| "fmt" | ||
| "io/ioutil" | ||
| "os" | ||
| "runtime" | ||
| "testing" | ||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be happier that this test was really checking that if it was asserting that the in/out streams had Oh, except it can be 0 for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If fixed that directly reading or writing to the streams and check with their underlying buffers. 👍
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good, thanks. |
||
| 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") | ||
| } | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that some
opsare always needed (e.g. to provide a stream). Would we want to stub those out to some default (eitheros.Std*orio.DiscardI suppose)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added default operations:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One can override them as users operations are applied after the default ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current approach looks valid; there may other caveats (what if want without containerised client, or without content-trust)"
possible alternatives are;
opsis emptyWithDefaults()option (that's applying these), which can be used if someone wants the defaults plus some custom configurations.But we can discuss those solutions (not sure which approach makes the most sense)