Merge pull request #2017 from buildkite/env-apply-in-place

Make env.Environment.Merge and .Apply operate in-place

bootstrap/bootstrap.go

@@ -377,15 +377,15 @@ func (b *Bootstrap) applyEnvironmentChanges(changes hook.HookScriptChanges, reda
 		return
 	}
 
-	mergedEnv := b.shell.Env.Apply(changes.Diff)
+	b.shell.Env.Apply(changes.Diff)
 
 	// reset output redactors based on new environment variable values
 	redactors.Flush()
-	redactors.Reset(redaction.GetValuesToRedact(b.shell, b.Config.RedactedVars, mergedEnv.Dump()))
+	redactors.Reset(redaction.GetValuesToRedact(b.shell, b.Config.RedactedVars, b.shell.Env.Dump()))
 
 	// First, let see any of the environment variables are supposed
 	// to change the bootstrap configuration at run time.
-	bootstrapConfigEnvChanges := b.Config.ReadFromEnvironment(mergedEnv)
+	bootstrapConfigEnvChanges := b.Config.ReadFromEnvironment(b.shell.Env)
 
 	// Print out the env vars that changed. As we go through each
 	// one, we'll determine if it was a special "bootstrap"
@@ -418,11 +418,6 @@ func (b *Bootstrap) applyEnvironmentChanges(changes hook.HookScriptChanges, reda
 			b.shell.Commentf("%s removed", k)
 		}
 	}
-
-	// Now that we've finished telling the user what's changed,
-	// let's mutate the current shell environment to include all
-	// the new values.
-	b.shell.Env = mergedEnv
 }
 
 func (b *Bootstrap) hasGlobalHook(name string) bool {

bootstrap/shell/shell.go

@@ -416,9 +416,9 @@ func (s *Shell) RunScript(ctx context.Context, path string, extra *env.Environme
 	}
 
 	// Combine the two slices of env, let the latter overwrite the former
-	currentEnv := env.FromSlice(cmd.Env)
-	customEnv := currentEnv.Merge(extra)
-	cmd.Env = customEnv.ToSlice()
+	environ := env.FromSlice(cmd.Env)
+	environ.Merge(extra)
+	cmd.Env = environ.ToSlice()
 
 	return s.executeCommand(ctx, cmd, s.Writer, executeFlags{
 		Stdout: true,

env/environment.go

@@ -172,35 +172,27 @@ func (e *Environment) Diff(other *Environment) Diff {
 }
 
 // Merge merges another env into this one and returns the result
-func (e *Environment) Merge(other *Environment) *Environment {
-	c := e.Copy()
-
+func (e *Environment) Merge(other *Environment) {
 	if other == nil {
-		return c
+		return
 	}
 
 	other.underlying.Range(func(k, v string) bool {
-		c.Set(k, v)
+		e.Set(k, v)
 		return true
 	})
-
-	return c
 }
 
-func (e *Environment) Apply(diff Diff) *Environment {
-	c := e.Copy()
-
+func (e *Environment) Apply(diff Diff) {
 	for k, v := range diff.Added {
-		c.Set(k, v)
+		e.Set(k, v)
 	}
 	for k, v := range diff.Changed {
-		c.Set(k, v.New)
+		e.Set(k, v.New)
 	}
 	for k := range diff.Removed {
-		c.underlying.Delete(k)
+		e.Remove(k)
 	}
-
-	return c
 }
 
 // Copy returns a copy of the env

env/environment_test.go

@@ -112,9 +112,9 @@ func TestEnvironmentMerge(t *testing.T) {
 	env1 := FromSlice([]string{"FOO=bar"})
 	env2 := FromSlice([]string{"BAR=foo"})
 
-	env3 := env1.Merge(env2)
+	env1.Merge(env2)
 
-	assert.Equal(t, env3.ToSlice(), []string{"BAR=foo", "FOO=bar"})
+	assert.Equal(t, env1.ToSlice(), []string{"BAR=foo", "FOO=bar"})
 }
 
 func TestEnvironmentCopy(t *testing.T) {
@@ -215,7 +215,7 @@ func TestEnvironmentApply(t *testing.T) {
 	t.Parallel()
 
 	env := New()
-	env = env.Apply(Diff{
+	env.Apply(Diff{
 		Added: map[string]string{
 			"LLAMAS_ENABLED": "1",
 		},
@@ -226,7 +226,7 @@ func TestEnvironmentApply(t *testing.T) {
 		"LLAMAS_ENABLED=1",
 	}).Dump(), env.Dump())
 
-	env = env.Apply(Diff{
+	env.Apply(Diff{
 		Added: map[string]string{
 			"ALPACAS_ENABLED": "1",
 		},
@@ -243,7 +243,7 @@ func TestEnvironmentApply(t *testing.T) {
 		"LLAMAS_ENABLED=0",
 	}).Dump(), env.Dump())
 
-	env = env.Apply(Diff{
+	env.Apply(Diff{
 		Added:   map[string]string{},
 		Changed: map[string]DiffPair{},
 		Removed: map[string]struct{}{

hook/scriptwrapper_test.go

@@ -59,7 +59,8 @@ func TestRunningHookDetectsChangedEnvironment(t *testing.T) {
 	// Windows’ batch 'SET >' normalises environment variables case so we apply
 	// the 'expected' and 'actual' diffs to a blank Environment which handles
 	// case normalisation for us
-	expected := env.New().Apply(env.Diff{
+	expected := env.New()
+	expected.Apply(env.Diff{
 		Added: map[string]string{
 			"LLAMAS":  "rock",
 			"Alpacas": "are ok",
@@ -68,7 +69,8 @@ func TestRunningHookDetectsChangedEnvironment(t *testing.T) {
 		Removed: map[string]struct{}{},
 	})
 
-	actual := env.New().Apply(changes.Diff)
+	actual := env.New()
+	actual.Apply(changes.Diff)
 
 	// The strict equals check here also ensures we aren't bubbling up the
 	// internal BUILDKITE_HOOK_EXIT_STATUS and BUILDKITE_HOOK_WORKING_DIR