From adabf2a2027fa9da3410febc474d55327e2abeb7 Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 22 Dec 2021 07:37:16 +0100 Subject: [PATCH] fix: rework setupShellCommand (#930) * fix: rework `setupShellCommand` * move all logic to separate function so we can test that later * split `step.Shell` and `step.WorkingDirectory` setup into own funcs * general cleanup of function * use `ActPath` to not collide with checked out repository * use `shellquote.Split()` instead of `strings.Fields()` for better command split * replace single string concat with `fmt` Signed-off-by: hackercat * lint(editorconfig): ignore *_test.go due to mixed style Signed-off-by: hackercat --- .editorconfig | 2 +- pkg/model/workflow_test.go | 1 + pkg/runner/runner_test.go | 2 + pkg/runner/step_context.go | 167 ++++++++++++--------- pkg/runner/testdata/shells/bash/push.yml | 18 ++- pkg/runner/testdata/shells/custom/push.yml | 14 ++ pkg/runner/testdata/shells/pwsh/push.yml | 14 +- pkg/runner/testdata/shells/python/push.yml | 15 +- pkg/runner/testdata/shells/sh/push.yml | 18 ++- 9 files changed, 168 insertions(+), 83 deletions(-) create mode 100644 pkg/runner/testdata/shells/custom/push.yml diff --git a/.editorconfig b/.editorconfig index c60bde8..8f8872f 100644 --- a/.editorconfig +++ b/.editorconfig @@ -12,7 +12,7 @@ indent_size = 4 indent_style = space indent_size = 2 -[{Dockerfile,*.md}] +[{Dockerfile,*.md,*_test.go}] indent_style = unset indent_size = unset diff --git a/pkg/model/workflow_test.go b/pkg/model/workflow_test.go index 86b4380..a3b08e4 100644 --- a/pkg/model/workflow_test.go +++ b/pkg/model/workflow_test.go @@ -266,6 +266,7 @@ func TestStep_ShellCommand(t *testing.T) { shell string want string }{ + {"pwsh -v '. {0}'", "pwsh -v '. {0}'"}, {"pwsh", "pwsh -command . '{0}'"}, {"powershell", "powershell -command . '{0}'"}, } diff --git a/pkg/runner/runner_test.go b/pkg/runner/runner_test.go index 7ca000c..dc31257 100644 --- a/pkg/runner/runner_test.go +++ b/pkg/runner/runner_test.go @@ -98,6 +98,8 @@ func TestRunEvent(t *testing.T) { {"testdata", "runs-on", "push", "", platforms, ""}, {"testdata", "checkout", "push", "", platforms, ""}, {"testdata", "shells/defaults", "push", "", platforms, ""}, + // TODO: figure out why it fails + // {"testdata", "shells/custom", "push", "", map[string]string{"ubuntu-latest": "ghcr.io/justingrote/act-pwsh:latest"}, ""}, // custom image with pwsh {"testdata", "shells/pwsh", "push", "", map[string]string{"ubuntu-latest": "ghcr.io/justingrote/act-pwsh:latest"}, ""}, // custom image with pwsh {"testdata", "shells/bash", "push", "", platforms, ""}, {"testdata", "shells/python", "push", "", map[string]string{"ubuntu-latest": "node:12-buster"}, ""}, // slim doesn't have python diff --git a/pkg/runner/step_context.go b/pkg/runner/step_context.go index e69aa40..00c1527 100644 --- a/pkg/runner/step_context.go +++ b/pkg/runner/step_context.go @@ -53,7 +53,7 @@ func (sc *StepContext) Executor() common.Executor { switch step.Type() { case model.StepTypeRun: return common.NewPipelineExecutor( - sc.setupShellCommand(), + sc.setupShellCommandExecutor(), sc.execJobContainer(), ) @@ -185,87 +185,106 @@ func (sc *StepContext) setupEnv(ctx context.Context) (ExpressionEvaluator, error return evaluator, nil } -// nolint:gocyclo -func (sc *StepContext) setupShellCommand() common.Executor { +func (sc *StepContext) setupWorkingDirectory() { rc := sc.RunContext step := sc.Step + + if step.WorkingDirectory == "" { + step.WorkingDirectory = rc.Run.Job().Defaults.Run.WorkingDirectory + } + + // jobs can receive context values, so we interpolate + step.WorkingDirectory = rc.ExprEval.Interpolate(step.WorkingDirectory) + + // but top level keys in workflow file like `defaults` or `env` can't + if step.WorkingDirectory == "" { + step.WorkingDirectory = rc.Run.Workflow.Defaults.Run.WorkingDirectory + } +} + +func (sc *StepContext) setupShell() { + rc := sc.RunContext + step := sc.Step + + if step.Shell == "" { + step.Shell = rc.Run.Job().Defaults.Run.Shell + } + + step.Shell = rc.ExprEval.Interpolate(step.Shell) + + if step.Shell == "" { + step.Shell = rc.Run.Workflow.Defaults.Run.Shell + } + + // current GitHub Runner behaviour is that default is `sh`, + // but if it's not container it validates with `which` command + // if `bash` is available, and provides `bash` if it is + // for now I'm going to leave below logic, will address it in different PR + // https://github.com/actions/runner/blob/9a829995e02d2db64efb939dc2f283002595d4d9/src/Runner.Worker/Handlers/ScriptHandler.cs#L87-L91 + if rc.Run.Job().Container() != nil { + if rc.Run.Job().Container().Image != "" && step.Shell == "" { + step.Shell = "sh" + } + } +} + +// TODO: Currently we just ignore top level keys, BUT we should return proper error on them +// BUTx2 I leave this for when we rewrite act to use actionlint for workflow validation +// so we return proper errors before any execution or spawning containers +// it will error anyway with: +// OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: "${{": executable file not found in $PATH: unknown +func (sc *StepContext) setupShellCommand() (name, script string, err error) { + sc.setupShell() + sc.setupWorkingDirectory() + + step := sc.Step + + script = sc.RunContext.ExprEval.Interpolate(step.Run) + + scCmd := step.ShellCommand() + + name = fmt.Sprintf("workflow/%s", step.ID) + + // Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L47-L64 + // Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L19-L27 + runPrepend := "" + runAppend := "" + switch step.Shell { + case "bash", "sh": + name += ".sh" + case "pwsh", "powershell": + name += ".ps1" + runPrepend = "$ErrorActionPreference = 'stop'" + runAppend = "if ((Test-Path -LiteralPath variable:/LASTEXITCODE)) { exit $LASTEXITCODE }" + case "cmd": + name += ".cmd" + runPrepend = "@echo off" + case "python": + name += ".py" + } + + script = fmt.Sprintf("%s\n%s\n%s", runPrepend, script, runAppend) + + log.Debugf("Wrote command \n%s\n to '%s'", script, name) + + scriptPath := fmt.Sprintf("%s/%s", ActPath, name) + sc.Cmd, err = shellquote.Split(strings.Replace(scCmd, `{0}`, scriptPath, 1)) + + return name, script, err +} + +func (sc *StepContext) setupShellCommandExecutor() common.Executor { + rc := sc.RunContext return func(ctx context.Context) error { - var script strings.Builder - var err error - - if step.WorkingDirectory == "" { - step.WorkingDirectory = rc.Run.Job().Defaults.Run.WorkingDirectory - } - if step.WorkingDirectory == "" { - step.WorkingDirectory = rc.Run.Workflow.Defaults.Run.WorkingDirectory - } - step.WorkingDirectory = rc.ExprEval.Interpolate(step.WorkingDirectory) - - run := rc.ExprEval.Interpolate(step.Run) - step.Shell = rc.ExprEval.Interpolate(step.Shell) - - if _, err = script.WriteString(run); err != nil { + scriptName, script, err := sc.setupShellCommand() + if err != nil { return err } - scriptName := fmt.Sprintf("workflow/%s", step.ID) - // Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L47-L64 - // Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L19-L27 - runPrepend := "" - runAppend := "" - scriptExt := "" - switch step.Shell { - case "bash", "sh": - scriptExt = ".sh" - case "pwsh", "powershell": - scriptExt = ".ps1" - runPrepend = "$ErrorActionPreference = 'stop'" - runAppend = "if ((Test-Path -LiteralPath variable:/LASTEXITCODE)) { exit $LASTEXITCODE }" - case "cmd": - scriptExt = ".cmd" - runPrepend = "@echo off" - case "python": - scriptExt = ".py" - } - - scriptName += scriptExt - run = runPrepend + "\n" + run + "\n" + runAppend - - log.Debugf("Wrote command '%s' to '%s'", run, scriptName) - scriptPath := fmt.Sprintf("%s/%s", rc.Config.ContainerWorkdir(), scriptName) - - if step.Shell == "" { - step.Shell = rc.Run.Job().Defaults.Run.Shell - } - if step.Shell == "" { - step.Shell = rc.Run.Workflow.Defaults.Run.Shell - } - if rc.Run.Job().Container() != nil { - if rc.Run.Job().Container().Image != "" && step.Shell == "" { - step.Shell = "sh" - } - } - scCmd := step.ShellCommand() - - var finalCMD []string - if step.Shell == "pwsh" || step.Shell == "powershell" { - finalCMD = strings.SplitN(scCmd, " ", 3) - } else { - finalCMD = strings.Fields(scCmd) - } - - for k, v := range finalCMD { - if strings.Contains(v, `{0}`) { - finalCMD[k] = strings.Replace(v, `{0}`, scriptPath, 1) - } - } - - sc.Cmd = finalCMD - - return rc.JobContainer.Copy(rc.Config.ContainerWorkdir(), &container.FileEntry{ + return rc.JobContainer.Copy(ActPath, &container.FileEntry{ Name: scriptName, Mode: 0755, - Body: script.String(), + Body: script, })(ctx) } } diff --git a/pkg/runner/testdata/shells/bash/push.yml b/pkg/runner/testdata/shells/bash/push.yml index 7032899..0685fdd 100644 --- a/pkg/runner/testdata/shells/bash/push.yml +++ b/pkg/runner/testdata/shells/bash/push.yml @@ -1,9 +1,11 @@ on: push +env: + MY_SHELL: bash jobs: check: runs-on: ubuntu-latest steps: - - shell: bash + - shell: ${{ env.MY_SHELL }} run: | if [[ -n "$BASH" ]]; then echo "I'm $BASH!" @@ -14,10 +16,22 @@ jobs: runs-on: ubuntu-latest container: node:12-buster-slim steps: - - shell: bash + - shell: ${{ env.MY_SHELL }} run: | if [[ -n "$BASH" ]]; then echo "I'm $BASH!" else exit 1 fi + check-job-default: + runs-on: ubuntu-latest + defaults: + run: + shell: ${{ env.MY_SHELL }} + steps: + - run: | + if [[ -n "$BASH" ]]; then + echo "I'm $BASH!" + else + exit 1 + fi diff --git a/pkg/runner/testdata/shells/custom/push.yml b/pkg/runner/testdata/shells/custom/push.yml new file mode 100644 index 0000000..d5086af --- /dev/null +++ b/pkg/runner/testdata/shells/custom/push.yml @@ -0,0 +1,14 @@ +on: push +jobs: + check: + runs-on: ubuntu-latest + steps: + # prints version and exits, it's not valid (for github) if {0} is not included + - shell: pwsh -v '. {0}' + run: '' + check-container: + runs-on: ubuntu-latest + container: ghcr.io/justingrote/act-pwsh:latest + steps: + - shell: pwsh -v '. {0}' + run: '' diff --git a/pkg/runner/testdata/shells/pwsh/push.yml b/pkg/runner/testdata/shells/pwsh/push.yml index 0aa89f7..d1aa0da 100644 --- a/pkg/runner/testdata/shells/pwsh/push.yml +++ b/pkg/runner/testdata/shells/pwsh/push.yml @@ -1,15 +1,25 @@ on: push +env: + MY_SHELL: pwsh jobs: check: runs-on: ubuntu-latest steps: - - shell: pwsh + - shell: ${{ env.MY_SHELL }} run: | $PSVersionTable check-container: runs-on: ubuntu-latest container: ghcr.io/justingrote/act-pwsh:latest steps: - - shell: pwsh + - shell: ${{ env.MY_SHELL }} run: | $PSVersionTable + check-job-default: + runs-on: ubuntu-latest + defaults: + run: + shell: ${{ env.MY_SHELL }} + steps: + - run: | + $PSVersionTable diff --git a/pkg/runner/testdata/shells/python/push.yml b/pkg/runner/testdata/shells/python/push.yml index 19870c4..11d0192 100644 --- a/pkg/runner/testdata/shells/python/push.yml +++ b/pkg/runner/testdata/shells/python/push.yml @@ -1,9 +1,11 @@ on: push +env: + MY_SHELL: python jobs: check: runs-on: ubuntu-latest steps: - - shell: python + - shell: ${{ env.MY_SHELL }} run: | import platform print(platform.python_version()) @@ -11,7 +13,16 @@ jobs: runs-on: ubuntu-latest container: node:12-buster steps: - - shell: python + - shell: ${{ env.MY_SHELL }} run: | import platform print(platform.python_version()) + check-job-default: + runs-on: ubuntu-latest + defaults: + run: + shell: ${{ env.MY_SHELL }} + steps: + - run: | + import platform + print(platform.python_version()) diff --git a/pkg/runner/testdata/shells/sh/push.yml b/pkg/runner/testdata/shells/sh/push.yml index 0c752be..0914ca2 100644 --- a/pkg/runner/testdata/shells/sh/push.yml +++ b/pkg/runner/testdata/shells/sh/push.yml @@ -1,9 +1,11 @@ on: push +env: + MY_SHELL: sh jobs: check: runs-on: ubuntu-latest steps: - - shell: sh + - shell: ${{ env.MY_SHELL }} run: | if [ -z ${BASH+x} ]; then echo "I'm sh!" @@ -14,10 +16,22 @@ jobs: runs-on: ubuntu-latest container: alpine:latest steps: - - shell: sh + - shell: ${{ env.MY_SHELL }} run: | if [ -z ${BASH+x} ]; then echo "I'm sh!" else exit 1 fi + check-job-default: + runs-on: ubuntu-latest + defaults: + run: + shell: ${{ env.MY_SHELL }} + steps: + - run: | + if [ -z ${BASH+x} ]; then + echo "I'm sh!" + else + exit 1 + fi