-
Notifications
You must be signed in to change notification settings - Fork 289
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 experiment to avoid a recursive trap #2209
Add experiment to avoid a recursive trap #2209
Conversation
9cbdd8f
to
5ba40b9
Compare
internal/job/executor.go
Outdated
@@ -1966,10 +1976,19 @@ func (e *Executor) defaultCommandPhase(ctx context.Context) error { | |||
return err | |||
} | |||
|
|||
// Update: The comment below is of dubious validity, because: |
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.
Might be better to incorporate or replace the dubious comment...
// The below is the "recursive trap".
// The original idea was to ...
// But ..., so we are phasing it out under an experiment.
// to show the error to users. | ||
e.shell.Errorf("The command was interrupted by a signal: %v", commandErr) | ||
|
||
// although error is an exit error, it's not returned. (seems like a bug) |
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.
(seems like a bug)
seems like a bug.
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.
Ship it ✅
Some jobs are run as a bash script of the form:
We now understand this causes a bug (see the code comments), and we want to avoid it.
This PR adds an experiment to avoid adding the trap to the script.