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

@@ -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
 	}
 }
 

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::")

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

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)

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 {

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::")

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

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

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