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