Skip to content

publish RunExec for use by docker/compose #3436

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Feb 25, 2022
Merged

publish RunExec for use by docker/compose #3436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 47 additions & 44 deletions cli/command/container/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,62 +16,65 @@ import (
"github.com/spf13/cobra"
)

type execOptions struct {
detachKeys string
interactive bool
tty bool
detach bool
user string
privileged bool
env opts.ListOpts
workdir string
container string
command []string
envFile opts.ListOpts
// ExecOptions group options for `exec` command
type ExecOptions struct {
DetachKeys string
Interactive bool
TTY bool
Detach bool
User string
Privileged bool
Env opts.ListOpts
Copy link
Member

@thaJeztah thaJeztah Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@ndeloof ndeloof Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@thaJeztah thaJeztah Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Workdir string
Container string
Command []string
EnvFile opts.ListOpts
Copy link
Member

@thaJeztah thaJeztah Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

}

func newExecOptions() execOptions {
return execOptions{
env: opts.NewListOpts(opts.ValidateEnv),
envFile: opts.NewListOpts(nil),
// NewExecOptions creates a new ExecOptions
func NewExecOptions() ExecOptions {
return ExecOptions{
Env: opts.NewListOpts(opts.ValidateEnv),
Copy link
Member

@thaJeztah thaJeztah Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

EnvFile: opts.NewListOpts(nil),
}
}

// NewExecCommand creates a new cobra.Command for `docker exec`
func NewExecCommand(dockerCli command.Cli) *cobra.Command {
options := newExecOptions()
options := NewExecOptions()

cmd := &cobra.Command{
Use: "exec [OPTIONS] CONTAINER COMMAND [ARG...]",
Short: "Run a command in a running container",
Args: cli.RequiresMinArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
options.container = args[0]
options.command = args[1:]
return runExec(dockerCli, options)
options.Container = args[0]
options.Command = args[1:]
return RunExec(dockerCli, options)
},
}

flags := cmd.Flags()
flags.SetInterspersed(false)

flags.StringVarP(&options.detachKeys, "detach-keys", "", "", "Override the key sequence for detaching a container")
flags.BoolVarP(&options.interactive, "interactive", "i", false, "Keep STDIN open even if not attached")
flags.BoolVarP(&options.tty, "tty", "t", false, "Allocate a pseudo-TTY")
flags.BoolVarP(&options.detach, "detach", "d", false, "Detached mode: run command in the background")
flags.StringVarP(&options.user, "user", "u", "", "Username or UID (format: <name|uid>[:<group|gid>])")
flags.BoolVarP(&options.privileged, "privileged", "", false, "Give extended privileges to the command")
flags.VarP(&options.env, "env", "e", "Set environment variables")
flags.StringVarP(&options.DetachKeys, "detach-keys", "", "", "Override the key sequence for detaching a container")
flags.BoolVarP(&options.Interactive, "interactive", "i", false, "Keep STDIN open even if not attached")
flags.BoolVarP(&options.TTY, "tty", "t", false, "Allocate a pseudo-TTY")
flags.BoolVarP(&options.Detach, "detach", "d", false, "Detached mode: run command in the background")
flags.StringVarP(&options.User, "user", "u", "", "Username or UID (format: <name|uid>[:<group|gid>])")
flags.BoolVarP(&options.Privileged, "privileged", "", false, "Give extended privileges to the command")
flags.VarP(&options.Env, "env", "e", "Set environment variables")
flags.SetAnnotation("env", "version", []string{"1.25"})
flags.Var(&options.envFile, "env-file", "Read in a file of environment variables")
flags.Var(&options.EnvFile, "env-file", "Read in a file of environment variables")
flags.SetAnnotation("env-file", "version", []string{"1.25"})
flags.StringVarP(&options.workdir, "workdir", "w", "", "Working directory inside the container")
flags.StringVarP(&options.Workdir, "workdir", "w", "", "Working directory inside the container")
flags.SetAnnotation("workdir", "version", []string{"1.35"})

return cmd
}

func runExec(dockerCli command.Cli, options execOptions) error {
// RunExec executes an `exec` command
func RunExec(dockerCli command.Cli, options ExecOptions) error {
execConfig, err := parseExec(options, dockerCli.ConfigFile())
if err != nil {
return err
Expand All @@ -84,7 +87,7 @@ func runExec(dockerCli command.Cli, options execOptions) error {
// otherwise if we error out we will leak execIDs on the server (and
// there's no easy way to clean those up). But also in order to make "not
// exist" errors take precedence we do a dummy inspect first.
if _, err := client.ContainerInspect(ctx, options.container); err != nil {
if _, err := client.ContainerInspect(ctx, options.Container); err != nil {
return err
}
if !execConfig.Detach {
Expand All @@ -93,7 +96,7 @@ func runExec(dockerCli command.Cli, options execOptions) error {
}
}

response, err := client.ContainerExecCreate(ctx, options.container, *execConfig)
response, err := client.ContainerExecCreate(ctx, options.Container, *execConfig)
if err != nil {
return err
}
Expand Down Expand Up @@ -195,33 +198,33 @@ func getExecExitStatus(ctx context.Context, client apiclient.ContainerAPIClient,

// parseExec parses the specified args for the specified command and generates
// an ExecConfig from it.
func parseExec(execOpts execOptions, configFile *configfile.ConfigFile) (*types.ExecConfig, error) {
func parseExec(execOpts ExecOptions, configFile *configfile.ConfigFile) (*types.ExecConfig, error) {
execConfig := &types.ExecConfig{
User: execOpts.user,
Privileged: execOpts.privileged,
Tty: execOpts.tty,
Cmd: execOpts.command,
Detach: execOpts.detach,
WorkingDir: execOpts.workdir,
User: execOpts.User,
Privileged: execOpts.Privileged,
Tty: execOpts.TTY,
Cmd: execOpts.Command,
Detach: execOpts.Detach,
WorkingDir: execOpts.Workdir,
}

// collect all the environment variables for the container
var err error
if execConfig.Env, err = opts.ReadKVEnvStrings(execOpts.envFile.GetAll(), execOpts.env.GetAll()); err != nil {
if execConfig.Env, err = opts.ReadKVEnvStrings(execOpts.EnvFile.GetAll(), execOpts.Env.GetAll()); err != nil {
return nil, err
}

// If -d is not set, attach to everything by default
if !execOpts.detach {
if !execOpts.Detach {
execConfig.AttachStdout = true
execConfig.AttachStderr = true
if execOpts.interactive {
if execOpts.Interactive {
execConfig.AttachStdin = true
}
}

if execOpts.detachKeys != "" {
execConfig.DetachKeys = execOpts.detachKeys
if execOpts.DetachKeys != "" {
execConfig.DetachKeys = execOpts.DetachKeys
} else {
execConfig.DetachKeys = configFile.DetachKeys
}
Expand Down
76 changes: 38 additions & 38 deletions cli/command/container/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import (
"gotest.tools/v3/fs"
)

func withDefaultOpts(options execOptions) execOptions {
options.env = opts.NewListOpts(opts.ValidateEnv)
options.envFile = opts.NewListOpts(nil)
if len(options.command) == 0 {
options.command = []string{"command"}
func withDefaultOpts(options ExecOptions) ExecOptions {
options.Env = opts.NewListOpts(opts.ValidateEnv)
options.EnvFile = opts.NewListOpts(nil)
if len(options.Command) == 0 {
options.Command = []string{"command"}
}
return options
}
Expand All @@ -35,7 +35,7 @@ TWO=2
defer tmpFile.Remove()

testcases := []struct {
options execOptions
options ExecOptions
configFile configfile.ConfigFile
expected types.ExecConfig
}{
Expand All @@ -45,23 +45,23 @@ TWO=2
AttachStdout: true,
AttachStderr: true,
},
options: withDefaultOpts(execOptions{}),
options: withDefaultOpts(ExecOptions{}),
},
{
expected: types.ExecConfig{
Cmd: []string{"command1", "command2"},
AttachStdout: true,
AttachStderr: true,
},
options: withDefaultOpts(execOptions{
command: []string{"command1", "command2"},
options: withDefaultOpts(ExecOptions{
Command: []string{"command1", "command2"},
}),
},
{
options: withDefaultOpts(execOptions{
interactive: true,
tty: true,
user: "uid",
options: withDefaultOpts(ExecOptions{
Interactive: true,
TTY: true,
User: "uid",
}),
expected: types.ExecConfig{
User: "uid",
Expand All @@ -73,17 +73,17 @@ TWO=2
},
},
{
options: withDefaultOpts(execOptions{detach: true}),
options: withDefaultOpts(ExecOptions{Detach: true}),
expected: types.ExecConfig{
Detach: true,
Cmd: []string{"command"},
},
},
{
options: withDefaultOpts(execOptions{
tty: true,
interactive: true,
detach: true,
options: withDefaultOpts(ExecOptions{
TTY: true,
Interactive: true,
Detach: true,
}),
expected: types.ExecConfig{
Detach: true,
Expand All @@ -92,7 +92,7 @@ TWO=2
},
},
{
options: withDefaultOpts(execOptions{detach: true}),
options: withDefaultOpts(ExecOptions{Detach: true}),
configFile: configfile.ConfigFile{DetachKeys: "de"},
expected: types.ExecConfig{
Cmd: []string{"command"},
Expand All @@ -101,9 +101,9 @@ TWO=2
},
},
{
options: withDefaultOpts(execOptions{
detach: true,
detachKeys: "ab",
options: withDefaultOpts(ExecOptions{
Detach: true,
DetachKeys: "ab",
}),
configFile: configfile.ConfigFile{DetachKeys: "de"},
expected: types.ExecConfig{
Expand All @@ -119,9 +119,9 @@ TWO=2
AttachStderr: true,
Env: []string{"ONE=1", "TWO=2"},
},
options: func() execOptions {
o := withDefaultOpts(execOptions{})
o.envFile.Set(tmpFile.Path())
options: func() ExecOptions {
o := withDefaultOpts(ExecOptions{})
o.EnvFile.Set(tmpFile.Path())
return o
}(),
},
Expand All @@ -132,10 +132,10 @@ TWO=2
AttachStderr: true,
Env: []string{"ONE=1", "TWO=2", "ONE=override"},
},
options: func() execOptions {
o := withDefaultOpts(execOptions{})
o.envFile.Set(tmpFile.Path())
o.env.Set("ONE=override")
options: func() ExecOptions {
o := withDefaultOpts(ExecOptions{})
o.EnvFile.Set(tmpFile.Path())
o.Env.Set("ONE=override")
return o
}(),
},
Expand All @@ -149,8 +149,8 @@ TWO=2
}

func TestParseExecNoSuchFile(t *testing.T) {
execOpts := withDefaultOpts(execOptions{})
execOpts.envFile.Set("no-such-env-file")
execOpts := withDefaultOpts(ExecOptions{})
execOpts.EnvFile.Set("no-such-env-file")
execConfig, err := parseExec(execOpts, &configfile.ConfigFile{})
assert.ErrorContains(t, err, "no-such-env-file")
assert.Check(t, os.IsNotExist(err))
Expand All @@ -160,23 +160,23 @@ func TestParseExecNoSuchFile(t *testing.T) {
func TestRunExec(t *testing.T) {
var testcases = []struct {
doc string
options execOptions
options ExecOptions
client fakeClient
expectedError string
expectedOut string
expectedErr string
}{
{
doc: "successful detach",
options: withDefaultOpts(execOptions{
container: "thecontainer",
detach: true,
options: withDefaultOpts(ExecOptions{
Container: "thecontainer",
Detach: true,
}),
client: fakeClient{execCreateFunc: execCreateWithID},
},
{
doc: "inspect error",
options: newExecOptions(),
options: NewExecOptions(),
client: fakeClient{
inspectFunc: func(string) (types.ContainerJSON, error) {
return types.ContainerJSON{}, errors.New("failed inspect")
Expand All @@ -186,7 +186,7 @@ func TestRunExec(t *testing.T) {
},
{
doc: "missing exec ID",
options: newExecOptions(),
options: NewExecOptions(),
expectedError: "exec ID empty",
},
}
Expand All @@ -195,7 +195,7 @@ func TestRunExec(t *testing.T) {
t.Run(testcase.doc, func(t *testing.T) {
cli := test.NewFakeCli(&testcase.client)

err := runExec(cli, testcase.options)
err := RunExec(cli, testcase.options)
if testcase.expectedError != "" {
assert.ErrorContains(t, err, testcase.expectedError)
} else {
Expand Down
Loading