fix: tolerate workflow that needs a missing job (#1595) #1619
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1619 +/- ##
==========================================
+ Coverage 61.22% 62.03% +0.81%
==========================================
Files 46 46
Lines 7141 7239 +98
==========================================
+ Hits 4372 4491 +119
+ Misses 2462 2440 -22
- Partials 307 308 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
With this, I'm able to run output |
|
With this change, you are allowing to run https://github.com/jsoref/necktos-act-issue-1595/actions/runs/4126583173/workflow. |
KnisterPeter
requested changes
Feb 13, 2023
There was a problem hiding this comment.
I'm fine with this now, but @ChristopherHX has a valid point.
If that's fixed I'll approve
jsoref
force-pushed
the
issue-1595
branch
2 times, most recently
from
ebbfc42 to
38fc5f0
Compare
|
@jsoref this pull request has failed checks 🛠 |
|
Seems like you fixed the exit code for the exact case I wrote above, but depending on the order of the workflows the exit code is still 0. I mean it is ok for me to run all valid workflows, but once one is an invalid workflow I expect exit code 1. So the first case
|
|
|
You are right about that flag..., silently dropping previous flags is unexpected behavior for me. Other cli parsers I know are throwing.
Hmm, can't we return both err and the partial result? Then just return the planner error if execution of the workflow succeeded. |
|
Seems to work for me, that is what I expect. This patch make this PR what I expect, however other maintainer and the owner can outvote mediff --git a/cmd/root.go b/cmd/root.go
index a9dc891..3619c7b 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -366,26 +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, err = planner.PlanJob(jobID)
+ filterPlan, plannerErr = planner.PlanJob(jobID)
} else if filterEventName != "" {
log.Debugf("Preparing plan for a event: %s", filterEventName)
- filterPlan, err = planner.PlanEvent(filterEventName)
+ filterPlan, plannerErr = planner.PlanEvent(filterEventName)
} else {
log.Debugf("Preparing plan with all jobs")
- filterPlan, err = planner.PlanAll()
+ filterPlan, plannerErr = planner.PlanAll()
}
- if err != nil {
- return err
+ 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
@@ -413,13 +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, err = planner.PlanJob(jobID)
+ plan, plannerErr = planner.PlanJob(jobID)
} else {
log.Debugf("Planning jobs for event: %s", eventName)
- plan, err = planner.PlanEvent(eventName)
+ plan, plannerErr = planner.PlanEvent(eventName)
}
- if err != nil {
- return err
+ if plan == nil && plannerErr != nil {
+ return plannerErr
}
// check to see if the main branch was defined
@@ -507,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/model/planner.go b/pkg/model/planner.go
index c7f4eb0..40bc67c 100644
--- a/pkg/model/planner.go
+++ b/pkg/model/planner.go
@@ -172,14 +172,15 @@ type workflowPlanner struct {
func (wp *workflowPlanner) PlanEvent(eventName string) (*Plan, error) {
plan := new(Plan)
if len(wp.workflows) == 0 {
- log.Debug("no workflows found by planner")
+ log.Debugf("no workflows found by planner")
return plan, nil
}
+ var lastErr error
for _, w := range wp.workflows {
events := w.On()
if len(events) == 0 {
- log.Debugf("no events found for workflow: %s", w.File)
+ log.Warn("no events found for workflow: %s", w.File)
continue
}
@@ -188,16 +189,14 @@ func (wp *workflowPlanner) PlanEvent(eventName string) (*Plan, error) {
stages, err := createStages(w, w.GetJobIDs()...)
if err != nil {
log.Warn(err)
+ lastErr = err
} else {
plan.mergeStages(stages)
}
}
}
}
- if len(plan.Stages) == 0 {
- return nil, fmt.Errorf("no reachable stages for %s event for %d workflow(s)", eventName, len(wp.workflows))
- }
- return plan, nil
+ return plan, lastErr
}
// PlanJob builds a new run to execute in parallel for a job name
|
|
I'm happy to apply most of that. This seems unnecessary: |
|
Feel free to drop this, unneeded change. I will approve once the behavior similar to my patch. You can also drop this part. - log.Debugf("no events found for workflow: %s", w.File)
+ log.Warn("no events found for workflow: %s", w.File) |
|
(I'm just trying to convince my computer to run some tests in VSCode before I amend.) |
ChristopherHX
previously approved these changes
Feb 14, 2023
There was a problem hiding this comment.
All in all it's fine from my side.
PlanAll and PlanJob should also return err for the planner
diff --git a/pkg/model/planner.go b/pkg/model/planner.go
index fc98684..1769b73 100644
--- a/pkg/model/planner.go
+++ b/pkg/model/planner.go
@@ -205,16 +205,18 @@ func (wp *workflowPlanner) PlanJob(jobName string) (*Plan, error) {
if len(wp.workflows) == 0 {
log.Debugf("no jobs found for workflow: %s", jobName)
}
+ var lastErr error
for _, w := range wp.workflows {
stages, err := createStages(w, jobName)
if err != nil {
log.Warn(err)
+ lastErr = err
} else {
plan.mergeStages(stages)
}
}
- return plan, nil
+ return plan, lastErr
}
// PlanAll builds a new run to execute in parallel all
@@ -224,17 +226,19 @@ func (wp *workflowPlanner) PlanAll() (*Plan, error) {
log.Debug("no workflows found by planner")
return plan, nil
}
+ var lastErr error
for _, w := range wp.workflows {
stages, err := createStages(w, w.GetJobIDs()...)
if err != nil {
log.Warn(err)
+ lastErr = err
} else {
plan.mergeStages(stages)
}
}
- return plan, nil
+ return plan, lastErr
}
// GetEvents gets all the events in the workflows file
|
@jsoref this pull request has failed checks 🛠 |
This enables createStages to return `unable to build dependency graph` Fix PlanEvent to properly report errors relating to events/workflows
KnisterPeter
approved these changes
Feb 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Try to fix #1595...
Commit Message
{{title}} (#1619)
Change planner functions to return errors
This enables createStages to return
unable to build dependency graphFix PlanEvent to properly report errors relating to events/workflows