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 <me@hackerc.at> * lint(editorconfig): ignore *_test.go due to mixed style Signed-off-by: hackercat <me@hackerc.at>
2021-12-22 07:37:16 +01:00 · 2021-12-22 07:37:16 +01:00 · adabf2a202
parent 4e0ba618d3
commit adabf2a202
9 changed files with 168 additions and 83 deletions
--- 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

--- 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}'"},
 	}
--- 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
--- 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
-	return func(ctx context.Context) error {
-		var script strings.Builder
-		var err error

 	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
 	}
-		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 {
-			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)
+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()

-		var finalCMD []string
-		if step.Shell == "pwsh" || step.Shell == "powershell" {
-			finalCMD = strings.SplitN(scCmd, " ", 3)
-		} else {
-			finalCMD = strings.Fields(scCmd)
+	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"
 	}

-		for k, v := range finalCMD {
-			if strings.Contains(v, `{0}`) {
-				finalCMD[k] = strings.Replace(v, `{0}`, scriptPath, 1)
-			}
+	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
 }

-		sc.Cmd = finalCMD
+func (sc *StepContext) setupShellCommandExecutor() common.Executor {
+	rc := sc.RunContext
+	return func(ctx context.Context) error {
+		scriptName, script, err := sc.setupShellCommand()
+		if err != nil {
+			return err
+		}

-		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)
 	}
 }
--- 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
--- a/pkg/runner/testdata/shells/custom/push.yml
+++ 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: ''
--- 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
--- 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())
--- 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