Skip to content
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
merged 12 commits into from
Jul 25, 2024

Conversation

vvagaytsev
Copy link
Collaborator

@vvagaytsev vvagaytsev commented Jul 25, 2024

What this PR does / why we need it:

This PR fixes the command result processing and error handling in Run and Test actions of exec type, and align the error handling behaviour between the commands.:

  • an error is returned immediately if the command exited with non-zero code, so Garden does not attempt to copy missing or incomplete artifacts; the error message includes the command output and the exit code.
  • the command execution output is always printed in the console if the command was successful; in case of error, it will be printed as a part of the error message, and won't be printed twice.
  • the artifacts are copied only if the command execution as successful
  • I/O errors while artifacts copying do not crash the Garden process, and are handled as the other runtime errors

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 in util/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.

@vvagaytsev vvagaytsev requested a review from stefreak July 25, 2024 10:57
@vvagaytsev vvagaytsev marked this pull request as ready for review July 25, 2024 10:57
@vvagaytsev vvagaytsev requested a review from thsig July 25, 2024 11:38
@vvagaytsev vvagaytsev enabled auto-merge July 25, 2024 14:14
Copy link
Collaborator

@thsig thsig left a 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!

core/src/plugins/exec/common.ts Show resolved Hide resolved
@vvagaytsev vvagaytsev requested a review from thsig July 25, 2024 15:04
@vvagaytsev vvagaytsev force-pushed the fix/6261-crash-when-test-fails branch from 165e487 to 64d2dde Compare July 25, 2024 15:05
thsig
thsig previously approved these changes Jul 25, 2024
@vvagaytsev vvagaytsev added this pull request to the merge queue Jul 25, 2024
Merged via the queue into main with commit 6a03430 Jul 25, 2024
40 checks passed
@vvagaytsev vvagaytsev deleted the fix/6261-crash-when-test-fails branch July 25, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash: Garden crashes on I/O errors while artifacts copying
2 participants