-
Notifications
You must be signed in to change notification settings - Fork 21
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
Return error instead of killing process #24
Conversation
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.
Hi @vvakar Thanks for your interest in contributing to the project.
The changes you propose are breaking changes making any existing test that uses this extension to change their behavior.
I think it would be better to add an option that controls this behavior and ensure the current behavior is the default. For example, add the option ContinueOnError
and
if err != nil && !opts.ContinueOnError then {
log.Fatal(err.Error() + " on command: " + name + " " + strings.Join(args, " "))
}
return string(out), err
Hi @pablochacin, point well taken! I'll make that change. |
440888f
to
ce48e8d
Compare
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.
LGTM
Thanks for approving @pablochacin! Pls note - I don't have write access to merge the PR. |
Rather than kill the whole load test (which is what
log.Fatal
does), proposing to delegate error handling so user can decide on: