From 9884da0122b7fc45595c4f2950c1044fcce2c3e6 Mon Sep 17 00:00:00 2001 From: ChristopherHX Date: Tue, 18 Apr 2023 20:09:57 +0200 Subject: [PATCH] fix: environment handling windows (host mode) (#1732) * fix: environment handling windows (host mode) * fixup * fixup * add more tests * fixup * fix setenv * fixes * [skip ci] Apply suggestions from code review Co-authored-by: Jason Song * Update side effects --------- Co-authored-by: Jason Song Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- pkg/container/executions_environment.go | 2 + pkg/container/host_environment.go | 4 ++ .../linux_container_environment_extensions.go | 4 ++ pkg/runner/action.go | 4 +- pkg/runner/action_composite.go | 14 ++++--- pkg/runner/command.go | 12 ++++-- pkg/runner/run_context.go | 9 +++++ pkg/runner/step.go | 38 ++++++++++++++++--- pkg/runner/step_test.go | 4 +- .../testdata/windows-add-env/action.yml | 7 ++++ pkg/runner/testdata/windows-add-env/push.yml | 17 +++++++++ .../testdata/windows-prepend-path/push.yml | 9 +++++ 12 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 pkg/runner/testdata/windows-add-env/action.yml diff --git a/pkg/container/executions_environment.go b/pkg/container/executions_environment.go index 1c21f943..41e3b57e 100644 --- a/pkg/container/executions_environment.go +++ b/pkg/container/executions_environment.go @@ -10,4 +10,6 @@ type ExecutionsEnvironment interface { DefaultPathVariable() string JoinPathVariable(...string) string GetRunnerContext(ctx context.Context) map[string]interface{} + // On windows PATH and Path are the same key + IsEnvironmentCaseInsensitive() bool } diff --git a/pkg/container/host_environment.go b/pkg/container/host_environment.go index 5d8c7dcd..abc1115a 100644 --- a/pkg/container/host_environment.go +++ b/pkg/container/host_environment.go @@ -419,3 +419,7 @@ func (e *HostEnvironment) ReplaceLogWriter(stdout io.Writer, stderr io.Writer) ( e.StdOut = stdout return org, org } + +func (*HostEnvironment) IsEnvironmentCaseInsensitive() bool { + return runtime.GOOS == "windows" +} diff --git a/pkg/container/linux_container_environment_extensions.go b/pkg/container/linux_container_environment_extensions.go index c369055d..d6734511 100644 --- a/pkg/container/linux_container_environment_extensions.go +++ b/pkg/container/linux_container_environment_extensions.go @@ -71,3 +71,7 @@ func (*LinuxContainerEnvironmentExtensions) GetRunnerContext(ctx context.Context "tool_cache": "/opt/hostedtoolcache", } } + +func (*LinuxContainerEnvironmentExtensions) IsEnvironmentCaseInsensitive() bool { + return false +} diff --git a/pkg/runner/action.go b/pkg/runner/action.go index a534170d..bd74b2e9 100644 --- a/pkg/runner/action.go +++ b/pkg/runner/action.go @@ -319,13 +319,13 @@ func evalDockerArgs(ctx context.Context, step step, action *model.Action, cmd *[ inputs[k] = eval.Interpolate(ctx, v) } } - mergeIntoMap(step.getEnv(), inputs) + mergeIntoMap(step, step.getEnv(), inputs) stepEE := rc.NewStepExpressionEvaluator(ctx, step) for i, v := range *cmd { (*cmd)[i] = stepEE.Interpolate(ctx, v) } - mergeIntoMap(step.getEnv(), action.Runs.Env) + mergeIntoMap(step, step.getEnv(), action.Runs.Env) ee := rc.NewStepExpressionEvaluator(ctx, step) for k, v := range *step.getEnv() { diff --git a/pkg/runner/action_composite.go b/pkg/runner/action_composite.go index b6ef58c8..0fc1fd89 100644 --- a/pkg/runner/action_composite.go +++ b/pkg/runner/action_composite.go @@ -105,13 +105,15 @@ func execAsComposite(step actionStep) common.Executor { rc.Masks = append(rc.Masks, compositeRC.Masks...) rc.ExtraPath = compositeRC.ExtraPath // compositeRC.Env is dirty, contains INPUT_ and merged step env, only rely on compositeRC.GlobalEnv - for k, v := range compositeRC.GlobalEnv { - rc.Env[k] = v - if rc.GlobalEnv == nil { - rc.GlobalEnv = map[string]string{} - } - rc.GlobalEnv[k] = v + mergeIntoMap := mergeIntoMapCaseSensitive + if rc.JobContainer.IsEnvironmentCaseInsensitive() { + mergeIntoMap = mergeIntoMapCaseInsensitive } + if rc.GlobalEnv == nil { + rc.GlobalEnv = map[string]string{} + } + mergeIntoMap(rc.GlobalEnv, compositeRC.GlobalEnv) + mergeIntoMap(rc.Env, compositeRC.GlobalEnv) return err } diff --git a/pkg/runner/command.go b/pkg/runner/command.go index f14eb7aa..e0abb645 100644 --- a/pkg/runner/command.go +++ b/pkg/runner/command.go @@ -87,12 +87,18 @@ func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg if rc.Env == nil { rc.Env = make(map[string]string) } - rc.Env[name] = arg - // for composite action GITHUB_ENV and set-env passing if rc.GlobalEnv == nil { rc.GlobalEnv = map[string]string{} } - rc.GlobalEnv[name] = arg + newenv := map[string]string{ + name: arg, + } + mergeIntoMap := mergeIntoMapCaseSensitive + if rc.JobContainer != nil && rc.JobContainer.IsEnvironmentCaseInsensitive() { + mergeIntoMap = mergeIntoMapCaseInsensitive + } + mergeIntoMap(rc.Env, newenv) + mergeIntoMap(rc.GlobalEnv, newenv) } func (rc *RunContext) setOutput(ctx context.Context, kvPairs map[string]string, arg string) { logger := common.Logger(ctx) diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index bbd8ec4a..fb04f89c 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -298,6 +298,15 @@ func (rc *RunContext) execJobContainer(cmd []string, env map[string]string, user func (rc *RunContext) ApplyExtraPath(ctx context.Context, env *map[string]string) { if rc.ExtraPath != nil && len(rc.ExtraPath) > 0 { path := rc.JobContainer.GetPathVariableName() + if rc.JobContainer.IsEnvironmentCaseInsensitive() { + // On windows system Path and PATH could also be in the map + for k := range *env { + if strings.EqualFold(path, k) { + path = k + break + } + } + } if (*env)[path] == "" { cenv := map[string]string{} var cpath string diff --git a/pkg/runner/step.go b/pkg/runner/step.go index 7cc355f4..cff48b1b 100644 --- a/pkg/runner/step.go +++ b/pkg/runner/step.go @@ -187,7 +187,7 @@ func setupEnv(ctx context.Context, step step) error { mergeEnv(ctx, step) // merge step env last, since it should not be overwritten - mergeIntoMap(step.getEnv(), step.getStepModel().GetEnv()) + mergeIntoMap(step, step.getEnv(), step.getStepModel().GetEnv()) exprEval := rc.NewExpressionEvaluator(ctx) for k, v := range *step.getEnv() { @@ -216,9 +216,9 @@ func mergeEnv(ctx context.Context, step step) { c := job.Container() if c != nil { - mergeIntoMap(env, rc.GetEnv(), c.Env) + mergeIntoMap(step, env, rc.GetEnv(), c.Env) } else { - mergeIntoMap(env, rc.GetEnv()) + mergeIntoMap(step, env, rc.GetEnv()) } rc.withGithubEnv(ctx, step.getGithubContext(ctx), *env) @@ -258,10 +258,38 @@ func isContinueOnError(ctx context.Context, expr string, step step, stage stepSt return continueOnError, nil } -func mergeIntoMap(target *map[string]string, maps ...map[string]string) { +func mergeIntoMap(step step, target *map[string]string, maps ...map[string]string) { + if rc := step.getRunContext(); rc != nil && rc.JobContainer != nil && rc.JobContainer.IsEnvironmentCaseInsensitive() { + mergeIntoMapCaseInsensitive(*target, maps...) + } else { + mergeIntoMapCaseSensitive(*target, maps...) + } +} + +func mergeIntoMapCaseSensitive(target map[string]string, maps ...map[string]string) { for _, m := range maps { for k, v := range m { - (*target)[k] = v + target[k] = v + } + } +} + +func mergeIntoMapCaseInsensitive(target map[string]string, maps ...map[string]string) { + foldKeys := make(map[string]string, len(target)) + for k := range target { + foldKeys[strings.ToLower(k)] = k + } + toKey := func(s string) string { + foldKey := strings.ToLower(s) + if k, ok := foldKeys[foldKey]; ok { + return k + } + foldKeys[strings.ToLower(foldKey)] = s + return s + } + for _, m := range maps { + for k, v := range m { + target[toKey(k)] = v } } } diff --git a/pkg/runner/step_test.go b/pkg/runner/step_test.go index 4fc77652..d08a1297 100644 --- a/pkg/runner/step_test.go +++ b/pkg/runner/step_test.go @@ -63,7 +63,9 @@ func TestMergeIntoMap(t *testing.T) { for _, tt := range table { t.Run(tt.name, func(t *testing.T) { - mergeIntoMap(&tt.target, tt.maps...) + mergeIntoMapCaseSensitive(tt.target, tt.maps...) + assert.Equal(t, tt.expected, tt.target) + mergeIntoMapCaseInsensitive(tt.target, tt.maps...) assert.Equal(t, tt.expected, tt.target) }) } diff --git a/pkg/runner/testdata/windows-add-env/action.yml b/pkg/runner/testdata/windows-add-env/action.yml new file mode 100644 index 00000000..a80684f3 --- /dev/null +++ b/pkg/runner/testdata/windows-add-env/action.yml @@ -0,0 +1,7 @@ +runs: + using: composite + steps: + - run: | + echo $env:GITHUB_ENV + echo "kEy=n/a" > $env:GITHUB_ENV + shell: pwsh \ No newline at end of file diff --git a/pkg/runner/testdata/windows-add-env/push.yml b/pkg/runner/testdata/windows-add-env/push.yml index 275c5f1f..bd233bb3 100644 --- a/pkg/runner/testdata/windows-add-env/push.yml +++ b/pkg/runner/testdata/windows-add-env/push.yml @@ -25,3 +25,20 @@ jobs: echo "Unexpected value for `$env:key2: $env:key2" exit 1 } + - run: | + echo $env:GITHUB_ENV + echo "KEY=test" > $env:GITHUB_ENV + echo "Key=expected" > $env:GITHUB_ENV + - name: Assert GITHUB_ENV is merged case insensitive + run: exit 1 + if: env.KEY != 'expected' || env.Key != 'expected' || env.key != 'expected' + - name: Assert step env is merged case insensitive + run: exit 1 + if: env.KEY != 'n/a' || env.Key != 'n/a' || env.key != 'n/a' + env: + KeY: 'n/a' + - uses: actions/checkout@v3 + - uses: ./windows-add-env + - name: Assert composite env is merged case insensitive + run: exit 1 + if: env.KEY != 'n/a' || env.Key != 'n/a' || env.key != 'n/a' \ No newline at end of file diff --git a/pkg/runner/testdata/windows-prepend-path/push.yml b/pkg/runner/testdata/windows-prepend-path/push.yml index 176de691..cf5026a6 100644 --- a/pkg/runner/testdata/windows-prepend-path/push.yml +++ b/pkg/runner/testdata/windows-prepend-path/push.yml @@ -11,6 +11,9 @@ jobs: mkdir build echo '@echo off' > build/test.cmd echo 'echo Hi' >> build/test.cmd + mkdir build2 + echo '@echo off' > build2/test2.cmd + echo 'echo test2' >> build2/test2.cmd - run: | echo '${{ tojson(runner) }}' ls @@ -23,3 +26,9 @@ jobs: - run: | echo $env:PATH test + - run: | + echo "PATH=$env:PATH;${{ github.workspace }}\build2" > $env:GITHUB_ENV + - run: | + echo $env:PATH + test + test2 \ No newline at end of file