-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fail-fast flag to src batch #1049
base: main
Are you sure you want to change the base?
Conversation
Hey @gabtorre Apologies for the long delay. I'll review this soon. |
for _, task := range tasks { | ||
fmt.Println(task.Repository) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over debug statement
c := make(chan os.Signal, 1) | ||
signal.Notify(c, os.Interrupt) | ||
|
||
go func() { | ||
select { | ||
case <-c: | ||
ctxCancel() | ||
ctxCancel(errors.New("Ctrl-C hit")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. TIL I learned about the .WithCancelCause
method.
@@ -101,7 +101,7 @@ Examples: | |||
} | |||
|
|||
ctx, cancel := contextCancelOnInterrupt(context.Background()) | |||
defer cancel() | |||
defer cancel(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be passing in nil directly here, an error could occur at any point in the execution of a batch spec.
Is there a reason why the handler
function doesn't return a named return value and we can then pass that into defer cancel
?
handler := func(args []string) (err error) {
...
ctx, cancel := contextCancelOnInterrupt(context.Background())
defer cancel(err)
...
}
This way we are guaranteed to always have access to whatever error occurs in the lifecycle of an execution.
cctx := executor.CancelableContext{ | ||
Context: ctx, | ||
Cancel: failFastCancel, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for this struct? Is there a reason we can't pass in the context and failFastCancel
as separate arguments?
Or better still have the failFastCancel
as one of the fields in executeBatchSpecOpts
, that way we are certain ctx
is always going to be of type context.Context
and wouldn't be confusing to anyone in the future;
@@ -177,7 +182,7 @@ func (c *Coordinator) buildSpecs(ctx context.Context, batchSpec *batcheslib.Batc | |||
|
|||
// ExecuteAndBuildSpecs executes the given tasks and builds changeset specs for the results. | |||
// It calls the ui on updates. | |||
func (c *Coordinator) ExecuteAndBuildSpecs(ctx context.Context, batchSpec *batcheslib.BatchSpec, tasks []*Task, ui TaskExecutionUI) ([]*batcheslib.ChangesetSpec, []string, error) { | |||
func (c *Coordinator) ExecuteAndBuildSpecs(ctx CancelableContext, batchSpec *batcheslib.BatchSpec, tasks []*Task, ui TaskExecutionUI) ([]*batcheslib.ChangesetSpec, []string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the arguments to this method is getting bulky, we can move it into a struct and pass in the cancel function explicitly.
func (c *Coordinator) ExecuteAndBuildSpecs(ctx CancelableContext, batchSpec *batcheslib.BatchSpec, tasks []*Task, ui TaskExecutionUI) ([]*batcheslib.ChangesetSpec, []string, error) { | |
type ExecuteAndBuildSpecsArgs struct { | |
batchSpec *batcheslib.BatchSpec | |
tasks []*Task | |
ui TaskExecutionUI | |
cancelFunc context.CancelCauseFunc | |
} | |
func (c *Coordinator) ExecuteAndBuildSpecs(ctx context.Context, args ExecuteAndBuildSpecsArgs) ([]*batcheslib.ChangesetSpec, []string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancelFunc isn't being used here, I wonder if we need to pass it in here as an argument?
This adds a new
--fail-fast
flag to thesrc batch
commands. When enabled, any errors encountered while executing steps in a repository will stop the execution of the whole batch spec. The flag is off by default, so existing behavior is preserved.Cause()
Test plan
Updated tests.