diff --git a/rule_pyflakes.go b/rule_pyflakes.go index b09201a15..5e4b62719 100644 --- a/rule_pyflakes.go +++ b/rule_pyflakes.go @@ -3,6 +3,7 @@ package actionlint import ( "bytes" "fmt" + "strings" "sync" ) @@ -18,7 +19,7 @@ func getShellIsPythonKind(shell *String) shellIsPythonKind { if shell == nil { return shellIsPythonKindUnspecified } - if shell.Value == "python" { + if shell.Value == "python" || strings.HasPrefix(shell.Value, "python ") { return shellIsPythonKindPython } return shellIsPythonKindNotPython @@ -34,15 +35,8 @@ type RulePyflakes struct { mu sync.Mutex } -// NewRulePyflakes creates new RulePyflakes instance. Parameter executable can be command name -// or relative/absolute file path. When the given executable is not found in system, it returns -// an error. -func NewRulePyflakes(executable string, proc *concurrentProcess) (*RulePyflakes, error) { - cmd, err := proc.newCommandRunner(executable) - if err != nil { - return nil, err - } - r := &RulePyflakes{ +func newRulePyflakes(cmd *externalCommand) *RulePyflakes { + return &RulePyflakes{ RuleBase: RuleBase{ name: "pyflakes", desc: "Checks for Python script when \"shell: python\" is configured using Pyflakes", @@ -51,7 +45,17 @@ func NewRulePyflakes(executable string, proc *concurrentProcess) (*RulePyflakes, workflowShellIsPython: shellIsPythonKindUnspecified, jobShellIsPython: shellIsPythonKindUnspecified, } - return r, nil +} + +// NewRulePyflakes creates new RulePyflakes instance. Parameter executable can be command name +// or relative/absolute file path. When the given executable is not found in system, it returns +// an error. +func NewRulePyflakes(executable string, proc *concurrentProcess) (*RulePyflakes, error) { + cmd, err := proc.newCommandRunner(executable) + if err != nil { + return nil, err + } + return newRulePyflakes(cmd), nil } // VisitJobPre is callback when visiting Job node before visiting its children. @@ -98,8 +102,8 @@ func (rule *RulePyflakes) VisitStep(n *Step) error { } func (rule *RulePyflakes) isPythonShell(r *ExecRun) bool { - if r.Shell != nil { - return r.Shell.Value == "python" + if k := getShellIsPythonKind(r.Shell); k != shellIsPythonKindUnspecified { + return k == shellIsPythonKindPython } if rule.jobShellIsPython != shellIsPythonKindUnspecified { diff --git a/rule_pyflakes_test.go b/rule_pyflakes_test.go new file mode 100644 index 000000000..e2555d446 --- /dev/null +++ b/rule_pyflakes_test.go @@ -0,0 +1,84 @@ +package actionlint + +import ( + "testing" +) + +func TestRulePyflakesDetectPythonShell(t *testing.T) { + tests := []struct { + what string + isPython bool + workflow string // Shell name set at 'defaults' in Workflow node + job string // Shell name set at 'defaults' in Job node + step string // Shell name set at 'shell' in Step node + }{ + { + what: "no default shell", + isPython: false, + }, + { + what: "workflow default", + isPython: true, + workflow: "python", + }, + { + what: "job default", + isPython: true, + job: "python", + }, + { + what: "step shell", + isPython: true, + step: "python", + }, + { + what: "custom shell", + isPython: true, + workflow: "python {0}", + }, + { + what: "other shell", + isPython: false, + workflow: "pwsh", + }, + { + what: "other custom shell", + isPython: false, + workflow: "bash -e {0}", + }, + } + + for _, tc := range tests { + t.Run(tc.what, func(t *testing.T) { + r := newRulePyflakes(&externalCommand{}) + + w := &Workflow{} + if tc.workflow != "" { + w.Defaults = &Defaults{ + Run: &DefaultsRun{ + Shell: &String{Value: tc.workflow}, + }, + } + } + r.VisitWorkflowPre(w) + + j := &Job{} + if tc.job != "" { + j.Defaults = &Defaults{ + Run: &DefaultsRun{ + Shell: &String{Value: tc.job}, + }, + } + } + r.VisitJobPre(j) + + e := &ExecRun{} + if tc.step != "" { + e.Shell = &String{Value: tc.step} + } + if have := r.isPythonShell(e); have != tc.isPython { + t.Fatalf("Actual isPython=%v but wanted isPython=%v", have, tc.isPython) + } + }) + } +} diff --git a/rule_shellcheck.go b/rule_shellcheck.go index b10ba220d..ccf9e5ff2 100644 --- a/rule_shellcheck.go +++ b/rule_shellcheck.go @@ -22,18 +22,12 @@ type RuleShellcheck struct { cmd *externalCommand workflowShell string jobShell string + runnerShell string mu sync.Mutex } -// NewRuleShellcheck creates new RuleShellcheck instance. Parameter executable can be command name -// or relative/absolute file path. When the given executable is not found in system, it returns an -// error as 2nd return value. -func NewRuleShellcheck(executable string, proc *concurrentProcess) (*RuleShellcheck, error) { - cmd, err := proc.newCommandRunner(executable) - if err != nil { - return nil, err - } - r := &RuleShellcheck{ +func newRuleShellcheck(cmd *externalCommand) *RuleShellcheck { + return &RuleShellcheck{ RuleBase: RuleBase{ name: "shellcheck", desc: "Checks for shell script sources in \"run:\" using shellcheck", @@ -41,8 +35,19 @@ func NewRuleShellcheck(executable string, proc *concurrentProcess) (*RuleShellch cmd: cmd, workflowShell: "", jobShell: "", + runnerShell: "", } - return r, nil +} + +// NewRuleShellcheck creates new RuleShellcheck instance. The executable argument can be command +// name or relative/absolute file path. When the given executable is not found in system, it returns +// an error as 2nd return value. +func NewRuleShellcheck(executable string, proc *concurrentProcess) (*RuleShellcheck, error) { + cmd, err := proc.newCommandRunner(executable) + if err != nil { + return nil, err + } + return newRuleShellcheck(cmd), nil } // VisitStep is callback when visiting Step node. @@ -52,12 +57,7 @@ func (rule *RuleShellcheck) VisitStep(n *Step) error { return nil } - name := rule.getShellName(run) - if name != "bash" && name != "sh" { - return nil - } - - rule.runShellcheck(run.Run.Value, name, run.RunPos) + rule.runShellcheck(run.Run.Value, rule.getShellName(run), run.RunPos) return nil } @@ -65,31 +65,27 @@ func (rule *RuleShellcheck) VisitStep(n *Step) error { func (rule *RuleShellcheck) VisitJobPre(n *Job) error { if n.Defaults != nil && n.Defaults.Run != nil && n.Defaults.Run.Shell != nil { rule.jobShell = n.Defaults.Run.Shell.Value - return nil } - if n.RunsOn == nil { - return nil - } - - for _, label := range n.RunsOn.Labels { - l := strings.ToLower(label.Value) - // Default shell on Windows is PowerShell. - // https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#using-a-specific-shell - if l == "windows" || strings.HasPrefix(l, "windows-") { - return nil + if n.RunsOn != nil { + for _, label := range n.RunsOn.Labels { + l := strings.ToLower(label.Value) + // Default shell on Windows is PowerShell. + // https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#using-a-specific-shell + if l == "windows" || strings.HasPrefix(l, "windows-") { + rule.runnerShell = "pwsh" + break + } } } - // TODO: When bash is not found, GitHub-hosted runner fallbacks to sh. What OSes require this behavior? - rule.jobShell = "bash" - return nil } // VisitJobPost is callback when visiting Job node after visiting its children. func (rule *RuleShellcheck) VisitJobPost(n *Job) error { rule.jobShell = "" + rule.runnerShell = "" return nil } @@ -114,7 +110,15 @@ func (rule *RuleShellcheck) getShellName(exec *ExecRun) string { if rule.jobShell != "" { return rule.jobShell } - return rule.workflowShell + if rule.workflowShell != "" { + return rule.workflowShell + } + if rule.runnerShell != "" { + return rule.runnerShell + } + // Note: Default shell on Windows is pwsh so this value is not always correct. + // Note: When bash is not found, GitHub-hosted runner fallbacks to sh. + return "bash" } // Replace ${{ ... }} with underscores like __________ @@ -156,7 +160,18 @@ func sanitizeExpressionsInScript(src string) string { } } -func (rule *RuleShellcheck) runShellcheck(src, sh string, pos *Pos) { +func (rule *RuleShellcheck) runShellcheck(src, shell string, pos *Pos) { + var sh string + if shell == "bash" || shell == "sh" { + sh = shell + } else if strings.HasPrefix(shell, "bash ") { + sh = "bash" + } else if strings.HasPrefix(shell, "sh ") { + sh = "sh" + } else { + return // Skip checking this shell script since shellcheck doesn't support it + } + src = sanitizeExpressionsInScript(src) rule.Debug("%s: Run shellcheck for %s script:\n%s", pos, sh, src) diff --git a/rule_shellcheck_test.go b/rule_shellcheck_test.go index f445c4597..862e9114a 100644 --- a/rule_shellcheck_test.go +++ b/rule_shellcheck_test.go @@ -1,7 +1,7 @@ package actionlint import ( - "strconv" + "fmt" "testing" ) @@ -65,7 +65,7 @@ func TestRuleShellcheckSanitizeExpressionsInScript(t *testing.T) { } for i, tc := range testCases { - t.Run(strconv.Itoa(i), func(t *testing.T) { + t.Run(fmt.Sprintf("%d_%s", i, tc.input), func(t *testing.T) { have := sanitizeExpressionsInScript(tc.input) if tc.want != have { t.Fatalf("sanitized result is unexpected.\nwant: %q\nhave: %q", tc.want, have) @@ -73,3 +73,127 @@ func TestRuleShellcheckSanitizeExpressionsInScript(t *testing.T) { }) } } + +// Regression for #409 +func TestRuleShellcheckDetectShell(t *testing.T) { + tests := []struct { + what string + want string + workflow string // Shell name set at 'defaults' in Workflow node + job string // Shell name set at 'defaults' in Job node + step string // Shell name set at 'shell' in Step node + runner string // Runner name at 'runs-on' in Job node + }{ + { + what: "no default shell", + want: "bash", + }, + { + what: "workflow default", + want: "pwsh", + workflow: "pwsh", + }, + { + what: "job default", + want: "pwsh", + job: "pwsh", + }, + { + what: "step config", + want: "pwsh", + step: "pwsh", + }, + { + what: "job default is more proioritized than workflow", + want: "pwsh", + workflow: "bash", + job: "pwsh", + }, + { + what: "step config is more proioritized than job", + want: "pwsh", + workflow: "sh", + job: "bash", + step: "pwsh", + }, + { + what: "default shell detected from runner", + want: "pwsh", + runner: "windows-latest", + }, + { + what: "workflow default is more proioritized than runner", + want: "bash", + workflow: "bash", + runner: "windows-latest", + }, + { + what: "job default is more proioritized than runner", + want: "bash", + job: "bash", + runner: "windows-latest", + }, + { + what: "step config is more proioritized than runner", + want: "bash", + step: "bash", + runner: "windows-latest", + }, + { + what: "no shell is detected from Ubuntu runner", + want: "bash", + runner: "ubuntu-latest", + }, + { + what: "custom bash", + want: "bash -e {0}", + workflow: "bash -e {0}", + }, + { + what: "custom sh", + want: "sh -e {0}", + workflow: "sh -e {0}", + }, + } + + for _, tc := range tests { + t.Run(tc.what, func(t *testing.T) { + r := newRuleShellcheck(&externalCommand{}) + + w := &Workflow{} + if tc.workflow != "" { + w.Defaults = &Defaults{ + Run: &DefaultsRun{ + Shell: &String{Value: tc.workflow}, + }, + } + } + r.VisitWorkflowPre(w) + + j := &Job{} + if tc.job != "" { + j.Defaults = &Defaults{ + Run: &DefaultsRun{ + Shell: &String{Value: tc.job}, + }, + } + } + if tc.runner != "" { + j.RunsOn = &Runner{ + Labels: []*String{ + {Value: tc.runner}, + }, + } + } + r.VisitJobPre(j) + + e := &ExecRun{} + if tc.step != "" { + e.Shell = &String{Value: tc.step} + } + if s := r.getShellName(e); s != tc.want { + t.Fatalf("detected shell %q but wanted %q", s, tc.want) + } + }) + } +} diff --git a/testdata/err/shellcheck_default_shell_detection.out b/testdata/err/shellcheck_default_shell_detection.out new file mode 100644 index 000000000..304b7e613 --- /dev/null +++ b/testdata/err/shellcheck_default_shell_detection.out @@ -0,0 +1,12 @@ +/test\.yaml:15:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:15:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:20:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:20:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:30:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:30:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:42:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:42:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:47:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:47:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:53:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ +/test\.yaml:53:9: shellcheck reported issue in this script: .+ \[shellcheck\]/ diff --git a/testdata/err/shellcheck_default_shell_detection.yaml b/testdata/err/shellcheck_default_shell_detection.yaml new file mode 100644 index 000000000..0ffc40430 --- /dev/null +++ b/testdata/err/shellcheck_default_shell_detection.yaml @@ -0,0 +1,54 @@ +on: push + +defaults: + run: + shell: pwsh + +jobs: + job-level: + runs-on: ubuntu-latest + defaults: + run: + shell: bash + steps: + # ERROR: shellcheck checks this script + - run: $Env:FOO = "FOO" + step-level: + runs-on: ubuntu-latest + steps: + # ERROR: shellcheck checks this script + - run: $Env:FOO = "FOO" + shell: bash + step-level-2: + # Even if pwsh is detected from runner or job's defaults + runs-on: windows-latest + defaults: + run: + shell: pwsh + steps: + # ERROR: shellcheck checks this script + - run: $Env:FOO = "FOO" + # Step-level `shell` is prioritized + shell: bash + step-level-1: + # Even if pwsh is detected from runner + runs-on: windows-latest + # `defaults` is prioritized + defaults: + run: + shell: bash + steps: + # ERROR: shellcheck checks this script + - run: $Env:FOO = "FOO" + custom-bash: + runs-on: ubuntu-latest + steps: + # ERROR: shellcheck checks this script + - run: $Env:FOO = "FOO" + shell: 'bash -e {0}' + custom-sh: + runs-on: ubuntu-latest + steps: + # ERROR: shellcheck checks this script + - run: $Env:FOO = "FOO" + shell: 'sh -e {0}' diff --git a/testdata/ok/issue-409-shellcheck-default-shell.yaml b/testdata/ok/issue-409-shellcheck-default-shell.yaml new file mode 100644 index 000000000..b6b241703 --- /dev/null +++ b/testdata/ok/issue-409-shellcheck-default-shell.yaml @@ -0,0 +1,12 @@ +on: push + +defaults: + run: + shell: pwsh + +jobs: + test: + runs-on: ubuntu-latest + steps: + # This script should not be checked by shellcheck + - run: $Env:FOO = "FOO" diff --git a/testdata/ok/shellcheck_detect_shell_from_runner.yaml b/testdata/ok/shellcheck_detect_shell_from_runner.yaml new file mode 100644 index 000000000..5ec435c19 --- /dev/null +++ b/testdata/ok/shellcheck_detect_shell_from_runner.yaml @@ -0,0 +1,7 @@ +on: push +jobs: + test: + runs-on: windows-latest + steps: + # This script should not be checked by shellcheck + - run: $Env:FOO = "FOO" diff --git a/testdata/ok/shellcheck_explicit_shell_name.yaml b/testdata/ok/shellcheck_explicit_shell_name.yaml deleted file mode 100644 index b04b91c08..000000000 --- a/testdata/ok/shellcheck_explicit_shell_name.yaml +++ /dev/null @@ -1,13 +0,0 @@ -on: push -jobs: - test: - strategy: - matrix: - os: [ubuntu-latest, windows-latest] - runs-on: ${{ matrix.os }} - steps: - - name: Show file content - # This causes SC1001 if `shell: pwsh` is not set explicitly - run: cat xxx\yyy.txt - if: ${{ matrix.os == 'windows-latest' }} - shell: pwsh diff --git a/testdata/ok/shellcheck_job_default_shell.yaml b/testdata/ok/shellcheck_job_default_shell.yaml new file mode 100644 index 000000000..8f6d12ac2 --- /dev/null +++ b/testdata/ok/shellcheck_job_default_shell.yaml @@ -0,0 +1,13 @@ +on: push +defaults: + run: + shell: bash +jobs: + test: + defaults: + run: + shell: pwsh + runs-on: ubuntu-latest + steps: + # This script should not be checked by shellcheck + - run: $Env:FOO = "FOO" diff --git a/testdata/ok/shellcheck_step_shell.yaml b/testdata/ok/shellcheck_step_shell.yaml new file mode 100644 index 000000000..5edd4644f --- /dev/null +++ b/testdata/ok/shellcheck_step_shell.yaml @@ -0,0 +1,16 @@ +on: push +jobs: + test: + strategy: + matrix: + os: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.os }} + steps: + # This causes SC1001 if `shell:` is not set explicitly + - run: cat xxx\yyy.txt + if: ${{ matrix.os == 'windows-latest' }} + shell: pwsh + # This causes SC1001 if `shell:` is not set explicitly + - run: cat xxx\yyy.txt + if: ${{ matrix.os == 'windows-latest' }} + shell: 'python {0}' diff --git a/testdata/ok/shellcheck_workflow_default_shell.yaml b/testdata/ok/shellcheck_workflow_default_shell.yaml new file mode 100644 index 000000000..36dedbd17 --- /dev/null +++ b/testdata/ok/shellcheck_workflow_default_shell.yaml @@ -0,0 +1,15 @@ +on: push +defaults: + run: + shell: pwsh +jobs: + test1: + runs-on: ubuntu-latest + steps: + # This script should not be checked by shellcheck + - run: $Env:FOO = "FOO" + test2: + runs-on: ubuntu-latest + steps: + # This script should not be checked by shellcheck + - run: $Env:FOO = "FOO"