improvement: make sure artifacts are always fetched #6532
Conversation
|
Sorry I should have made it a draft. These changes do not account to all the use-cases unfortunately, since if the run command returns a non-zero exit code, it will not throw. So I am amending the code as we speak. |
|
Good to go now, @vvagaytsev |
core/src/plugins/exec/run.ts
Outdated
| @@ -32,8 +32,34 @@ export type ExecRun = GardenSdkActionDefinitionActionType<typeof execRun> | |||
| execRun.addHandler("run", async ({ artifactsPath, log, action, ctx }) => { | |||
| const startedAt = new Date() | |||
| const { command, env, artifacts } = action.getSpec() | |||
| const runCommandErrors: unknown[] = [] | |||
There was a problem hiding this comment.
If there's always one element in the array, I think it's cleaner to use let here:
| const runCommandErrors: unknown[] = [] | |
| let runCommandError: unknown | undefined |
There was a problem hiding this comment.
runCommandErrors is never reassigned, it will give you a type error if you use let.
There was a problem hiding this comment.
it's never reassigned because you push an element to the array, but why use an array in the first place if it'll never have more than one element?
There was a problem hiding this comment.
aaah, that's what you meant. Sorry didn't understand you from your first comment. Sure thing, that makes sense.
There was a problem hiding this comment.
@10ko I've pushed a commit with the change I had in mind, does it look good to you?
Co-authored-by: Steffen Neubauer <steffen@garden.io>
What this PR does / why we need it:
When executing
RunorTestactions of typeexec, we now always try to export the artifacts.More specifically, we make sure to fetch artifacts in the case of:
spec.commandfailing with a non-zero exit code.spec.commandthrowing unexpectedly.In case the
spec.commandruns successfully and the fetching fails, we maintain the current behaviour of throwing an error.This PR closes #6524
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: