improvement: make sure artifacts are always fetched (#6532)

* improvement: make sure artifacts are always fetched * chore: add comments * improvement: always fetch artifacts in Run exec as well * fix: some edge cases * chore: comments * Update core/src/plugins/exec/test.ts Co-authored-by: Steffen Neubauer <steffen@garden.io> * fix: wrong condition * refactor: use let instead of array and avoid implicit any --------- Co-authored-by: Steffen Neubauer <steffen@garden.io>
garden-io · Oct 17, 2024 · 593c9cb · 593c9cb
1 parent a0ef5f1
commit 593c9cb
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 7 deletions.
diff --git a/core/src/plugins/exec/run.ts b/core/src/plugins/exec/run.ts
@@ -14,6 +14,7 @@ import { styles } from "../../logger/styles.js"
 import { copyArtifacts, execRunCommand } from "./common.js"
 import { execRunSpecSchema, execRuntimeOutputsSchema, execStaticOutputsSchema } from "./config.js"
 import { execProvider } from "./exec.js"
+import { InternalError } from "../../exceptions.js"
 
 export const execRun = execProvider.createActionType({
   kind: "Run",
@@ -32,8 +33,38 @@ export type ExecRun = GardenSdkActionDefinitionActionType<typeof execRun>
 execRun.addHandler("run", async ({ artifactsPath, log, action, ctx }) => {
   const startedAt = new Date()
   const { command, env, artifacts } = action.getSpec()
+  let runCommandError: unknown | undefined
 
-  const commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
+  let commandResult: Awaited<ReturnType<typeof execRunCommand>> | undefined
+  try {
+    // Execute the run command
+    commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
+  } catch (error) {
+    // Store error to be thrown at the end after trying to fetch artifacts
+    runCommandError = error
+  }
+
+  try {
+    // Try to fetch artifacts
+    await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
+  } catch (error) {
+    if (runCommandError || !commandResult?.success) {
+      // If the run command has failed or thrown an error, and the artifact copy has failed as well, we just log the artifact copy error
+      // since we'll trow the run command error later on.
+      log.error(`Failed to copy artifacts: ${error}`)
+    } else {
+      throw error
+    }
+  }
+
+  if (runCommandError) {
+    // The run command failed, so we throw the error
+    throw runCommandError
+  } else if (!commandResult) {
+    throw new InternalError({
+      message: "CommandResult should not be undefined if there was no error",
+    })
+  }
 
   const detail = {
     moduleName: action.moduleName(),
@@ -70,7 +101,5 @@ execRun.addHandler("run", async ({ artifactsPath, log, action, ctx }) => {
     )
   }
 
-  await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
-
   return result
 })
diff --git a/core/src/plugins/exec/test.ts b/core/src/plugins/exec/test.ts
@@ -14,6 +14,7 @@ import { styles } from "../../logger/styles.js"
 import { copyArtifacts, execRunCommand } from "./common.js"
 import { execRunSpecSchema, execRuntimeOutputsSchema, execStaticOutputsSchema } from "./config.js"
 import { execProvider } from "./exec.js"
+import { InternalError } from "../../exceptions.js"
 
 export const execTestSpecSchema = execRunSpecSchema
 
@@ -34,8 +35,38 @@ export type ExecTest = GardenSdkActionDefinitionActionType<typeof execTest>
 execTest.addHandler("run", async ({ log, action, artifactsPath, ctx }) => {
   const startedAt = new Date()
   const { command, env, artifacts } = action.getSpec()
+  let runCommandError: unknown | undefined
 
-  const commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
+  let commandResult: Awaited<ReturnType<typeof execRunCommand>> | undefined
+  try {
+    // Execute the test command
+    commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
+  } catch (error) {
+    // Store error to be thrown at the end after trying to fetch artifacts
+    runCommandError = error
+  }
+
+  try {
+    // Try to fetch artifacts
+    await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
+  } catch (error) {
+    if (runCommandError || !commandResult?.success) {
+      // If the test command has failed or thrown an error, and the artifact copy has failed as well, we just log the artifact copy error
+      // since we'll trow the test command error later on.
+      log.error(`Failed to copy artifacts: ${error}`)
+    } else {
+      throw error
+    }
+  }
+
+  if (runCommandError) {
+    // The test command failed, so we throw the error
+    throw runCommandError
+  } else if (!commandResult) {
+    throw new InternalError({
+      message: "CommandResult should not be undefined if there was no error",
+    })
+  }
 
   const detail = {
     moduleName: action.moduleName(),
@@ -71,8 +102,5 @@ execTest.addHandler("run", async ({ log, action, artifactsPath, ctx }) => {
       })
     )
   }
-
-  await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
-
   return result
 })