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

[mahadzaryab1]

Added a method WithEnv that takes in a string array ([]string ) and overrides the environment executing a exec.Command to that array. This PR fixes #80.

[bitfield]

Great work so far! Just a couple of points:

• Do these tests work on Windows? (You can add Windows-specific tests to script_windows_test.go)
• What potential bugs in the environment-setting code are we failing to catch in tests here?

[bitfield]

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

[mahadzaryab1]

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

[bitfield]

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.

[mahadzaryab1]

@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!

[bitfield]

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

[mahadzaryab1]

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

[bitfield]

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.

[mahadzaryab1]

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

[mahadzaryab1]

Great work so far! Just a couple of points:

• Do these tests work on Windows? (You can add Windows-specific tests to script_windows_test.go)
• What potential bugs in the environment-setting code are we failing to catch in tests here?

Thank you so much for the great feedback @bitfield! I addressed some of your feedback and had a couple of follow-up questions:

• Is it possible to run the windows-specific tests locally? (I'm on a MacBook)
• For potential bugs - one possibility could be passing in malformed environment variables. Should we add some tests for this? Let me know if there's something else that I'm missing.

[bitfield]

• Is it possible to run the windows-specific tests locally? (I'm on a MacBook)

That's a good question—I'd like to be able to do this too. I can run them in CI, but of course it would be helpful for you to get the results locally. What about running the tests in a Windows Docker image? Would that work?

• For potential bugs - one possibility could be passing in malformed environment variables. Should we add some tests for this? Let me know if there's something else that I'm missing.

Well, that's really the user's problem! We don't do anything but pass on what they gave us to the exec.Cmd, which is absolutely right. The kind of bugs I meant are the ones in our logic. Basically, we could forget to set p.Env, and we could forget to reference it in either p.Exec or p.ExecForEach, and I think the existing tests cover those. So we're good.

There is still the awkward question of what happens if someone writes some arbitrary Filter function on the pipe that does something like os.Getenv. You could imagine users thinking that p.WithEnv should affect this, but it won't. As far as I can see there's no way around this, unless we called os.Setenv, which is exactly what we don't want to do. Maybe we could clarify this in the documentation somehow?

[mahadzaryab1]

@bitfield do you know if the windows tests run in WSL?

[bitfield]

@bitfield do you know if the windows tests run in WSL?

I don't.

[bitfield]

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.

[bitfield]

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.

[bitfield]

Nice work so far!

[mahadzaryab1]

@bitfield addressed your feedback! What should we do about the TestWithEnv_UnsetsAllEnvVarsGivenEmptySlice test case? It currently relies on the printenv command and is failing on Windows. We can't echo a specific environment variable in this case since we're not setting anything.

[bitfield]

@bitfield addressed your feedback! What should we do about the TestWithEnv_UnsetsAllEnvVarsGivenEmptySlice test case? It currently relies on the printenv command and is failing on Windows. We can't echo a specific environment variable in this case since we're not setting anything.

Hmm, what about echoing some variable which would usually be there (HOME or PATH, for example), and checking that it's empty? That proves that WithEnv erased the environment.

We'll need a different one on Windows (HOMEPATH)?

[mahadzaryab1]

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

@bitfield I tried a different approach where I do an os.SetEnv and test that it doesn't show up when passing in an empty slice. What do you think of this approach? This was mostly to keep things platform-agnostic so we don't have to write different tests for different platforms.

[bitfield]

Not a bad idea in principle, but unfortunately Setenv isn't safe to use in a parallel test, because it affects the environment for all tests. (If we were going to do that, we could use t.Setenv, which does the cleanup automatically, but it's still not parallelisable.)

Actually, what we care about is not the whole test binary's environment, but just the pipe's, isn't it? When you frame it that way, all we need to do is create a pipe with some environment variable already set, and check that WithEnv unsets it. Something like this, perhaps?

func TestWithEnv_UnsetsAllEnvVarsGivenEmptySlice(t *testing.T) {
	t.Parallel()
	p := script.NewPipe().WithEnv([]string{"ENV1=test1"}).Exec("sh -c 'echo ENV1=$ENV1'")
	want := "ENV1=test1\n"
	got, err := p.String()
	if err != nil {
		t.Fatal(err)
	}
	if got != want {
		t.Fatalf("want %q, got %q", want, got)
	}
	got, err = p.Echo("").WithEnv([]string{}).Exec("sh -c 'echo ENV1=$ENV1'").String()
	if err != nil {
		t.Fatal(err)
	}
	want = "ENV1=\n"
	if got != want {
		t.Errorf("want %q, got %q", want, got)
	}
}

[mahadzaryab1]

Great idea! Thanks @bitfield

[bitfield]

Great job, @mahadzaryab1, thanks a lot! Everybody wants to propose features, and even do quick-and-dirty implementations of them, but that's easy. Few people are willing to do the slow, detailed, careful, high-quality implementation and polish that characterises durable software. You've done that on several issues, and that makes you script's most valuable contributor by far!

Anything else you'd like to work on? I think we have enough to put a release together now, but if you could pick an existing issue to solve, or even suggest a new one, what would it be?

[mahadzaryab1]

Great job, @mahadzaryab1, thanks a lot! Everybody wants to propose features, and even do quick-and-dirty implementations of them, but that's easy. Few people are willing to do the slow, detailed, careful, high-quality implementation and polish that characterises durable software. You've done that on several issues, and that makes you script's most valuable contributor by far!

Anything else you'd like to work on? I think we have enough to put a release together now, but if you could pick an existing issue to solve, or even suggest a new one, what would it be?

@bitfield Thank you so much for your kind words and guidance in helping me working on these issues. I love writing Go and learnt a lot about the language from your code reviews! In terms of proposing new features, I would love to and I'll definitely start to give this more thought and open issues for any ideas that I have. However, in the meantime, I also do want to keep contributing to script so I wanted to ask if there were any features needed implementing? I would love to contribute in whatever way that I can and whatever it is that the users of this package require.

[bitfield]

Great! Well, have a look at the open issues—there are quite a few of them, and most are waiting for some sort of design or further consideration, so I'm sure your input would be very helpful here!