From 6461b5e1504fc8fbb8f1f812728f978079305e50 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 +++++-- parse.go | 3 +- rule_workflow_run.go | 112 ++++++++++++++++++++++++++++++++++++++ rule_workflow_run_test.go | 73 +++++++++++++++++++++++++ 6 files changed, 243 insertions(+), 5 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..cec8a7657 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. + 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..81e27048f 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. @@ -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) @@ -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) } diff --git a/parse.go b/parse.go index fb5e43e64..1f792750c 100644 --- a/parse.go +++ b/parse.go @@ -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 { @@ -1428,6 +1428,7 @@ func Parse(b []byte) (*Workflow, []*Error) { p := &parser{} w := p.parse(&n) + w.Path = path return w, p.errors } 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..e31f576d9 --- /dev/null +++ b/rule_workflow_run_test.go @@ -0,0 +1,73 @@ +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)) + } + if !reflect.DeepEqual(tc.expectErrs, errs) { + t.Fatalf("Errors does not match:\nWanted: %v\nBut have: %v", tc.expectErrs, errs) + } + }) + } +}