fix: better and more consistent error handling in CLI commands

src/cli.ts

@@ -26,9 +26,14 @@ import {
   Parameter,
   StringParameter,
   EnvironmentOption,
+  CommandResult,
 } from "./commands/base"
 import { ValidateCommand } from "./commands/validate"
-import { InternalError, PluginError } from "./exceptions"
+import {
+  GardenError,
+  InternalError,
+  PluginError,
+} from "./exceptions"
 import { Garden } from "./garden"
 import { FileWriter } from "./logger/writers"
 import { getLogger, RootLogNode } from "./logger"
@@ -139,7 +144,7 @@ interface OptConfig {
 export interface ParseResults {
   argv: any
   code: number
-  errors: Error[]
+  errors: (GardenError | Error)[]
 }
 
 interface SywacParseResults extends ParseResults {
@@ -320,7 +325,6 @@ export class GardenCli {
 
       // We attach the action result to cli context so that we can process it in the parse method
       cliContext.details.result = result
-
     }
 
     // Command specific positional args and options are set inside the builder function
@@ -342,25 +346,30 @@ export class GardenCli {
 
   async parse(): Promise<ParseResults> {
     const parseResult: SywacParseResults = await this.program.parse()
-    const { argv, details, errors, code, output: cliOutput } = parseResult
-    const { result } = details
+    const { argv, details, errors: parseErrors, output: cliOutput } = parseResult
+    const commandResult: CommandResult = details.result
     const { output } = argv
 
+    let code = parseResult.code
+
     // --help or --version options were called so we log the cli output and exit
-    if (cliOutput && errors.length < 1) {
+    if (cliOutput && parseErrors.length < 1) {
       this.logger.stop()
       console.log(cliOutput)
       process.exit(parseResult.code)
     }
 
-    // --output option set and there is content to output
-    if (output && (result !== undefined || errors.length > 0)) {
+    const errors: GardenError[] = parseErrors
+      .map(e => ({ type: "parameter", message: e.toString() }))
+      .concat(commandResult.errors || [])
+
+    // --output option set
+    if (output) {
       const renderer = OUTPUT_RENDERERS[output]
       if (errors.length > 0) {
-        const msg = errors.join("\n")
-        console.error(renderer({ result: msg }))
+        console.error(renderer({ success: false, errors }))
       } else {
-        console.log(renderer({ success: true, result }))
+        console.log(renderer({ success: true, ...commandResult }))
       }
       // Note: this circumvents an issue where the process exits before the output is fully flushed
       await sleep(100)
@@ -370,8 +379,10 @@ export class GardenCli {
       errors.forEach(err => this.logger.error({ msg: err.message, error: err }))
 
       if (this.logger.writers.find(w => w instanceof FileWriter)) {
-        this.logger.info(`See ${ERROR_LOG_FILENAME} for detailed error message`)
+        this.logger.info(`\nSee ${ERROR_LOG_FILENAME} for detailed error message`)
       }
+
+      code = 1
     }
 
     this.logger.stop()

src/commands/base.ts

@@ -6,7 +6,12 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+import {
+  GardenError,
+  RuntimeError,
+} from "../exceptions"
 import { PluginContext } from "../plugin-context"
+import { TaskResults } from "../task-graph"
 
 export class ValidationError extends Error { }
 
@@ -120,6 +125,11 @@ export interface CommandConstructor {
   new(parent?: Command): Command
 }
 
+export interface CommandResult<T = any> {
+  result?: T
+  errors?: GardenError[]
+}
+
 export abstract class Command<T extends Parameters = {}, U extends Parameters = {}> {
   abstract name: string
   abstract help: string
@@ -141,5 +151,22 @@ export abstract class Command<T extends Parameters = {}, U extends Parameters =
   // subclass implementations need to explicitly set the types in the implemented function signature. So for now we
   // can't enforce the types of `args` and `opts` automatically at the abstract class level and have to specify
   // the types explicitly on the subclassed methods.
-  abstract async action(ctx: PluginContext, args: ParameterValues<T>, opts: ParameterValues<U>): Promise<any>
+  abstract async action(ctx: PluginContext, args: ParameterValues<T>, opts: ParameterValues<U>): Promise<CommandResult>
+}
+
+export async function handleTaskResults(
+  ctx: PluginContext, taskType: string, result: TaskResults,
+): Promise<CommandResult<TaskResults>> {
+  const failed = Object.values(result).filter(r => !!r.error).length
+
+  if (failed) {
+    const error = new RuntimeError(`${failed} ${taskType} task(s) failed!`, {
+      result,
+    })
+    return { errors: [error] }
+  } else {
+    ctx.log.info("")
+    ctx.log.header({ emoji: "heavy_check_mark", command: `Done!` })
+    return { result }
+  }
 }

src/commands/build.ts

@@ -7,7 +7,14 @@
  */
 
 import { PluginContext } from "../plugin-context"
-import { BooleanParameter, Command, ParameterValues, StringParameter } from "./base"
+import {
+  BooleanParameter,
+  Command,
+  CommandResult,
+  handleTaskResults,
+  ParameterValues,
+  StringParameter,
+} from "./base"
 import { BuildTask } from "../tasks/build"
 import { TaskResults } from "../task-graph"
 
@@ -32,24 +39,21 @@ export class BuildCommand extends Command<typeof buildArguments, typeof buildOpt
   arguments = buildArguments
   options = buildOptions
 
-  async action(ctx: PluginContext, args: BuildArguments, opts: BuildOptions): Promise<TaskResults> {
+  async action(ctx: PluginContext, args: BuildArguments, opts: BuildOptions): Promise<CommandResult<TaskResults>> {
     await ctx.clearBuilds()
     const names = args.module ? args.module.split(",") : undefined
     const modules = await ctx.getModules(names)
 
-    ctx.log.header({ emoji: "hammer", command: "build" })
+    ctx.log.header({ emoji: "hammer", command: "Build" })
 
-    const result = await ctx.processModules({
+    const results = await ctx.processModules({
       modules,
       watch: opts.watch,
       process: async (module) => {
         return [await BuildTask.factory({ ctx, module, force: opts.force })]
       },
     })
 
-    ctx.log.info("")
-    ctx.log.header({ emoji: "heavy_check_mark", command: `Done!` })
-
-    return result
+    return handleTaskResults(ctx, "build", results)
   }
 }

src/commands/call.ts

@@ -119,10 +119,12 @@ export class CallCommand extends Command<typeof callArgs> {
     res.data && ctx.log.info(chalk.white(resStr))
 
     return {
-      serviceName,
-      path,
-      url,
-      response: pick(res, ["status", "statusText", "headers", "data"]),
+      result: {
+        serviceName,
+        path,
+        url,
+        response: pick(res, ["status", "statusText", "headers", "data"]),
+      },
     }
   }
 }

src/commands/config/delete.ts

@@ -7,7 +7,13 @@
  */
 
 import { PluginContext } from "../../plugin-context"
-import { Command, ParameterValues, StringParameter } from "../base"
+import { DeleteConfigResult } from "../../types/plugin/outputs"
+import {
+  Command,
+  CommandResult,
+  ParameterValues,
+  StringParameter,
+} from "../base"
 import { NotFoundError } from "../../exceptions"
 
 export const configDeleteArgs = {
@@ -28,16 +34,16 @@ export class ConfigDeleteCommand extends Command<typeof configDeleteArgs> {
 
   arguments = configDeleteArgs
 
-  async action(ctx: PluginContext, args: DeleteArgs) {
+  async action(ctx: PluginContext, args: DeleteArgs): Promise<CommandResult<DeleteConfigResult>> {
     const key = args.key.split(".")
-    const res = await ctx.deleteConfig({ key })
+    const result = await ctx.deleteConfig({ key })
 
-    if (res.found) {
+    if (result.found) {
       ctx.log.info(`Deleted config key ${args.key}`)
     } else {
       throw new NotFoundError(`Could not find config key ${args.key}`, { key })
     }
 
-    return { ok: true }
+    return { result }
   }
 }

src/commands/config/index.ts

@@ -22,5 +22,5 @@ export class ConfigCommand extends Command {
     ConfigDeleteCommand,
   ]
 
-  async action() { }
+  async action() { return {} }
 }

src/commands/config/set.ts

@@ -7,7 +7,13 @@
  */
 
 import { PluginContext } from "../../plugin-context"
-import { Command, ParameterValues, StringParameter } from "../base"
+import { SetConfigResult } from "../../types/plugin/outputs"
+import {
+  Command,
+  CommandResult,
+  ParameterValues,
+  StringParameter,
+} from "../base"
 
 export const configSetArgs = {
   key: new StringParameter({
@@ -30,10 +36,10 @@ export class ConfigSetCommand extends Command<typeof configSetArgs> {
 
   arguments = configSetArgs
 
-  async action(ctx: PluginContext, args: SetArgs) {
+  async action(ctx: PluginContext, args: SetArgs): Promise<CommandResult<SetConfigResult>> {
     const key = args.key.split(".")
-    await ctx.setConfig({ key, value: args.value })
+    const result = await ctx.setConfig({ key, value: args.value })
     ctx.log.info(`Set config key ${args.key}`)
-    return { ok: true }
+    return { result }
   }
 }

src/commands/deploy.ts

@@ -8,7 +8,14 @@
 
 import { PluginContext } from "../plugin-context"
 import { DeployTask } from "../tasks/deploy"
-import { BooleanParameter, Command, ParameterValues, StringParameter } from "./base"
+import {
+  BooleanParameter,
+  Command,
+  CommandResult,
+  handleTaskResults,
+  ParameterValues,
+  StringParameter,
+} from "./base"
 import { TaskResults } from "../task-graph"
 
 export const deployArgs = {
@@ -34,13 +41,13 @@ export class DeployCommand extends Command<typeof deployArgs, typeof deployOpts>
   arguments = deployArgs
   options = deployOpts
 
-  async action(ctx: PluginContext, args: Args, opts: Opts): Promise<TaskResults> {
+  async action(ctx: PluginContext, args: Args, opts: Opts): Promise<CommandResult<TaskResults>> {
     const names = args.service ? args.service.split(",") : undefined
     const services = await ctx.getServices(names)
 
     if (services.length === 0) {
       ctx.log.warn({ msg: "No services found. Aborting." })
-      return {}
+      return { result: {} }
     }
 
     ctx.log.header({ emoji: "rocket", command: "Deploy" })
@@ -52,17 +59,14 @@ export class DeployCommand extends Command<typeof deployArgs, typeof deployOpts>
     const force = opts.force
     const forceBuild = opts["force-build"]
 
-    const result = await ctx.processServices({
+    const results = await ctx.processServices({
       services,
       watch,
       process: async (service) => {
         return [await DeployTask.factory({ ctx, service, force, forceBuild })]
       },
     })
 
-    ctx.log.info("")
-    ctx.log.header({ emoji: "heavy_check_mark", command: `Done!` })
-
-    return result
+    return handleTaskResults(ctx, "deploy", results)
   }
 }

src/commands/dev.ts

@@ -43,7 +43,7 @@ export class DevCommand extends Command {
         ctx.log.info({ msg: "No modules found in project." })
       }
       ctx.log.info({ msg: "Aborting..." })
-      return
+      return {}
     }
 
     await ctx.processModules({
@@ -61,6 +61,8 @@ export class DevCommand extends Command {
         }
       },
     })
+
+    return {}
   }
 }
 

src/commands/environment/destroy.ts

@@ -8,23 +8,26 @@
 
 import { PluginContext } from "../../plugin-context"
 
-import { Command } from "../base"
+import {
+  Command,
+  CommandResult,
+} from "../base"
 import { EnvironmentStatusMap } from "../../types/plugin/outputs"
 
 export class EnvironmentDestroyCommand extends Command {
   name = "destroy"
   alias = "d"
   help = "Destroy environment"
 
-  async action(ctx: PluginContext): Promise<EnvironmentStatusMap> {
+  async action(ctx: PluginContext): Promise<CommandResult<EnvironmentStatusMap>> {
     const { name } = ctx.getEnvironment()
     ctx.log.header({ emoji: "skull_and_crossbones", command: `Destroying ${name} environment` })
 
     const result = await ctx.destroyEnvironment({})
 
     ctx.log.finish()
 
-    return result
+    return { result }
   }
 
 }

src/commands/environment/index.ts

@@ -20,5 +20,5 @@ export class EnvironmentCommand extends Command {
     EnvironmentDestroyCommand,
   ]
 
-  async action() { }
+  async action() { return {} }
 }