diff --git a/cmd/root.go b/cmd/root.go index 1f72344df66..a9dc89100a7 100644 --- a/cmd/root.go +++ b/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 diff --git a/pkg/artifacts/server_test.go b/pkg/artifacts/server_test.go index 259c9542c4e..943820ca6a9 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 73e3488a564..0186d1d5b0b 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,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 diff --git a/pkg/model/workflow_test.go b/pkg/model/workflow_test.go index d978f163c01..a789233857f 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 b080b4dbb1d..a5687f9396e 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 0a83537ebc0..8b13cf9264f 100644 --- a/pkg/runner/runner_test.go +++ b/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::") diff --git a/pkg/runner/testdata/issue-1595/missing.yml b/pkg/runner/testdata/issue-1595/missing.yml new file mode 100644 index 00000000000..3b4adf48d01 --- /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 00000000000..2140a0bd41c --- /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 00000000000..48d4b55d850 --- /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