fix(k8s): fix failing tasks not throwing errors

Before this fix, failing tasks (e.g. with an invalid command or args
configured) would succeed.

Also added integration tests for the failing case for tests and tasks
for our k8s plugins, and fixed/improved logging of test/task
logs and errors.

garden-service/src/commands/run/task.ts

@@ -82,7 +82,7 @@ export class RunTaskCommand extends Command<Args, Opts> {
       log.info("")
       // TODO: The command will need to be updated to stream logs: see https://github.com/garden-io/garden/issues/630.
       // It's ok with the current providers but the shape might change in the future.
-      log.info(chalk.white(result.output.outputs.log))
+      log.info(chalk.white(result.output.outputs.log.trim()))
       printFooter(footerLog)
     }
 

garden-service/src/plugins/kubernetes/container/run.ts

@@ -71,22 +71,20 @@ export async function runContainerTask(params: RunTaskParams<ContainerModule>):
     ignoreError: true, // to ensure results get stored when an error occurs
   })
 
-  const result = {
+  const result: RunTaskResult = {
     ...res,
     taskName: task.name,
     outputs: {
-      log: res.output || "",
+      log: res.log || "",
     },
   }
 
-  await storeTaskResult({
+  return storeTaskResult({
     ctx,
     log,
     module,
     result,
     taskVersion,
     taskName: task.name,
   })
-
-  return result
 }

garden-service/src/plugins/kubernetes/helm/handlers.ts

@@ -17,6 +17,7 @@ import { hotReloadHelmChart } from "./hot-reload"
 import { getServiceLogs } from "./logs"
 import { testHelmModule } from "./test"
 import { getPortForwardHandler } from "../port-forward"
+import { getTaskResult } from "../task-results"
 
 export const helmHandlers: Partial<ModuleAndRuntimeActionHandlers<HelmModule>> = {
   build: buildHelmModule,
@@ -27,6 +28,7 @@ export const helmHandlers: Partial<ModuleAndRuntimeActionHandlers<HelmModule>> =
   getPortForward: getPortForwardHandler,
   getServiceLogs,
   getServiceStatus,
+  getTaskResult,
   getTestResult,
   hotReloadService: hotReloadHelmChart,
   // TODO: add publishModule handler

garden-service/src/plugins/kubernetes/run.ts

@@ -376,7 +376,7 @@ export class PodRunner extends PodRunnerParams {
       version: module.version.versionString,
       startedAt,
       completedAt: new Date(),
-      log: res.output,
+      log: res.output + res.stderr,
       success: res.code === 0,
     }
   }

garden-service/src/plugins/kubernetes/task-results.ts

@@ -82,12 +82,19 @@ interface StoreTaskResultParams {
  *
  * TODO: Implement a CRD for this.
  */
-export async function storeTaskResult({ ctx, log, module, taskName, taskVersion, result }: StoreTaskResultParams) {
+export async function storeTaskResult({
+  ctx,
+  log,
+  module,
+  taskName,
+  taskVersion,
+  result,
+}: StoreTaskResultParams): Promise<RunTaskResult> {
   const provider = <KubernetesProvider>ctx.provider
   const api = await KubeApi.factory(log, provider)
   const namespace = await getMetadataNamespace(ctx, log, provider)
 
-  const data = trimRunOutput(result)
+  const data: RunTaskResult = trimRunOutput(result)
 
   await upsertConfigMap({
     api,
@@ -101,4 +108,6 @@ export async function storeTaskResult({ ctx, log, module, taskName, taskVersion,
     },
     data,
   })
+
+  return data
 }

garden-service/src/tasks/task.ts

@@ -30,6 +30,12 @@ export interface TaskTaskParams {
   forceBuild: boolean
 }
 
+class RunTaskError extends Error {
+  toString() {
+    return this.message
+  }
+}
+
 export class TaskTask extends BaseTask {
   // ... to be renamed soon.
   type: TaskType = "task"
@@ -158,11 +164,15 @@ export class TaskTask extends BaseTask {
       log.setError()
       throw err
     }
-
-    log.setSuccess({
-      msg: chalk.green(`Done (took ${log.getDuration(1)} sec)`),
-      append: true,
-    })
+    if (result.success) {
+      log.setSuccess({
+        msg: chalk.green(`Done (took ${log.getDuration(1)} sec)`),
+        append: true,
+      })
+    } else {
+      log.setError(`Failed!`)
+      throw new RunTaskError(result.log)
+    }
 
     return result
   }

garden-service/src/util/ext-tools.ts

@@ -320,8 +320,17 @@ export class BinaryCmd extends Library {
 
   async spawnAndWait({ args, cwd, env, log, ignoreError, stdout, stderr, timeout, tty }: SpawnParams) {
     const path = await this.getPath(log)
+
+    if (!args) {
+      args = []
+    }
+    if (!cwd) {
+      cwd = dirname(path)
+    }
+
+    log.debug(`Spawning '${path} ${args.join(" ")}' in ${cwd}`)
     return spawn(path, args || [], {
-      cwd: cwd || dirname(path),
+      cwd,
       timeout: this.getTimeout(timeout),
       ignoreError,
       env,

garden-service/test/integ/src/plugins/kubernetes/container/container.ts

@@ -330,6 +330,37 @@ describe("kubernetes container module handlers", () => {
       expect(result[key]!.output.log.trim()).to.equal("ok")
     })
 
+    it("should fail if an error occurs, but store the result", async () => {
+      const task = await graph.getTask("echo-task")
+      task.config.spec.command = ["bork"] // this will fail
+
+      const testTask = new TaskTask({
+        garden,
+        graph,
+        task,
+        log: garden.log,
+        force: true,
+        forceBuild: false,
+        version: task.module.version,
+      })
+
+      await expectError(
+        async () => await garden.processTasks([testTask], { throwOnError: true }),
+        (err) => expect(err.message).to.match(/bork/)
+      )
+
+      const actions = await garden.getActionRouter()
+
+      // We also verify that, despite the task failing, its result was still saved.
+      result = await actions.getTaskResult({
+        log: garden.log,
+        task,
+        taskVersion: task.module.version,
+      })
+
+      expect(result).to.exist
+    })
+
     context("artifacts are specified", () => {
       it("should copy artifacts out of the container", async () => {
         const task = await graph.getTask("artifacts-task")
@@ -450,6 +481,42 @@ describe("kubernetes container module handlers", () => {
       expect(result[key]!.output.log.trim()).to.equal("ok")
     })
 
+    it("should fail if an error occurs, but store the result", async () => {
+      const module = await graph.getModule("simple")
+
+      const testConfig = findByName(module.testConfigs, "echo-test")!
+      testConfig.spec.command = ["bork"] // this will fail
+
+      const testTask = new TestTask({
+        garden,
+        graph,
+        module,
+        testConfig,
+        log: garden.log,
+        force: true,
+        forceBuild: false,
+        version: module.version,
+        _guard: true,
+      })
+
+      await expectError(
+        async () => await garden.processTasks([testTask], { throwOnError: true }),
+        (err) => expect(err.message).to.match(/bork/)
+      )
+
+      const actions = await garden.getActionRouter()
+
+      // We also verify that, despite the test failing, its result was still saved.
+      result = await actions.getTestResult({
+        log: garden.log,
+        module,
+        testName: testConfig.name,
+        testVersion: testTask.version,
+      })
+
+      expect(result).to.exist
+    })
+
     context("artifacts are specified", () => {
       it("should copy artifacts out of the container", async () => {
         const module = await graph.getModule("simple")

garden-service/test/integ/src/plugins/kubernetes/helm/run.ts

@@ -1,6 +1,6 @@
 import { expect } from "chai"
 
-import { TestGarden } from "../../../../../helpers"
+import { TestGarden, expectError } from "../../../../../helpers"
 import { ConfigGraph } from "../../../../../../src/config-graph"
 import { getHelmTestGarden } from "./common"
 import { TaskTask } from "../../../../../../src/tasks/task"
@@ -40,6 +40,37 @@ describe("runHelmTask", () => {
     expect(result!.output.log.trim()).to.equal("ok")
   })
 
+  it("should fail if an error occurs, but store the result", async () => {
+    const task = await graph.getTask("echo-task")
+    task.config.spec.command = ["bork"] // this will fail
+
+    const testTask = new TaskTask({
+      garden,
+      graph,
+      task,
+      log: garden.log,
+      force: true,
+      forceBuild: false,
+      version: task.module.version,
+    })
+
+    await expectError(
+      async () => await garden.processTasks([testTask], { throwOnError: true }),
+      (err) => expect(err.message).to.match(/bork/)
+    )
+
+    const actions = await garden.getActionRouter()
+
+    // We also verify that, despite the task failing, its result was still saved.
+    const result = await actions.getTaskResult({
+      log: garden.log,
+      task,
+      taskVersion: task.module.version,
+    })
+
+    expect(result).to.exist
+  })
+
   context("artifacts are specified", () => {
     it("should copy artifacts out of the container", async () => {
       const task = await graph.getTask("artifacts-task")

garden-service/test/integ/src/plugins/kubernetes/helm/test.ts

@@ -1,6 +1,6 @@
 import { expect } from "chai"
 
-import { TestGarden } from "../../../../../helpers"
+import { TestGarden, expectError } from "../../../../../helpers"
 import { ConfigGraph } from "../../../../../../src/config-graph"
 import { getHelmTestGarden } from "./common"
 import { TestTask } from "../../../../../../src/tasks/test"
@@ -43,6 +43,42 @@ describe("testHelmModule", () => {
     expect(result!.output.log.trim()).to.equal("ok")
   })
 
+  it("should fail if an error occurs, but store the result", async () => {
+    const module = await graph.getModule("artifacts")
+
+    const testConfig = findByName(module.testConfigs, "echo-test")!
+    testConfig.spec.command = ["bork"] // this will fail
+
+    const testTask = new TestTask({
+      garden,
+      graph,
+      module,
+      testConfig,
+      log: garden.log,
+      force: true,
+      forceBuild: false,
+      version: module.version,
+      _guard: true,
+    })
+
+    await expectError(
+      async () => await garden.processTasks([testTask], { throwOnError: true }),
+      (err) => expect(err.message).to.match(/bork/)
+    )
+
+    const actions = await garden.getActionRouter()
+
+    // We also verify that, despite the test failing, its result was still saved.
+    const result = await actions.getTestResult({
+      log: garden.log,
+      module,
+      testName: testConfig.name,
+      testVersion: testTask.version,
+    })
+
+    expect(result).to.exist
+  })
+
   context("artifacts are specified", () => {
     it("should copy artifacts out of the container", async () => {
       const module = await graph.getModule("artifacts")