Skip to content

Commit

Permalink
Added workflow_run.workflows references check
Browse files Browse the repository at this point in the history
  • Loading branch information
gmazzo committed Jul 1, 2024
1 parent 0a2b221 commit 0f1ab87
Show file tree
Hide file tree
Showing 6 changed files with 246 additions and 5 deletions.
2 changes: 2 additions & 0 deletions ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,8 @@ type Job struct {
// Workflow is root of workflow syntax tree, which represents one workflow configuration file.
// https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions
type Workflow struct {
// Path is the path of the workflow.
Path string
// Name is name of the workflow. This field can be nil when user didn't specify the name explicitly.
// https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#name
Name *String
Expand Down
39 changes: 39 additions & 0 deletions docs/checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ List of checks:
- [Environment variable names](#check-env-var-names)
- [Permissions](#permissions)
- [Reusable workflows](#check-reusable-workflows)
- [Workflow references at `workflow_run`](#check-workflow-run-refs)
- [ID naming convention](#id-naming-convention)
- [Contexts and special functions availability](#ctx-spfunc-availability)
- [Deprecated workflow commands](#check-deprecated-workflow-commands)
Expand Down Expand Up @@ -2497,6 +2498,44 @@ as `{version: string}`. In the downstream job, actionlint can report an error at

Note that this check only works with local reusable workflow (starting with `./`).

<a name="check-workflow-run-refss"></a>
## Workflows references at `workflow_run`
[`workflow_run` is a trigger event](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run) to run a workflow when another workflow is completed.

actionlint checks that all workflows referenced by name at `workflow_run.workflows` exists.

Example input:

```yaml
# file: .github/workflows/build.yml
on: push
```
```yaml
# file: .github/workflows/test.yml
name: Tests
on: push
```
```yaml
# file: .github/workflows/notify-errors.yml
on:
workflow_run:
types: [completed]
workflows:
- build
- test # ERROR: This workflow does not exist, its name is `Tests`
```
Output:

```
:6:9: Workflow "test" is not defined [workflow_run]
|
6 | - test # ERROR: This workflow does not exist, its name is `Tests`
| ^~~~
```

> [!NOTE]
> This check only applies when all workflow files are analyzed together
<a name="id-naming-convention"></a>
## ID naming convention

Expand Down
19 changes: 15 additions & 4 deletions linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
dbg := l.debugWriter()
acf := NewLocalActionsCacheFactory(dbg)
rwcf := NewLocalReusableWorkflowCacheFactory(cwd, dbg)
rwrst := NewRuleWorkflowRunSharedState()

type workspace struct {
path string
Expand Down Expand Up @@ -350,7 +351,7 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
w.path = r // Use relative path if possible
}
}
errs, err := l.check(w.path, src, proj, proc, ac, rwc)
errs, err := l.check(w.path, src, proj, proc, ac, rwc, rwrst)
if err != nil {
return fmt.Errorf("fatal error while checking %s: %w", w.path, err)
}
Expand All @@ -372,6 +373,12 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
// called safely.
proc.wait()

rwErrs := rwrst.ComputeMissingReferences()
for i := range ws {
w := &ws[i]
w.errs = append(w.errs, rwErrs[w.path]...)
}

total := 0
for i := range ws {
total += len(ws[i].errs)
Expand Down Expand Up @@ -429,7 +436,7 @@ func (l *Linter) LintFile(path string, project *Project) ([]*Error, error) {
dbg := l.debugWriter()
localActions := NewLocalActionsCache(project, dbg)
localReusableWorkflows := NewLocalReusableWorkflowCache(project, l.cwd, dbg)
errs, err := l.check(path, src, project, proc, localActions, localReusableWorkflows)
errs, err := l.check(path, src, project, proc, localActions, localReusableWorkflows, nil)
proc.wait()
if err != nil {
return nil, err
Expand Down Expand Up @@ -461,7 +468,7 @@ func (l *Linter) Lint(path string, content []byte, project *Project) ([]*Error,
dbg := l.debugWriter()
localActions := NewLocalActionsCache(project, dbg)
localReusableWorkflows := NewLocalReusableWorkflowCache(project, l.cwd, dbg)
errs, err := l.check(path, content, project, proc, localActions, localReusableWorkflows)
errs, err := l.check(path, content, project, proc, localActions, localReusableWorkflows, nil)
proc.wait()
if err != nil {
return nil, err
Expand All @@ -481,6 +488,7 @@ func (l *Linter) check(
proc *concurrentProcess,
localActions *LocalActionsCache,
localReusableWorkflows *LocalReusableWorkflowCache,
ruleWorkflowRunState *RuleWorkflowRunSharedState,
) ([]*Error, error) {
// Note: This method is called to check multiple files in parallel.
// It must be thread safe assuming fields of Linter are not modified while running.
Expand Down Expand Up @@ -508,7 +516,7 @@ func (l *Linter) check(
l.debug("No config was found")
}

w, all := Parse(content)
w, all := Parse(path, content)

if l.logLevel >= LogLevelVerbose {
elapsed := time.Since(start)
Expand Down Expand Up @@ -555,6 +563,9 @@ func (l *Linter) check(
} else {
l.log("Rule \"pyflakes\" was disabled since pyflakes command name was empty")
}
if ruleWorkflowRunState != nil {
rules = append(rules, NewRuleWorkflowRun(ruleWorkflowRunState))
}
if l.onRulesCreated != nil {
rules = l.onRulesCreated(rules)
}
Expand Down
3 changes: 2 additions & 1 deletion parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -1416,7 +1416,7 @@ func handleYAMLError(err error) []*Error {
// Parse parses given source as byte sequence into workflow syntax tree. It returns all errors
// detected while parsing the input. It means that detecting one error does not stop parsing. Even
// if one or more errors are detected, parser will try to continue parsing and finding more errors.
func Parse(b []byte) (*Workflow, []*Error) {
func Parse(path string, b []byte) (*Workflow, []*Error) {
var n yaml.Node

if err := yaml.Unmarshal(b, &n); err != nil {
Expand All @@ -1428,6 +1428,7 @@ func Parse(b []byte) (*Workflow, []*Error) {

p := &parser{}
w := p.parse(&n)
w.Path = path

return w, p.errors
}
112 changes: 112 additions & 0 deletions rule_workflow_run.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package actionlint

import (
"path/filepath"
"strings"
"sync"
)

type RuleWorkflowRunSharedState struct {
refs map[string]*workflowRef
mutex sync.Mutex
}

type RuleWorkflowRun struct {
RuleBase
*RuleWorkflowRunSharedState
}

type workflowRef struct {
seen bool
refAt []*workflowRefBy
}

type workflowRefBy struct {
path string
*Pos
}

func NewRuleWorkflowRunSharedState() *RuleWorkflowRunSharedState {
return &RuleWorkflowRunSharedState{
refs: make(map[string]*workflowRef),
mutex: sync.Mutex{},
}
}

func NewRuleWorkflowRun(state *RuleWorkflowRunSharedState) *RuleWorkflowRun {
return &RuleWorkflowRun{
RuleBase: RuleBase{
name: "workflow_run",
desc: "Checks referenced workflows in `workflow_run` exists",
},
RuleWorkflowRunSharedState: state,
}
}

func (state *RuleWorkflowRunSharedState) resolve(workflow string) *workflowRef {
ref := state.refs[workflow]
if ref == nil {
ref = &workflowRef{refAt: make([]*workflowRefBy, 0)}
state.refs[workflow] = ref
}
return ref
}

func (w *Workflow) computeName() string {
if w.Name != nil {
return w.Name.Value
}
return strings.TrimSuffix(filepath.Base(w.Path), filepath.Ext(w.Path))
}

func (rule *RuleWorkflowRun) VisitWorkflowPre(node *Workflow) error {
rule.workflowSeen(node)

for _, event := range node.On {
switch e := event.(type) {
case *WebhookEvent:
for _, ref := range e.Workflows {
rule.workflowReferenced(node.Path, ref)
}
}
}
return nil
}

func (rule *RuleWorkflowRun) workflowSeen(node *Workflow) {
name := node.computeName()
rule.Debug("workflowSeen: %q\n", name)

rule.mutex.Lock()
defer rule.mutex.Unlock()

rule.resolve(name).seen = true
}

func (rule *RuleWorkflowRun) workflowReferenced(path string, refAt *String) {
rule.Debug("workflowReferenced: %q\n", refAt.Value)

rule.mutex.Lock()
defer rule.mutex.Unlock()

ref := rule.resolve(refAt.Value)
ref.refAt = append(ref.refAt, &workflowRefBy{path, refAt.Pos})
}

func (state *RuleWorkflowRunSharedState) ComputeMissingReferences() map[string][]*Error {
state.mutex.Lock()
defer state.mutex.Unlock()

errs := make(map[string][]*Error)
for workflow, refs := range state.refs {
if !refs.seen {
for _, refAt := range refs.refAt {
errs[refAt.path] = append(
errs[refAt.path],
errorfAt(refAt.Pos, "workflow_run", "Workflow %q is not defined", workflow),
)
}
}
}
return errs
}
76 changes: 76 additions & 0 deletions rule_workflow_run_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package actionlint

import (
"reflect"
"testing"
)

func TestRuleWorkflowRunCheck(t *testing.T) {
type expectedErr struct {
path string
workflows []*Workflow
}

pos := &Pos{1, 1}

expectErr := func(expected ...expectedErr) map[string][]*Error {
errs := make(map[string][]*Error)
for _, e := range expected {
for _, w := range e.workflows {
name := w.computeName()
errs[e.path] = append(errs[e.path], errorfAt(pos, "workflow_run", "Workflow %q is not defined", name))
}
}
return errs
}

w1 := &Workflow{Path: "repo/w1.yml"}
w2 := &Workflow{Path: "repo/w2.yml", Name: &String{Value: "A Workflow", Pos: pos}}
w3 := &Workflow{
Path: "repo/w3.yml",
Name: &String{Value: "anotherWorkflow", Pos: &Pos{1, 1}},
On: []Event{&WebhookEvent{
Hook: &String{Value: "workflow_run"},
Workflows: []*String{
{Value: "w1", Pos: pos},
{Value: "A Workflow", Pos: pos},
}}},
}

tests := []struct {
name string
workflows []*Workflow
expectErrs map[string][]*Error
}{
{"only w1", []*Workflow{w1}, expectErr()},
{"only w2", []*Workflow{w2}, expectErr()},
{"w1 and w2", []*Workflow{w1, w2}, expectErr()},
{"only w3", []*Workflow{w3}, expectErr(expectedErr{"repo/w3.yml", []*Workflow{w1, w2}})},
{"w1 and w3", []*Workflow{w1, w3}, expectErr(expectedErr{"repo/w3.yml", []*Workflow{w2}})},
{"w2 and w3", []*Workflow{w2, w3}, expectErr(expectedErr{"repo/w3.yml", []*Workflow{w1}})},
{"all", []*Workflow{w1, w2, w3}, expectErr()},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
r := NewRuleWorkflowRun(NewRuleWorkflowRunSharedState())
for _, w := range tc.workflows {
if err := r.VisitWorkflowPre(w); err != nil {
t.Fatal(err)
}
}

errs := r.ComputeMissingReferences()

if len(tc.expectErrs) != len(errs) {
t.Fatalf("Wanted %d errors but have %d", len(tc.expectErrs), len(errs))
}
for key, expected := range tc.expectErrs {
actual := errs[key]
if !reflect.DeepEqual(expected, actual) {
t.Fatalf("Errors does not match:\nWanted: %v\nBut have: %v", expected, actual)
}
}
})
}
}

0 comments on commit 0f1ab87

Please sign in to comment.