Use result.Result in more places. #2568
Conversation
| return nil, res | ||
| } | ||
|
|
||
| return nil, result.Error("an error occurred while advancing the preview") |
There was a problem hiding this comment.
unfortunate. it seems like we've lost the original err information (this was a problem before).
There was a problem hiding this comment.
I think that's OK - we should have already printed it, right?
There was a problem hiding this comment.
Oh, nevermind - in this case, we won't have printed it. Could you instead return res verbatim without creating the new error?
| "run_error": { | ||
| program: path.join(base, "040.run_error"), | ||
| expectResourceCount: 0, | ||
| expectError: "Program exited with non-zero exit code: 1", |
There was a problem hiding this comment.
this error will change in a followup PR.
CyrusNajmabadi
added
the
impact/no-changelog-required
| if res != nil { | ||
| err = res.Error() | ||
| } | ||
|
|
||
| // Figure out if execution failed and why. Step generation and execution errors trump cancellation. | ||
| if err != nil || pe.stepExec.Errored() { | ||
| err = execError("failed", preview) | ||
| } else if canceled { | ||
| err = execError("canceled", preview) |
There was a problem hiding this comment.
I'm thinking this should be a bail as well; cancellation doesn't sound to me like something that should be a non-bail Result.
Co-Authored-By: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
|
Talked to @swgillespie . Will take on some of hte feedback later when we can be sure we don't make any scenarios worse. specificaly, we don't wnat to end up printing out the same error multiple times. |
No description provided.