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

@@ -368,13 +368,16 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str
 
 		if jobID != "" {
 			log.Debugf("Preparing plan with a job: %s", jobID)
-			filterPlan = planner.PlanJob(jobID)
+			filterPlan, err = planner.PlanJob(jobID)
 		} else if filterEventName != "" {
 			log.Debugf("Preparing plan for a event: %s", filterEventName)
-			filterPlan = planner.PlanEvent(filterEventName)
+			filterPlan, err = planner.PlanEvent(filterEventName)
 		} else {
 			log.Debugf("Preparing plan with all jobs")
-			filterPlan = planner.PlanAll()
+			filterPlan, err = planner.PlanAll()
+		}
+		if err != nil {
+			return err
 		}
 
 		if list {
@@ -410,10 +413,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, err = planner.PlanJob(jobID)
 		} else {
 			log.Debugf("Planning jobs for event: %s", eventName)
-			plan = planner.PlanEvent(eventName)
+			plan, err = planner.PlanEvent(eventName)
+		}
+		if err != nil {
+			return err
 		}
 
 		// check to see if the main branch was defined

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,65 @@ 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.Debugf("no workflows found for workflow: %s", eventName)
 	}
 
 	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 {
+					plan.mergeStages(stages)
+				}
 			}
 		}
 	}
-	return plan
+	if len(plan.Stages) == 0 {
+		return nil, fmt.Errorf("No reachable stages for %d workflow(s)", len(wp.workflows))
+	}
+	return plan, nil
 }
 
 // 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)
 	}
 
 	for _, w := range wp.workflows {
-		plan.mergeStages(createStages(w, jobName))
+		stages, err := createStages(w, jobName)
+		if err == nil {
+			plan.mergeStages(stages)
+		}
 	}
-	return plan
+	return plan, nil
 }
 
 // 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")
 	}
 
 	for _, w := range wp.workflows {
-		plan.mergeStages(createStages(w, w.GetJobIDs()...))
+		stages, err := createStages(w, w.GetJobIDs()...)
+		if err == nil {
+			plan.mergeStages(stages)
+		}
 	}
 
-	return plan
+	return plan, nil
 }
 
 // GetEvents gets all the events in the workflows file
@@ -282,7 +300,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,10 +317,13 @@ 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 {
 		stage := new(Stage)
+		jobDependenciesCount := len(jobDependencies)
 		for jID, jDeps := range jobDependencies {
 			// make sure all deps are in the graph already
 			if listInStages(jDeps, stages...) {
@@ -314,12 +335,21 @@ func createStages(w *Workflow, jobIDs ...string) []*Stage {
 			}
 		}
 		if len(stage.Runs) == 0 {
-			log.Fatalf("Unable to build dependency graph!")
+			err = fmt.Errorf("unable to build dependency graph for %s (%s)", w.Name, w.File)
+			log.Warn(err)
+			if jobDependenciesCount == len(jobDependencies) {
+				break
+			}
+		} else {
+			stages = append(stages, stage)
 		}
-		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,54 @@ func init() {
 	secrets = map[string]string{}
 }
 
+func TestGraphMissingEvent(t *testing.T) {
+	planner, err := model.NewWorkflowPlanner("testdata/issue-1595/no-event.yml", true)
+	assert.NoError(t, err)
+
+	plan, err := planner.PlanEvent("push")
+	assert.Equal(t, err.Error(), "No reachable stages for 1 workflow(s)")
+	assert.Nil(t, plan)
+}
+
+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.Equal(t, err.Error(), "No reachable stages for 1 workflow(s)")
+	assert.Nil(t, plan)
+}
+
+func TestGraphWithMissing(t *testing.T) {
+	planner, err := model.NewWorkflowPlanner("testdata/issue-1595/missing.yml", true)
+	assert.NoError(t, err)
+
+	plan, err := planner.PlanEvent("push")
+	assert.NoError(t, err)
+	assert.NotNil(t, plan)
+	assert.Equal(t, 1, len(plan.Stages))
+}
+
+func TestGraphWithSomeMissing(t *testing.T) {
+	log.SetLevel(log.DebugLevel)
+
+	planner, err := model.NewWorkflowPlanner("testdata/issue-1595/", true)
+	assert.NoError(t, err)
+
+	plan, err := planner.PlanAll()
+	assert.NoError(t, err)
+	assert.NotNil(t, plan)
+	assert.Equal(t, 1, len(plan.Stages))
+}
+
 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 +105,9 @@ 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.Error(t, err)
+	assert.Nil(t, plan)
 }
 
 type TestJobFileInfo struct {
@@ -105,13 +148,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