publish RunExec for use by docker/compose

[ndeloof]

- What I did
make "exec" logic reusable by docker-compose without having to duplicate code

- How I did it
made runExec and execOptions public so this code can be reused in docker-compose

- How to verify it
see docker/compose#9197

- Description for the changelog

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

[thaJeztah]

Thanks! Overall makes sense, but left some comments (I think newExecOptions() should probably be exported so that the correct ListOpts are set)

Also looks like a linter is complaining about missing Godoc

cli/command/container/exec.go:19:6: exported type `ExecOptions` should have comment or be unexported (golint)
type ExecOptions struct {
^
cli/command/container/exec.go:74:1: exported function `RunExec` should have comment or be unexported (golint)
func RunExec(dockerCli command.Cli, options ExecOptions) error {
^

[thaJeztah]

While we're changing this, could you change this to TTY? (it's an acronym)

[ndeloof]

I guess the one on types.ExecConfig is generated from swagger, so we can't do the same?

[thaJeztah]

Hm... good question; actually, I don't think it is generated. Looking at https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/api/types/configs.go#L30-L44

I don't see a "generated" comment in that file, so that one was probably done manually (but using the wrong casing 😞). The files that were generated have a comment somewhere at the top; https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/api/types/plugin_env.go#L3-L4

// This file was generated by the swagger tool.// Editing this file might prove futile when you re-run the swagger generate command

But it's a good point; perhaps (as a follow-up?) we should either use that type for the options here and/or create an alias for it in the api (or client) package.

[thaJeztah]

Not something to change, more "thinking out loud"; when we're gonna work on "slicing and dicing" bits to make them more reusable, we should have a good look at options like this (does it have to be a special opts.ListOpts type here, or could a generic type (such as a []string) work for these).

[ndeloof]

docker/compose uses []string and some user reported the help make in unclear, as it says expected type is array.

[thaJeztah]

Ah, yes, the way Cobra generates the names for value types being directly related to the "type" used is often not very handy 😞

Was discussing that recently again with @crazy-max on docker/cli-docs-tool#19 (comment), where (it's kinda tacky, but we should improve things) we added an annotation to allow a custom "name" for values to be set (which could be somewhat more descriptive than stringArray (e.g.)

[thaJeztah]

This one needs some thinking as well (besides env-file being a PITA overall); EnvFile effectively is just an easy way to propagate Env; if we want to refactor things to make them more usable, we could decide to remove EnvFile option from this, and instead have an (easy to reuse) utility to append / update env-vars in Env from a file.

(no need to change, more "these are things we should look at")

[thaJeztah]

This shows a bit why I think we should look at whether or not opts.ListOpts as option is "handy", as the code using the ExecOptions would have to know that it needs to use a opts.ListOpts, but also that it needs to use one that has a specific validation rule set (opts.ValidateEnv).

Some utilities for this, adding some methods to ExecOptions itself, or perhaps using functional arguments could help for such cases.

(again, one more "these are things we should look at")

[codecov-commenter]

Codecov Report

Merging #3436 (93e9b78) into master (cf8c4ba) will increase coverage by 0.95%.
The diff coverage is 56.14%.

@@ Coverage Diff @@## master #3436 +/- ##
==========================================
+ Coverage 57.33% 58.28% +0.95% 
==========================================
Files 304 287 -17 Lines 26379 24144 -2235 ==========================================
- Hits 15124 14073 -1051 + Misses 10329 9212 -1117 + Partials 926 859 -67 

[thaJeztah]

LGTM, thanks for updating!

Remind me that we should write down some of the things we were discussing on this PR (I think this PR makes sense for the short term, but we really need to look at improving these things in the longer term, without having to export things that were not originally designed to be exported (so may not have been "very well designed" to be used externally).