diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index 934ebb3adf..5db91a3f42 100644 --- a/bootstrap/bootstrap.go +++ b/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 { diff --git a/bootstrap/shell/shell.go b/bootstrap/shell/shell.go index dda84dc39c..39a3dcb7f3 100644 --- a/bootstrap/shell/shell.go +++ b/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, diff --git a/env/environment.go b/env/environment.go index 241164bfa9..37674f98c2 100644 --- a/env/environment.go +++ b/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 diff --git a/env/environment_test.go b/env/environment_test.go index 6da356b5b5..3ff786f63e 100644 --- a/env/environment_test.go +++ b/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{}{ diff --git a/hook/scriptwrapper_test.go b/hook/scriptwrapper_test.go index 7fe285722a..0dd2dcc1e0 100644 --- a/hook/scriptwrapper_test.go +++ b/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