From c6392c60af6a276fe9156971061fe97beb63bf2a Mon Sep 17 00:00:00 2001 From: Ridai Govinda Pombo Date: Mon, 29 Jun 2020 11:50:22 -0300 Subject: [PATCH 1/2] By default, ExecShell will add and io.Discard CombinedOutput will be `io.Discard` except when someone passed IgnoreResults. Seems like a good way of throwing "async" command calls that doesn't need to be checked after execution. --- containers.go | 8 ++++++++ containers_test.go | 8 ++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/containers.go b/containers.go index 6aadf4d..f01c438 100644 --- a/containers.go +++ b/containers.go @@ -501,6 +501,9 @@ type ExecShellOptions struct { // If Interactive is true, then stdio from this exec is attached to the stdio // of the running process. Interactive bool + + // If IgnoreResults is true, then no output will be watched and no exit code would emerge + IgnoreResults bool } // ExecShell executes a bash shell command inside a container. If the process @@ -511,6 +514,11 @@ func ExecShell(ctx context.Context, client *docker.Client, containerID string, c opts = new(ExecShellOptions) } + // If the caller expects a result, we need to at least attach an IO discarding type + if opts.CombinedOutput == nil && !opts.IgnoreResults { + opts.CombinedOutput = ioutil.Discard + } + execOpts := docker.CreateExecOptions{ Context: ctx, Env: opts.Env, diff --git a/containers_test.go b/containers_test.go index 77c4fdd..976a794 100644 --- a/containers_test.go +++ b/containers_test.go @@ -66,9 +66,7 @@ func TestExecExitError(t *testing.T) { } cmdStat := "stat /somewhere/strange/that/do/not/exist" - err = ExecShell(ctx, client, containerID, cmdStat, &ExecShellOptions{ - CombinedOutput: os.Stdout, - }) + err = ExecShell(ctx, client, containerID, cmdStat, &ExecShellOptions{}) if err != nil { if code, ok := IsExitError(err); !ok || code != 1 { t.Errorf("error = %v; want exit code 1", err) @@ -79,9 +77,7 @@ func TestExecExitError(t *testing.T) { // Should have the "-p" flag cmdMkdir := "mkdir /somewhere/strange/that/do/not/exist" - err = ExecShell(ctx, client, containerID, cmdMkdir, &ExecShellOptions{ - CombinedOutput: os.Stdout, - }) + err = ExecShell(ctx, client, containerID, cmdMkdir, &ExecShellOptions{}) if err != nil { if code, ok := IsExitError(err); !ok || code != 1 { t.Errorf("error = %v; want exit code 1", err) From bc3103e5e1f1d13a767fdbb901b95a7b54095ec3 Mon Sep 17 00:00:00 2001 From: Ridai Govinda Pombo Date: Mon, 29 Jun 2020 13:36:06 -0300 Subject: [PATCH 2/2] Address PR feedback No "IgnoreResults" option, too confusing. We only use `ioutil.Discard` when starting the exection Tests don't check `err`, as asked in #17 [ch1592] --- containers.go | 18 ++++++++---------- containers_test.go | 18 ++++++------------ 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/containers.go b/containers.go index f01c438..8daf2bb 100644 --- a/containers.go +++ b/containers.go @@ -501,9 +501,6 @@ type ExecShellOptions struct { // If Interactive is true, then stdio from this exec is attached to the stdio // of the running process. Interactive bool - - // If IgnoreResults is true, then no output will be watched and no exit code would emerge - IgnoreResults bool } // ExecShell executes a bash shell command inside a container. If the process @@ -514,18 +511,13 @@ func ExecShell(ctx context.Context, client *docker.Client, containerID string, c opts = new(ExecShellOptions) } - // If the caller expects a result, we need to at least attach an IO discarding type - if opts.CombinedOutput == nil && !opts.IgnoreResults { - opts.CombinedOutput = ioutil.Discard - } - execOpts := docker.CreateExecOptions{ Context: ctx, Env: opts.Env, Cmd: []string{"bash", "-c", cmdString}, AttachStdin: opts.Interactive, - AttachStdout: opts.Interactive || opts.CombinedOutput != nil, - AttachStderr: opts.Interactive || opts.CombinedOutput != nil, + AttachStdout: true, + AttachStderr: true, Tty: opts.Interactive, Container: containerID, WorkingDir: opts.Dir, @@ -550,6 +542,12 @@ func ExecShell(ctx context.Context, client *docker.Client, containerID string, c startOpts.OutputStream = opts.CombinedOutput startOpts.ErrorStream = opts.CombinedOutput } + + if opts.CombinedOutput == nil { + startOpts.OutputStream = ioutil.Discard + startOpts.ErrorStream = ioutil.Discard + } + err = client.StartExec(exec.ID, startOpts) if err != nil { return fmt.Errorf("execute shell command in container %s: %w", containerID, err) diff --git a/containers_test.go b/containers_test.go index 976a794..c3705ca 100644 --- a/containers_test.go +++ b/containers_test.go @@ -67,23 +67,17 @@ func TestExecExitError(t *testing.T) { cmdStat := "stat /somewhere/strange/that/do/not/exist" err = ExecShell(ctx, client, containerID, cmdStat, &ExecShellOptions{}) - if err != nil { - if code, ok := IsExitError(err); !ok || code != 1 { - t.Errorf("error = %v; want exit code 1", err) - } - } else { - t.Error("Expecting an error, but none was returned") + if code, ok := IsExitError(err); !ok || code != 1 { + t.Logf("is it exit error? %v", ok) + t.Errorf("error = %v; want exit code 1, got %d", err, code) } // Should have the "-p" flag cmdMkdir := "mkdir /somewhere/strange/that/do/not/exist" err = ExecShell(ctx, client, containerID, cmdMkdir, &ExecShellOptions{}) - if err != nil { - if code, ok := IsExitError(err); !ok || code != 1 { - t.Errorf("error = %v; want exit code 1", err) - } - } else { - t.Error("Expecting an error, but none was returned") + if code, ok := IsExitError(err); !ok || code != 1 { + t.Logf("is it exit error? %v", ok) + t.Errorf("error = %v; want exit code 1, got %d", err, code) } }