Skip to content
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

Add command.Addwriter() support #1198

Merged
merged 1 commit into from
Mar 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 33 additions & 3 deletions pkg/command/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import (

// A generic command abstraction
type Command struct {
cmds []*command
cmds []*command
stdErrWriters, stdOutWriters []io.Writer
}

// The internal command representation
Expand Down Expand Up @@ -64,6 +65,8 @@ func NewWithWorkDir(workDir, cmd string, args ...string) *Command {
Cmd: cmdWithDir(workDir, cmd, args...),
pipeWriter: nil,
}},
stdErrWriters: []io.Writer{},
stdOutWriters: []io.Writer{},
}
}

Expand All @@ -88,6 +91,29 @@ func (c *Command) Pipe(cmd string, args ...string) *Command {
return c
}

// AddWriter can be used to add an additional output (stdout) and error
// (stderr) writer to the command, for example when having the need to log to
// files.
func (c *Command) AddWriter(writer io.Writer) *Command {
c.AddOutputWriter(writer)
c.AddErrorWriter(writer)
return c
}

// AddErrorWriter can be used to add an additional error (stderr) writer to the
// command, for example when having the need to log to files.
func (c *Command) AddErrorWriter(writer io.Writer) *Command {
c.stdErrWriters = append(c.stdErrWriters, writer)
return c
}

// AddOutputWriter can be used to add an additional output (stdout) writer to
// the command, for example when having the need to log to files.
func (c *Command) AddOutputWriter(writer io.Writer) *Command {
c.stdOutWriters = append(c.stdOutWriters, writer)
return c
}

// Run starts the command and waits for it to finish. It returns an error if
// the command execution was not possible at all, otherwise the Status.
// This method prints the commands output during execution
Expand Down Expand Up @@ -190,8 +216,12 @@ func (c *Command) run(printOutput bool) (res *Status, err error) {

var stdOutWriter, stdErrWriter io.Writer
if printOutput {
stdOutWriter = io.MultiWriter(os.Stdout, stdOutBuffer)
stdErrWriter = io.MultiWriter(os.Stderr, stdErrBuffer)
stdOutWriter = io.MultiWriter(append(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, but just wondering if you think we would ever want to support supplying separate writers for Stdout and Stderr? I could be mistaken here but it looks like if you want one to be written to a passed in writer, than you must allow for both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this gives us more flexibility. I changed the API to allow writers for stdout and stderr separately. Or we just use AddWriter for both.

Updated the test cases as well 👍

[]io.Writer{os.Stdout, stdOutBuffer}, c.stdOutWriters...,
)...)
stdErrWriter = io.MultiWriter(append(
[]io.Writer{os.Stderr, stdErrBuffer}, c.stdErrWriters...,
)...)
} else {
stdOutWriter = stdOutBuffer
stdErrWriter = stdErrBuffer
Expand Down
103 changes: 103 additions & 0 deletions pkg/command/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ limitations under the License.
package command

import (
"bytes"
"io/ioutil"
"os"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -197,3 +200,103 @@ func TestFailureRunSilentSuccessOutput(t *testing.T) {
require.NotNil(t, err)
require.Nil(t, res)
}

func TestSuccessLogWriter(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()

res, err := New("echo", "Hello World").AddWriter(f).RunSuccessOutput()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Equal(t, res.Output(), string(content))
}

func TestSuccessLogWriterMultiple(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()
b := &bytes.Buffer{}

res, err := New("echo", "Hello World").
AddWriter(f).
AddWriter(b).
RunSuccessOutput()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Equal(t, res.Output(), string(content))
require.Equal(t, res.Output(), b.String())
}

func TestSuccessLogWriterSilent(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()

err = New("echo", "Hello World").AddWriter(f).RunSilentSuccess()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Empty(t, content)
}

func TestSuccessLogWriterStdErr(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()

res, err := New("bash", "-c", ">&2 echo error").
AddWriter(f).RunSuccessOutput()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Equal(t, res.Error(), string(content))
}

func TestSuccessLogWriterStdErrAndStdOut(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()

res, err := New("bash", "-c", ">&2 echo stderr; echo stdout").
AddWriter(f).RunSuccessOutput()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Equal(t, res.Output()+res.Error(), string(content))
}

func TestSuccessLogWriterStdErrAndStdOutOnlyStdErr(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()

res, err := New("bash", "-c", ">&2 echo stderr; echo stdout").
AddErrorWriter(f).RunSuccessOutput()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Equal(t, res.Error(), string(content))
}

func TestSuccessLogWriterStdErrAndStdOutOnlyStdOut(t *testing.T) {
f, err := ioutil.TempFile("", "log")
require.Nil(t, err)
defer func() { require.Nil(t, os.Remove(f.Name())) }()

res, err := New("bash", "-c", ">&2 echo stderr; echo stdout").
AddOutputWriter(f).RunSuccessOutput()
require.Nil(t, err)

content, err := ioutil.ReadFile(f.Name())
require.Nil(t, err)
require.Equal(t, res.Output(), string(content))
}
14 changes: 4 additions & 10 deletions pkg/gcp/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"fmt"
"io/ioutil"
"os"
"os/exec"
"path"
"path/filepath"
"strconv"
Expand Down Expand Up @@ -232,7 +231,7 @@ func RunSingleJob(o *Options, jobName, uploaded, version string, subs map[string
args = append(args, diskSizeArg)
}

cmd := exec.Command("gcloud", args...)
cmd := command.New("gcloud", args...)

if o.LogDir != "" {
p := path.Join(o.LogDir, strings.ReplaceAll(jobName, "/", "-")+".log")
Expand All @@ -243,16 +242,11 @@ func RunSingleJob(o *Options, jobName, uploaded, version string, subs map[string
}

defer f.Close()

cmd.Stdout = f
cmd.Stderr = f
} else {
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
cmd.AddWriter(f)
}

if err := cmd.Run(); err != nil {
return errors.Wrapf(err, "error running %s", cmd.Args)
if err := cmd.RunSuccess(); err != nil {
return errors.Wrapf(err, "error running %s", cmd.String())
}

return nil
Expand Down