Introduce functional arguments to NewDockerCli for a more stable API. #1633

silvin-lubecki · 2019-01-23T23:41:04Z

- What I did
Introduce functional arguments to NewDockerCli for a more stable API.
Add an Apply function to the Cli interface to modify it after construction.

- A picture of a cute animal (not mandatory but encouraged)

codecov-io · 2019-01-23T23:46:12Z

Codecov Report

Merging #1633 into master will increase coverage by 0.2%.
The diff coverage is 59.21%.

@@            Coverage Diff            @@
##           master    #1633     +/-   ##
=========================================
+ Coverage   55.69%   55.89%   +0.2%     
=========================================
  Files         301      299      -2     
  Lines       20470    20479      +9     
=========================================
+ Hits        11400    11446     +46     
+ Misses       8250     8210     -40     
- Partials      820      823      +3

thaJeztah

thanks! 😍

left some comments inline; after this is merged, we can go through other options to see if we can migrate more options to functional arguments 👍

thaJeztah · 2019-01-24T00:29:27Z

cli/command/cli.go

 }

+// DockerCliOperator applies a modification on a DockerCli.
+type DockerCliOperator func(cli *DockerCli)


perhaps s/DockerCliOperator/DockerCliOption/ ?

note that strictly, we should've used CLI (all caps), but that ship has likely sailed 😓

One thing I'm thinking; should we change the signature to return an error? This would allow us to capture possible errors in the configuration (which could be if these functional options take (e.g.) configuration from something else, and there's an issue with the configuration, or (e.g.) if there's conflicting options set

@vdemeester @ijc wdyt?

I do agree on the "return error" 👼 😉. I do like Operator somehow 🙃

👍 for the renaming.
About the error, I thought about that too, I just wondered it may modify too much code for the caller, as suddenly it has to return and propagate an error, if it didn't already. But ok I'll add that 👍

I think that this func typedef should certainly return an error, but I don't think it necessarily has to follow that NewDockerCli needs to propagate it for the user to handle, it could be swallowed by generating a log message and an os.Exit inside that function. I'm not sure I like that but it's an option to consider.

One, ugly, alternative would be to stash the error and return from Initialize instead -- that sounds pretty gross but it would mean deferring the error (or even the processing of the options entirely) until the DockerCli was actually about to be used -- not sure how many users unconditionally make a DockerCli on init but make a more dynamic decision about whether to use (and this Initialize) it further down the line. I suspect not too many (in which case I think this is a terrible idea...)

I thought this sort of thing was customarily called a [fF]ooOption (or even Opt) rather than Operator. Maybe I'm wrong but:

$ pwd .../gocode/src/github.com/docker/cli $ git grep type.*func.*Oper $ git grep type.*func.*Opt cli/compose/loader/loader.go:func Load(configDetails types.ConfigDetails, options ...func(*Options)) (*types.Config, error) { vendor/github.com/containerd/containerd/archive/tar_opts.go:type ApplyOpt func(options *ApplyOptions) error vendor/github.com/containerd/containerd/client_opts.go:type ClientOpt func(c *clientOpts) error vendor/github.com/containerd/containerd/content/content.go:type WriterOpt func(*WriterOpts) error vendor/github.com/containerd/containerd/export.go:type ExportOpt func(c *exportOpts) error vendor/github.com/containerd/containerd/images/image.go:type DeleteOpt func(context.Context, *DeleteOptions) error vendor/github.com/containerd/containerd/import.go:type ImportOpt func(c *importOpts) error vendor/github.com/containerd/containerd/leases/lease.go:type DeleteOpt func(context.Context, *DeleteOptions) error vendor/github.com/containerd/containerd/remotes/docker/schema1/converter.go:type ConvertOpt func(context.Context, *ConvertOptions) error vendor/google.golang.org/grpc/clientconn.go:type DialOption func(*dialOptions) vendor/google.golang.org/grpc/interceptor.go:type UnaryInvoker func(ctx context.Context, method string, req, reply interface{}, cc *ClientConn, opts ...CallOption) error vendor/google.golang.org/grpc/interceptor.go:type UnaryClientInterceptor func(ctx context.Context, method string, req, reply interface{}, cc *ClientConn, invoker UnaryInvoker, opts ...CallOption) error vendor/google.golang.org/grpc/interceptor.go:type Streamer func(ctx context.Context, desc *StreamDesc, cc *ClientConn, method string, opts ...CallOption) (ClientStream, error) vendor/google.golang.org/grpc/interceptor.go:type StreamClientInterceptor func(ctx context.Context, desc *StreamDesc, cc *ClientConn, method string, streamer Streamer, opts ...CallOption) (ClientStream, error) vendor/k8s.io/client-go/tools/cache/listwatch.go:type ListFunc func(options metav1.ListOptions) (runtime.Object, error) vendor/k8s.io/client-go/tools/cache/listwatch.go:type WatchFunc func(options metav1.ListOptions) (watch.Interface, error) vendor/k8s.io/client-go/tools/pager/pager.go:type ListPageFunc func(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error)

(not all of those are due to this initialiser pattern, FWIW moby/moby and containerd/containerd show a similar pattern)

I think that this func typedef should certainly return an error, but I don't think it necessarily has to follow that NewDockerCli needs to propagate it for the user to handle

I've seen two patterns; one that never errors, and one that can error. I think having an error is more flexible (errors can be ignored if we don't need them, or a functional option can just return nil)

but make a more dynamic decision about whether to use (and this Initialize) it further down the line.

Right, so I was thinking about that a bit; there's some options that may depend on runtime options (e.g. if a specific flag or environment variable is set) and those have to be evaluated when a command is triggered.

Functions are first-class citizens in Go; should we have something to pass functions that can be executed at runtime (e.g. for the context/config store to be evaluated when the command is run, instead of having to manually call Initialize() before running a command?

Having the process exit on NewDockerCli() seem a bit dangerous to me, for 3rd party consumers. I'd vote for NewDockerCli() returning an error.

For Post-Initialize functions, we can still use Apply()

thaJeztah · 2019-01-24T00:33:46Z

cli/command/cli.go

+	return cli
+}
+
+// WithStandardStreams sets a cli in, out and err streams with the standard streams.


Perhaps we should move all these options to a separate file (e.g. cli/command/cli_options.go or cli/command/options.go)

I'll do that 👍

vdemeester

@silvin-lubecki I do like that 😍 a lot !

vdemeester · 2019-01-24T08:06:03Z

cli/command/cli.go

 }

+// DockerCliOperator applies a modification on a DockerCli.
+type DockerCliOperator func(cli *DockerCli)


I do agree on the "return error" 👼 😉. I do like Operator somehow 🙃

ijc · 2019-01-24T10:31:44Z

cli/command/cli.go

-	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.
+func NewDockerCli(ops ...DockerCliOperator) *DockerCli {
+	cli := &DockerCli{}


Does this mean that some ops are always needed (e.g. to provide a stream). Would we want to stub those out to some default (either os.Std* or io.Discard I suppose)?

I added default operations:

func NewDockerCli(ops ...DockerCliOperator) (*DockerCli, error) { cli := &DockerCli{} defaultOps := []DockerCliOperator{ WithStandardStreams(), WithContentTrustFromEnv(), WithContainerizedClient(containerizedengine.NewClient), } ops = append(defaultOps, ops...) if err := cli.Apply(ops...); err != nil { return nil, err } return cli, nil }

One can override them as users operations are applied after the default ones.

I think the current approach looks valid; there may other caveats (what if want without containerised client, or without content-trust)"

possible alternatives are;

only apply defaults if ops is empty

add a WithDefaults() 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)

ijc · 2019-01-24T10:33:13Z

cli/command/cli.go

+}
+
+// WithContentTrust enables content trust on a cli.
+func WithContentTrust() DockerCliOperator {


FWIW I think the default should be to obey the DOCKER_CONTENT_TRUST variable (encapsulated via contentTrustEnabled() today) and there should be one (or two) of these to force it on or off explicitly.

👍 for default contentTrust value initialized using the env variable, plus to enable/disable explicitly, I would have the signature be func WithContentTrust(enabled bool) DockerCliOperator

ijc · 2019-01-24T10:34:24Z

cmd/docker/docker_test.go

 func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) {
-	discard := ioutil.Discard
-	cmd := newDockerCommand(command.NewDockerCli(os.Stdin, discard, discard, false, nil))
+	cmd := newDockerCommand(command.NewDockerCli(command.WithInputStream(os.Stdin)))


cf what I wrote above this appears to rely on the streams defaulting to discard.

simonferquel

Some tweaks needed but I like the approach very much

simonferquel · 2019-01-24T14:09:11Z

cli/command/cli.go

 }

+// DockerCliOperator applies a modification on a DockerCli.
+type DockerCliOperator func(cli *DockerCli)


Having the process exit on NewDockerCli() seem a bit dangerous to me, for 3rd party consumers. I'd vote for NewDockerCli() returning an error.

simonferquel · 2019-01-24T14:10:26Z

cli/command/cli.go

 }

+// DockerCliOperator applies a modification on a DockerCli.
+type DockerCliOperator func(cli *DockerCli)


For Post-Initialize functions, we can still use Apply()

simonferquel · 2019-01-24T14:13:04Z

cli/command/cli.go

+}
+
+// WithStandardStreams sets a cli in, out and err streams with the standard streams.
+func WithStandardStreams() DockerCliOperator {


I feel like it should not be an option, and be set by default (the user would use With*Stream to override defaults). Ideally NewDockerCli without parameters should just work.

simonferquel · 2019-01-24T14:14:54Z

cli/command/cli.go

+}
+
+// WithContentTrust enables content trust on a cli.
+func WithContentTrust() DockerCliOperator {


👍 for default contentTrust value initialized using the env variable, plus to enable/disable explicitly, I would have the signature be func WithContentTrust(enabled bool) DockerCliOperator

simonferquel · 2019-01-24T14:18:07Z

cmd/docker/docker.go

+	logrus.SetOutput(os.Stderr)
+
+	dockerCli := command.NewDockerCli(
+		command.WithInputStream(stdin),


within the CLI code, no option should be required. (good defaults for streams, containerizedClient and content trust in the constructor). This is to make it easier for 3rd party to have (by default) the exact same behavior as the CLI itself, and use the options only to customize this behavior.

simonferquel · 2019-01-24T16:57:37Z

lgtm

vdemeester

Nits 👼
Also, InStream and OutStream should have an type alias in cli/command 👼

vdemeester · 2019-01-24T17:02:01Z

cli/command/cli.go

 	configFile            *configfile.ConfigFile
-	in                    *InStream
-	out                   *OutStream
+	in                    *streams.InStream


-> streams.In

vdemeester · 2019-01-24T17:02:07Z

cli/command/cli.go

-	in                    *InStream
-	out                   *OutStream
+	in                    *streams.InStream
+	out                   *streams.OutStream


streams.Out

ijc · 2019-01-24T17:06:06Z

cli/command/cli_options.go

+	return func(cli *DockerCli) error {
+		// Set terminal emulation based on platform as required.
+		stdin, stdout, stderr := term.StdStreams()
+		cli.in = streams.NewInStream(stdin)


If you accept @vdemeester 's suggestion to make the type streams.In you'd want to s/Stream//g here too.

in order not to break (too much) existing code (and I know, this PR WILL break some code), we can alias all the moved Stream related symbols so that command.NewInStream, command.InStream etc still work

(on the + side, it will make for less code changes in this PR as well)

TBH I'm inclined towards just biting the bullet and making/propagating the change. The necessary updates are pretty mechanical and we should try and avoid building up compatibility debt (which can never be removed) when we don't really need to.

Up to the maintainers though.

If it's a one-time change, I agree with grouping breaking changes together (separate commit though)

ijc · 2019-01-24T17:09:17Z

cli/command/cli_options.go

+// WithContentTrustFromEnv enables content trust on a cli from environment variable DOCKER_CONTENT_TRUST value.
+func WithContentTrustFromEnv() DockerCliOption {
+	return func(cli *DockerCli) error {
+		cli.contentTrust = func() bool {


What is the inner func() bool needed for here?

Well I just moved func contentTrustEnabled() bool from cmd/docker/docker.go to here, but Ok I can simplify it 👍

ijc · 2019-01-24T17:14:52Z

cli/command/cli_test.go

+	assert.NilError(t, err)
+	assert.Equal(t, client, nil)
+
+	// Apply can modify a dockerCli after construction


I'd be happier that this test was really checking that if it was asserting that the in/out streams had FD() != 0 before this apply.

Oh, except it can be 0 for In anyway, since that is the stdin fd too. Not sure how best to get around that -- perhaps by calling something (like runVersion?) to actually output something into the buf and check it went where you hoped it would?

If fixed that directly reading or writing to the streams and check with their underlying buffers. 👍

Looks good, thanks.

ijc · 2019-01-24T17:19:46Z

cmd/docker/docker.go

-	// Set terminal emulation based on platform as required.
-	stdin, stdout, stderr := term.StdStreams()
-	logrus.SetOutput(stderr)
+	logrus.SetOutput(os.Stderr)


Are we happy that this is still the same thing even on Windows? I know that term.StdStreams will still be called implicitly by the following NewDockerCli. I think it's the same fd as you'd get here so maybe^Wprobably that's fine.

One other subtle change here I just realised is that NewDockerCli will now always call term.StdStreams(), which unconditionally does some ioctls on stdio, even if the caller uses their own WithStandardStreams to set something else. I'm not going to pretend I know what those ioctls are doing -- are we sure they are safe and universally desirable?

Good call. Can you change the default stdstreams handling in NewDockerCLI to be done at the end, only if there are missing streams (and if only stderr is missng, don't call term.StdStreams()).

simonferquel · 2019-01-25T15:25:41Z

cli/command/cli.go

+		return nil, err
+	}
+	if cli.out == nil || cli.in == nil {
+		WithStandardStreams()(cli)


If only "in" is set, both in and out will be overwritten by WithStandardStream

simonferquel · 2019-01-25T15:25:54Z

cli/command/cli.go

+		WithStandardStreams()(cli)
+	}
+	if cli.err == nil {
+		cli.err = ioutil.Discard


Should default to os.Stderr

I'm wondering about that... 🤔 But why not.

silvin-lubecki · 2019-01-25T16:13:24Z

@simonferquel @thaJeztah @vdemeester @ijc Please take another look 😺

silvin-lubecki · 2019-01-25T16:13:49Z

I'll squash the commits once they are LGTMed

cli/command/cli.go

ijc · 2019-01-25T16:33:59Z

cli/command/streams.go

+)
+
+// InStream is an input stream used by the DockerCli to read user input
+// Deprecated: Use github.com/docker/cli/cli/streams.In instead


When do we expect we would be able to get rid of these if not right now?

I don't think we have an official depreciation plan, but I think that 2 releases is a good start, so let's say we will remove them after the 19.09 release? Do you want me to add a comment about it in the code?

@ijc it's also a matter of go usage… We do what go does with context and golang.org/x/context. I'm not really sure when (if any time), golang.org/x/context will go…
The hope is that people using those types, will see the deprecation, and update their code…

vdemeester

LGTM 🐯

simonferquel

LGTM

ijc

LGTM

thaJeztah

changes LGTM, but can you squash some of the commits into logical groups (if possible)?

…remove a cyclic dependency from command to internal/containerizedengine Aliasing old types * streams.InStream -> streams.In * streams.NewInStream -> streams.NewIn * streams.OutStream -> streams.Out * streams.NewOutStream -> streams.NewOut Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki · 2019-01-28T14:12:36Z

@thaJeztah just squashed and re-organized the commits. PTAL 🐱

vdemeester · 2019-01-28T14:39:30Z

yay 🎉

ijc · 2019-01-28T17:28:00Z

cmd/docker/docker.go

-	logrus.SetOutput(stderr)
+	dockerCli, err := command.NewDockerCli()
+	if err != nil {
+		fmt.Fprintln(dockerCli.Err(), err)


@silvin-lubecki Sorry for not spotting this before but -- dockerCli will be nil here since this is the error case. I guess os.Stderr is ok in this context.

Right, will fix that in a follow-up! Thank you for spotting that @ijc ! 🤗

nice catch 👍

Fixed with #1645

silvin-lubecki requested review from ijc, thaJeztah and vdemeester January 23, 2019 23:41

GordonTheTurtle added the status/0-triage label Jan 23, 2019

silvin-lubecki mentioned this pull request Jan 23, 2019

Basic framework for writing and running CLI plugins #1564

Merged

9 tasks

thaJeztah added status/2-code-review and removed status/0-triage labels Jan 24, 2019

thaJeztah requested changes Jan 24, 2019

View reviewed changes

vdemeester reviewed Jan 24, 2019

View reviewed changes

ijc reviewed Jan 24, 2019

View reviewed changes

simonferquel suggested changes Jan 24, 2019

View reviewed changes

vdemeester requested changes Jan 24, 2019

View reviewed changes

ijc reviewed Jan 24, 2019

View reviewed changes

silvin-lubecki force-pushed the refactor-docker-cli-construction branch from 45fbbc0 to 1f53b9c Compare January 25, 2019 15:13

simonferquel reviewed Jan 25, 2019

View reviewed changes

cli/command/cli.go Outdated Show resolved Hide resolved

simonferquel reviewed Jan 25, 2019

View reviewed changes

cli/command/cli.go Outdated Show resolved Hide resolved

ijc reviewed Jan 25, 2019

View reviewed changes

silvin-lubecki force-pushed the refactor-docker-cli-construction branch from f6edd15 to 14d21b7 Compare January 27, 2019 22:09

vdemeester approved these changes Jan 28, 2019

View reviewed changes

simonferquel approved these changes Jan 28, 2019

View reviewed changes

ijc approved these changes Jan 28, 2019

View reviewed changes

thaJeztah approved these changes Jan 28, 2019

View reviewed changes

silvin-lubecki added 2 commits January 28, 2019 14:36

Introduce functional arguments to NewDockerCli for a more stable API.

7f207f3

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki force-pushed the refactor-docker-cli-construction branch from 9415339 to 7f207f3 Compare January 28, 2019 13:54

vdemeester merged commit f95ca8e into docker:master Jan 28, 2019

GordonTheTurtle added this to the 19.03.0 milestone Jan 28, 2019

silvin-lubecki deleted the refactor-docker-cli-construction branch January 28, 2019 14:51

ijc reviewed Jan 28, 2019

View reviewed changes

This was referenced Apr 1, 2023

cli/command: fix deprecation comments for Stream types #4152

Merged

[23.0 backport] cli/command: fix deprecation comments for Stream types #4154

Merged

thaJeztah mentioned this pull request Apr 9, 2023

cli/command: remove deprecated Stream types #4174

Merged

thaJeztah mentioned this pull request Sep 24, 2025

cli/command: NewDockerCli: don't depend on DockerCli.Apply #6467

Merged

Introduce functional arguments to NewDockerCli for a more stable API. #1633

Introduce functional arguments to NewDockerCli for a more stable API. #1633

Uh oh!

Conversation

silvin-lubecki commented Jan 23, 2019

Uh oh!

codecov-io commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonferquel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonferquel commented Jan 24, 2019

Uh oh!

vdemeester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-io commented Jan 23, 2019 •

edited

Loading

vdemeester left a comment •

edited

Loading