-
Notifications
You must be signed in to change notification settings - Fork 273
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
fix(exec): fix error handling in exec run and test actions #6319
Merged
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
Add missing handler for `CpyError`. Do not crash, throw a `RuntimeError` instead.
Otherwise, it won't appear if case of artifacts copying error
Otherwise, a confusing error message on missing artifacts may appear. This also prevents duplicate logging of command output.
To have the same order as for run actions.
thsig
requested changes
Jul 25, 2024
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.
Had one comment there, looks good otherwise!
vvagaytsev
force-pushed
the
fix/6261-crash-when-test-fails
branch
from
July 25, 2024 15:05
165e487
to
64d2dde
Compare
thsig
previously approved these changes
Jul 25, 2024
thsig
approved these changes
Jul 25, 2024
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.
What this PR does / why we need it:
This PR fixes the command result processing and error handling in
Run
andTest
actions ofexec
type, and align the error handling behaviour between the commands.:Which issue(s) this PR fixes:
Fixes #6261
Special notes for your reviewer:
The changes have been tested manually. We do not have integration tests for exec provider yet, so let's skip it. We do not change the affected part of the code often.
There is another function
copyArtifacts
inutil/artifacts.ts
which is used on the action router-level in:Both places seem to be irrelevant to the issue in question. There is a kind of clean-up in the finally block where the produced artifacts are copied form the temp directory before cleaning it up.
I'm not sure if we need to do that at all, but let's leave the code as-is, and change that part in a separate PR if necessary.