From 872fb58109e4caa504df1a446007a58c62922e39 Mon Sep 17 00:00:00 2001 From: Guillermo Mazzola Date: Sun, 18 Feb 2024 18:53:22 +0100 Subject: [PATCH] Added `workflow_run.workflows` references check --- ast.go | 2 + docs/checks.md | 39 +++++++++++++ linter.go | 19 ++++++- rule_workflow_run.go | 112 ++++++++++++++++++++++++++++++++++++++ rule_workflow_run_test.go | 76 ++++++++++++++++++++++++++ 5 files changed, 245 insertions(+), 3 deletions(-) create mode 100644 rule_workflow_run.go create mode 100644 rule_workflow_run_test.go diff --git a/ast.go b/ast.go index 29be0db7b..5e056769d 100644 --- a/ast.go +++ b/ast.go @@ -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. This field is internal and not defined by GitHub workflow's syntax + 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 diff --git a/docs/checks.md b/docs/checks.md index daa7a9a68..0b0495353 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -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) @@ -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 `./`). + +## 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 + ## ID naming convention diff --git a/linter.go b/linter.go index b438f4445..b4d09a84a 100644 --- a/linter.go +++ b/linter.go @@ -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 @@ -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) } @@ -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) @@ -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 @@ -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 @@ -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. @@ -516,6 +524,8 @@ func (l *Linter) check( } if w != nil { + w.Path = path + dbg := l.debugWriter() rules := []Rule{ @@ -555,6 +565,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) } diff --git a/rule_workflow_run.go b/rule_workflow_run.go new file mode 100644 index 000000000..e30aceacc --- /dev/null +++ b/rule_workflow_run.go @@ -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 +} diff --git a/rule_workflow_run_test.go b/rule_workflow_run_test.go new file mode 100644 index 000000000..aa8930eca --- /dev/null +++ b/rule_workflow_run_test.go @@ -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) + } + } + }) + } +}