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

feat: add WithEnv method for setting a task's environment variables #208

Merged
merged 19 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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
21 changes: 21 additions & 0 deletions script.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ type Pipe struct {
// because pipe stages are concurrent, protect 'err'
mu *sync.Mutex
err error

// env contains the environment to run any exec commands with.
// Each entry in the array should be of the form key=value.
// If env is not nil, it will replace the default environment variables
// when executing commands.
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's worth pointing out also that if env is an empty array, it will remove everything, and commands will be run with a completely empty environment. Since nil and empty arrays usually have the same semantics, this would otherwise be slightly surprising (but completely correct) behaviour.

Copy link
Owner

Choose a reason for hiding this comment

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

The place to say this, of course, would be in the doc comment for WithEnv—in fact, we don't really need to say anything much about the struct field itself.

env []string
bitfield marked this conversation as resolved.
Show resolved Hide resolved
}

// Args creates a pipe containing the program's command-line arguments from
Expand Down Expand Up @@ -166,6 +172,7 @@ func NewPipe() *Pipe {
mu: new(sync.Mutex),
stdout: os.Stdout,
httpClient: http.DefaultClient,
env: nil,
}
}

Expand Down Expand Up @@ -388,6 +395,9 @@ func (p *Pipe) Exec(cmdLine string) *Pipe {
if p.stderr != nil {
cmd.Stderr = p.stderr
}
if p.env != nil {
cmd.Env = p.env
}
err = cmd.Start()
if err != nil {
fmt.Fprintln(cmd.Stderr, err)
Expand Down Expand Up @@ -428,6 +438,9 @@ func (p *Pipe) ExecForEach(cmdLine string) *Pipe {
if p.stderr != nil {
cmd.Stderr = p.stderr
}
if p.env != nil {
cmd.Env = p.env
}
err = cmd.Start()
if err != nil {
fmt.Fprintln(cmd.Stderr, err)
Expand Down Expand Up @@ -898,6 +911,14 @@ func (p *Pipe) WithStdout(w io.Writer) *Pipe {
return p
}

// WithEnv sets the pipe's environment to the string array env. This will override
bitfield marked this conversation as resolved.
Show resolved Hide resolved
// the default process environment variables when executing commands run via [Pipe.Exec]
// or [Pipe.ExecForEach].
func (p *Pipe) WithEnv(env []string) *Pipe {
p.env = env
return p
}

// WriteFile writes the pipe's contents to the file path, truncating it if it
// exists, and returns the number of bytes successfully written, or an error.
func (p *Pipe) WriteFile(path string) (int64, error) {
Expand Down
52 changes: 52 additions & 0 deletions script_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"os"
"path/filepath"
"regexp"
"slices"
"strings"
"testing"
"testing/iotest"
Expand Down Expand Up @@ -1768,6 +1769,57 @@ func TestWithStdout_SetsSpecifiedWriterAsStdout(t *testing.T) {
}
}

func TestWithEnv_SetEmptyEnvironment(t *testing.T) {
bitfield marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()
buf := new(bytes.Buffer)
env := []string{}

_, err := script.NewPipe().WithStdout(buf).WithEnv(env).Exec("printenv").Stdout()
bitfield marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Fatal(err)
}

if len(buf.String()) != 0 {
got := strings.Split(strings.TrimRight(buf.String(), "\n"), "\n")
t.Errorf("expected 0 environment variables, got %d %v", len(got), got)
bitfield marked this conversation as resolved.
Show resolved Hide resolved
}
}

func TestWithEnv_SetMultipleEnvVars(t *testing.T) {
bitfield marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()
buf := new(bytes.Buffer)
env := []string{"ENV1=test1", "ENV2=test2"}

_, err := script.NewPipe().WithStdout(buf).WithEnv(env).Exec("printenv").Stdout()
Copy link
Owner

Choose a reason for hiding this comment

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

It seems a shame to add an implicit dependency on the printenv command, doesn't it? Could we use the shell instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bitfield would you please be able to elaborate on this for me? i'm not sure what it means to use the shell instead

Copy link
Owner

Choose a reason for hiding this comment

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

Well, you'll notice that script itself doesn't rely on any external commands being present—which is a good thing, because we basically can't rely on that! And the tests don't use many external commands either: currently just go (which seems at least reasonable) and echo (which exists most places, is usually a shell builtin, and even works on Windows, though with slightly different syntax).

If we could stick to those, without adding a new dependency that we don't absolutely need, that would be a nice bonus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bitfield there is the env command but i'm not sure that's too different from using printenv. the other option would be to use echo $ENV1 to capture the value of a single environment variable but i don't believe there's a way to print all the variables using echo. let me know what you think!

Copy link
Owner

Choose a reason for hiding this comment

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

If we could avoid adding a new external dependency, that would be great!

Copy link
Contributor Author

@mahadzaryab1 mahadzaryab1 Aug 17, 2024

Choose a reason for hiding this comment

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

@bitfield using echo doesn't actually work since the variables aren't exported. I'm not sure if there's another way to print the variables we want without using env or printenv. Let me know how you think we should proceed.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is a bit of a subtle one. If you did something like this:

Exec("echo ENV1=$ENV1")

that wouldn't work, because $-expansion is done by the shell, and we're not executing a shell! We're executing the echo command directly, which doesn't interpolate variables.

On the other hand, if we exec a shell instead:

Exec("sh -c 'echo ENV1=$ENV1'")

this works as expected, because the shell expands $ENV1 before passing it to echo.

We do use sh -c in a couple of existing tests, so there's no problem with depending on it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's really cool! Thanks for the explanation @bitfield

if err != nil {
t.Fatal(err)
}

got := strings.Split(strings.TrimRight(buf.String(), "\n"), "\n")
bitfield marked this conversation as resolved.
Show resolved Hide resolved

if !slices.Equal(got, env) {
t.Errorf("expected %v, got %v", env, got)
}
}

func TestNotSettingEnvFallsBackToDefaultEnvironment(t *testing.T) {
bitfield marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()
buf := new(bytes.Buffer)

_, err := script.NewPipe().WithStdout(buf).Exec("printenv").Stdout()
if err != nil {
t.Fatal(err)
}

// not setting the environment should simply use the task's default environment
expected := os.Environ()
bitfield marked this conversation as resolved.
Show resolved Hide resolved
got := strings.Split(strings.TrimRight(buf.String(), "\n"), "\n")

if !slices.Equal(got, expected) {
t.Errorf("expected %v, got %v", expected, got)
}
}

func TestErrorReturnsErrorSetByPreviousPipeStage(t *testing.T) {
t.Parallel()
p := script.File("testdata/nonexistent.txt")
Expand Down