From be4a1477a5b73b3e17f79891ec6147413acb4df7 Mon Sep 17 00:00:00 2001
From: Josh Soref <2119212+jsoref@users.noreply.github.com>
Date: Thu, 16 Feb 2023 11:41:59 -0500
Subject: [PATCH] fix: tolerate workflow that needs a missing job (#1595)
(#1619)
Change planner functions to return errors
This enables createStages to return `unable to build dependency graph`
Fix PlanEvent to properly report errors relating to events/workflows
---
cmd/root.go | 43 ++++++--
pkg/artifacts/server_test.go | 15 ++-
pkg/model/planner.go | 71 +++++++++---
pkg/model/workflow_test.go | 3 +-
pkg/runner/reusable_workflow.go | 5 +-
pkg/runner/runner_test.go | 115 ++++++++++++++++++--
pkg/runner/testdata/issue-1595/missing.yml | 16 +++
pkg/runner/testdata/issue-1595/no-event.yml | 8 ++
pkg/runner/testdata/issue-1595/no-first.yml | 10 ++
9 files changed, 238 insertions(+), 48 deletions(-)
create mode 100644 pkg/runner/testdata/issue-1595/missing.yml
create mode 100644 pkg/runner/testdata/issue-1595/no-event.yml
create mode 100644 pkg/runner/testdata/issue-1595/no-first.yml
diff --git a/cmd/root.go b/cmd/root.go
index 23f411c..3619c7b 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -354,7 +354,7 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str
var filterPlan *model.Plan
// Determine the event name to be filtered
- var filterEventName string = ""
+ var filterEventName string
if len(args) > 0 {
log.Debugf("Using first passed in arguments event for filtering: %s", args[0])
@@ -366,23 +366,35 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str
filterEventName = events[0]
}
+ var plannerErr error
if jobID != "" {
log.Debugf("Preparing plan with a job: %s", jobID)
- filterPlan = planner.PlanJob(jobID)
+ filterPlan, plannerErr = planner.PlanJob(jobID)
} else if filterEventName != "" {
log.Debugf("Preparing plan for a event: %s", filterEventName)
- filterPlan = planner.PlanEvent(filterEventName)
+ filterPlan, plannerErr = planner.PlanEvent(filterEventName)
} else {
log.Debugf("Preparing plan with all jobs")
- filterPlan = planner.PlanAll()
+ filterPlan, plannerErr = planner.PlanAll()
+ }
+ if filterPlan == nil && plannerErr != nil {
+ return plannerErr
}
if list {
- return printList(filterPlan)
+ err = printList(filterPlan)
+ if err != nil {
+ return err
+ }
+ return plannerErr
}
if graph {
- return drawGraph(filterPlan)
+ err = drawGraph(filterPlan)
+ if err != nil {
+ return err
+ }
+ return plannerErr
}
// plan with triggered jobs
@@ -410,10 +422,13 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str
// build the plan for this run
if jobID != "" {
log.Debugf("Planning job: %s", jobID)
- plan = planner.PlanJob(jobID)
+ plan, plannerErr = planner.PlanJob(jobID)
} else {
log.Debugf("Planning jobs for event: %s", eventName)
- plan = planner.PlanEvent(eventName)
+ plan, plannerErr = planner.PlanEvent(eventName)
+ }
+ if plan == nil && plannerErr != nil {
+ return plannerErr
}
// check to see if the main branch was defined
@@ -501,14 +516,22 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str
if watch, err := cmd.Flags().GetBool("watch"); err != nil {
return err
} else if watch {
- return watchAndRun(ctx, r.NewPlanExecutor(plan))
+ err = watchAndRun(ctx, r.NewPlanExecutor(plan))
+ if err != nil {
+ return err
+ }
+ return plannerErr
}
executor := r.NewPlanExecutor(plan).Finally(func(ctx context.Context) error {
cancel()
return nil
})
- return executor(ctx)
+ err = executor(ctx)
+ if err != nil {
+ return err
+ }
+ return plannerErr
}
}
diff --git a/pkg/artifacts/server_test.go b/pkg/artifacts/server_test.go
index 259c954..943820c 100644
--- a/pkg/artifacts/server_test.go
+++ b/pkg/artifacts/server_test.go
@@ -297,13 +297,16 @@ func runTestJobFile(ctx context.Context, t *testing.T, tjfi TestJobFileInfo) {
planner, err := model.NewWorkflowPlanner(fullWorkflowPath, true)
assert.Nil(t, err, fullWorkflowPath)
- plan := planner.PlanEvent(tjfi.eventName)
-
- err = runner.NewPlanExecutor(plan)(ctx)
- if tjfi.errorMessage == "" {
- assert.Nil(t, err, fullWorkflowPath)
+ plan, err := planner.PlanEvent(tjfi.eventName)
+ if err == nil {
+ err = runner.NewPlanExecutor(plan)(ctx)
+ if tjfi.errorMessage == "" {
+ assert.Nil(t, err, fullWorkflowPath)
+ } else {
+ assert.Error(t, err, tjfi.errorMessage)
+ }
} else {
- assert.Error(t, err, tjfi.errorMessage)
+ assert.Nil(t, plan)
}
fmt.Println("::endgroup::")
diff --git a/pkg/model/planner.go b/pkg/model/planner.go
index 73e3488..1769b73 100644
--- a/pkg/model/planner.go
+++ b/pkg/model/planner.go
@@ -15,9 +15,9 @@ import (
// WorkflowPlanner contains methods for creating plans
type WorkflowPlanner interface {
- PlanEvent(eventName string) *Plan
- PlanJob(jobName string) *Plan
- PlanAll() *Plan
+ PlanEvent(eventName string) (*Plan, error)
+ PlanJob(jobName string) (*Plan, error)
+ PlanAll() (*Plan, error)
GetEvents() []string
}
@@ -169,47 +169,76 @@ type workflowPlanner struct {
}
// PlanEvent builds a new list of runs to execute in parallel for an event name
-func (wp *workflowPlanner) PlanEvent(eventName string) *Plan {
+func (wp *workflowPlanner) PlanEvent(eventName string) (*Plan, error) {
plan := new(Plan)
if len(wp.workflows) == 0 {
- log.Debugf("no events found for workflow: %s", eventName)
+ log.Debug("no workflows found by planner")
+ return plan, nil
}
+ var lastErr error
for _, w := range wp.workflows {
- for _, e := range w.On() {
+ events := w.On()
+ if len(events) == 0 {
+ log.Debugf("no events found for workflow: %s", w.File)
+ continue
+ }
+
+ for _, e := range events {
if e == eventName {
- plan.mergeStages(createStages(w, w.GetJobIDs()...))
+ stages, err := createStages(w, w.GetJobIDs()...)
+ if err != nil {
+ log.Warn(err)
+ lastErr = err
+ } else {
+ plan.mergeStages(stages)
+ }
}
}
}
- return plan
+ return plan, lastErr
}
// PlanJob builds a new run to execute in parallel for a job name
-func (wp *workflowPlanner) PlanJob(jobName string) *Plan {
+func (wp *workflowPlanner) PlanJob(jobName string) (*Plan, error) {
plan := new(Plan)
if len(wp.workflows) == 0 {
log.Debugf("no jobs found for workflow: %s", jobName)
}
+ var lastErr error
for _, w := range wp.workflows {
- plan.mergeStages(createStages(w, jobName))
+ stages, err := createStages(w, jobName)
+ if err != nil {
+ log.Warn(err)
+ lastErr = err
+ } else {
+ plan.mergeStages(stages)
+ }
}
- return plan
+ return plan, lastErr
}
// PlanAll builds a new run to execute in parallel all
-func (wp *workflowPlanner) PlanAll() *Plan {
+func (wp *workflowPlanner) PlanAll() (*Plan, error) {
plan := new(Plan)
if len(wp.workflows) == 0 {
- log.Debugf("no jobs found for loaded workflows")
+ log.Debug("no workflows found by planner")
+ return plan, nil
}
+ var lastErr error
for _, w := range wp.workflows {
- plan.mergeStages(createStages(w, w.GetJobIDs()...))
+ stages, err := createStages(w, w.GetJobIDs()...)
+ if err != nil {
+ log.Warn(err)
+ lastErr = err
+ } else {
+ plan.mergeStages(stages)
+ }
}
- return plan
+ return plan, lastErr
}
// GetEvents gets all the events in the workflows file
@@ -282,7 +311,7 @@ func (p *Plan) mergeStages(stages []*Stage) {
p.Stages = newStages
}
-func createStages(w *Workflow, jobIDs ...string) []*Stage {
+func createStages(w *Workflow, jobIDs ...string) ([]*Stage, error) {
// first, build a list of all the necessary jobs to run, and their dependencies
jobDependencies := make(map[string][]string)
for len(jobIDs) > 0 {
@@ -299,6 +328,8 @@ func createStages(w *Workflow, jobIDs ...string) []*Stage {
jobIDs = newJobIDs
}
+ var err error
+
// next, build an execution graph
stages := make([]*Stage, 0)
for len(jobDependencies) > 0 {
@@ -314,12 +345,16 @@ func createStages(w *Workflow, jobIDs ...string) []*Stage {
}
}
if len(stage.Runs) == 0 {
- log.Fatalf("Unable to build dependency graph!")
+ return nil, fmt.Errorf("unable to build dependency graph for %s (%s)", w.Name, w.File)
}
stages = append(stages, stage)
}
- return stages
+ if len(stages) == 0 && err != nil {
+ return nil, err
+ }
+
+ return stages, nil
}
// return true iff all strings in srcList exist in at least one of the stages
diff --git a/pkg/model/workflow_test.go b/pkg/model/workflow_test.go
index d978f16..a789233 100644
--- a/pkg/model/workflow_test.go
+++ b/pkg/model/workflow_test.go
@@ -241,7 +241,8 @@ func TestReadWorkflow_Strategy(t *testing.T) {
w, err := NewWorkflowPlanner("testdata/strategy/push.yml", true)
assert.NoError(t, err)
- p := w.PlanJob("strategy-only-max-parallel")
+ p, err := w.PlanJob("strategy-only-max-parallel")
+ assert.NoError(t, err)
assert.Equal(t, len(p.Stages), 1)
assert.Equal(t, len(p.Stages[0].Runs), 1)
diff --git a/pkg/runner/reusable_workflow.go b/pkg/runner/reusable_workflow.go
index b080b4d..a5687f9 100644
--- a/pkg/runner/reusable_workflow.go
+++ b/pkg/runner/reusable_workflow.go
@@ -74,7 +74,10 @@ func newReusableWorkflowExecutor(rc *RunContext, directory string, workflow stri
return err
}
- plan := planner.PlanEvent("workflow_call")
+ plan, err := planner.PlanEvent("workflow_call")
+ if err != nil {
+ return err
+ }
runner, err := NewReusableWorkflowRunner(rc)
if err != nil {
diff --git a/pkg/runner/runner_test.go b/pkg/runner/runner_test.go
index 0a83537..fb51be9 100644
--- a/pkg/runner/runner_test.go
+++ b/pkg/runner/runner_test.go
@@ -49,12 +49,99 @@ func init() {
secrets = map[string]string{}
}
+func TestNoWorkflowsFoundByPlanner(t *testing.T) {
+ planner, err := model.NewWorkflowPlanner("res", true)
+ assert.NoError(t, err)
+
+ out := log.StandardLogger().Out
+ var buf bytes.Buffer
+ log.SetOutput(&buf)
+ log.SetLevel(log.DebugLevel)
+ plan, err := planner.PlanEvent("pull_request")
+ assert.NotNil(t, plan)
+ assert.NoError(t, err)
+ assert.Contains(t, buf.String(), "no workflows found by planner")
+ buf.Reset()
+ plan, err = planner.PlanAll()
+ assert.NotNil(t, plan)
+ assert.NoError(t, err)
+ assert.Contains(t, buf.String(), "no workflows found by planner")
+ log.SetOutput(out)
+}
+
+func TestGraphMissingEvent(t *testing.T) {
+ planner, err := model.NewWorkflowPlanner("testdata/issue-1595/no-event.yml", true)
+ assert.NoError(t, err)
+
+ out := log.StandardLogger().Out
+ var buf bytes.Buffer
+ log.SetOutput(&buf)
+ log.SetLevel(log.DebugLevel)
+
+ plan, err := planner.PlanEvent("push")
+ assert.NoError(t, err)
+ assert.NotNil(t, plan)
+ assert.Equal(t, 0, len(plan.Stages))
+
+ assert.Contains(t, buf.String(), "no events found for workflow: no-event.yml")
+ log.SetOutput(out)
+}
+
+func TestGraphMissingFirst(t *testing.T) {
+ planner, err := model.NewWorkflowPlanner("testdata/issue-1595/no-first.yml", true)
+ assert.NoError(t, err)
+
+ plan, err := planner.PlanEvent("push")
+ assert.EqualError(t, err, "unable to build dependency graph for no first (no-first.yml)")
+ assert.NotNil(t, plan)
+ assert.Equal(t, 0, len(plan.Stages))
+}
+
+func TestGraphWithMissing(t *testing.T) {
+ planner, err := model.NewWorkflowPlanner("testdata/issue-1595/missing.yml", true)
+ assert.NoError(t, err)
+
+ out := log.StandardLogger().Out
+ var buf bytes.Buffer
+ log.SetOutput(&buf)
+ log.SetLevel(log.DebugLevel)
+
+ plan, err := planner.PlanEvent("push")
+ assert.NotNil(t, plan)
+ assert.Equal(t, 0, len(plan.Stages))
+ assert.EqualError(t, err, "unable to build dependency graph for missing (missing.yml)")
+ assert.Contains(t, buf.String(), "unable to build dependency graph for missing (missing.yml)")
+ log.SetOutput(out)
+}
+
+func TestGraphWithSomeMissing(t *testing.T) {
+ log.SetLevel(log.DebugLevel)
+
+ planner, err := model.NewWorkflowPlanner("testdata/issue-1595/", true)
+ assert.NoError(t, err)
+
+ out := log.StandardLogger().Out
+ var buf bytes.Buffer
+ log.SetOutput(&buf)
+ log.SetLevel(log.DebugLevel)
+
+ plan, err := planner.PlanAll()
+ assert.Error(t, err, "unable to build dependency graph for no first (no-first.yml)")
+ assert.NotNil(t, plan)
+ assert.Equal(t, 1, len(plan.Stages))
+ assert.Contains(t, buf.String(), "unable to build dependency graph for missing (missing.yml)")
+ assert.Contains(t, buf.String(), "unable to build dependency graph for no first (no-first.yml)")
+ log.SetOutput(out)
+}
+
func TestGraphEvent(t *testing.T) {
planner, err := model.NewWorkflowPlanner("testdata/basic", true)
- assert.Nil(t, err)
+ assert.NoError(t, err)
- plan := planner.PlanEvent("push")
- assert.Nil(t, err)
+ plan, err := planner.PlanEvent("push")
+ assert.NoError(t, err)
+ assert.NotNil(t, plan)
+ assert.NotNil(t, plan.Stages)
assert.Equal(t, len(plan.Stages), 3, "stages")
assert.Equal(t, len(plan.Stages[0].Runs), 1, "stage0.runs")
assert.Equal(t, len(plan.Stages[1].Runs), 1, "stage1.runs")
@@ -63,8 +150,10 @@ func TestGraphEvent(t *testing.T) {
assert.Equal(t, plan.Stages[1].Runs[0].JobID, "build", "jobid")
assert.Equal(t, plan.Stages[2].Runs[0].JobID, "test", "jobid")
- plan = planner.PlanEvent("release")
- assert.Equal(t, len(plan.Stages), 0, "stages")
+ plan, err = planner.PlanEvent("release")
+ assert.NoError(t, err)
+ assert.NotNil(t, plan)
+ assert.Equal(t, 0, len(plan.Stages))
}
type TestJobFileInfo struct {
@@ -105,13 +194,15 @@ func (j *TestJobFileInfo) runTest(ctx context.Context, t *testing.T, cfg *Config
planner, err := model.NewWorkflowPlanner(fullWorkflowPath, true)
assert.Nil(t, err, fullWorkflowPath)
- plan := planner.PlanEvent(j.eventName)
-
- err = runner.NewPlanExecutor(plan)(ctx)
- if j.errorMessage == "" {
- assert.Nil(t, err, fullWorkflowPath)
- } else {
- assert.Error(t, err, j.errorMessage)
+ plan, err := planner.PlanEvent(j.eventName)
+ assert.True(t, (err == nil) != (plan == nil), "PlanEvent should return either a plan or an error")
+ if err == nil && plan != nil {
+ err = runner.NewPlanExecutor(plan)(ctx)
+ if j.errorMessage == "" {
+ assert.Nil(t, err, fullWorkflowPath)
+ } else {
+ assert.Error(t, err, j.errorMessage)
+ }
}
fmt.Println("::endgroup::")
diff --git a/pkg/runner/testdata/issue-1595/missing.yml b/pkg/runner/testdata/issue-1595/missing.yml
new file mode 100644
index 0000000..3b4adf4
--- /dev/null
+++ b/pkg/runner/testdata/issue-1595/missing.yml
@@ -0,0 +1,16 @@
+name: missing
+on: push
+
+jobs:
+ second:
+ runs-on: ubuntu-latest
+ needs: first
+ steps:
+ - run: echo How did you get here?
+ shell: bash
+
+ standalone:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo Hello world
+ shell: bash
diff --git a/pkg/runner/testdata/issue-1595/no-event.yml b/pkg/runner/testdata/issue-1595/no-event.yml
new file mode 100644
index 0000000..2140a0b
--- /dev/null
+++ b/pkg/runner/testdata/issue-1595/no-event.yml
@@ -0,0 +1,8 @@
+name: no event
+
+jobs:
+ stuck:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo How did you get here?
+ shell: bash
diff --git a/pkg/runner/testdata/issue-1595/no-first.yml b/pkg/runner/testdata/issue-1595/no-first.yml
new file mode 100644
index 0000000..48d4b55
--- /dev/null
+++ b/pkg/runner/testdata/issue-1595/no-first.yml
@@ -0,0 +1,10 @@
+name: no first
+on: push
+
+jobs:
+ second:
+ runs-on: ubuntu-latest
+ needs: first
+ steps:
+ - run: echo How did you get here?
+ shell: bash