From ff647e3cf4066ae3f2606c89f4d27c32e08dd781 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 30 Aug 2023 23:00:17 +0200 Subject: [PATCH 01/17] fix(framework): several error handling improvements - Made sure that InternalError is only used for errors that "should not happen" and also to wrap unhandled errors that originate from libraries or the NodeJS standard library. - Stop masking leaf errors (leaf means basically the root cause). One of the most occuring errors on Amplitude is "graph", but that's a wrapping error. This will give us better insights in what error types actually causing command failures. - Consistently handle unexpected errors and format them equally on all levels. - Format InternalErrors as crashes with an explanation and a call to action to report the bug on all levels - Treat uncaught errors on the CLI level (cli.ts) as crashes, even if they are GardenError instances other than InternalError, because the GardenCli class should not throw but return an exit code. --- cli/src/cli.ts | 16 +++- core/src/actions/base.ts | 4 +- core/src/build-staging/helpers.ts | 8 +- core/src/cache.ts | 5 +- core/src/cli/cli.ts | 33 +++++--- core/src/cli/helpers.ts | 5 +- core/src/commands/base.ts | 4 +- core/src/commands/custom.ts | 2 +- core/src/commands/login.ts | 2 +- core/src/commands/version.ts | 9 ++- core/src/config-store/base.ts | 2 +- core/src/docs/template-strings.ts | 2 +- core/src/exceptions.ts | 96 +++++++++++++++++------ core/src/garden.ts | 9 ++- core/src/graph/actions.ts | 2 +- core/src/graph/config-graph.ts | 1 + core/src/graph/nodes.ts | 10 ++- core/src/graph/results.ts | 18 ++++- core/src/graph/solver.ts | 17 ++-- core/src/logger/logger.ts | 5 +- core/src/logger/renderers.ts | 6 +- core/src/plugins/kubernetes/kubernetes.ts | 2 +- core/src/plugins/kubernetes/util.ts | 7 +- core/src/router/base.ts | 8 +- core/src/router/module.ts | 4 +- core/src/tasks/resolve-action.ts | 5 +- core/src/util/streams.ts | 5 +- core/src/util/util.ts | 18 ++++- core/src/watch.ts | 7 +- core/test/unit/src/commands/version.ts | 2 +- 30 files changed, 219 insertions(+), 95 deletions(-) diff --git a/cli/src/cli.ts b/cli/src/cli.ts index c495da6faa..94de879774 100644 --- a/cli/src/cli.ts +++ b/cli/src/cli.ts @@ -13,6 +13,7 @@ import { GlobalConfigStore } from "@garden-io/core/build/src/config-store/global import { getOtelSDK } from "@garden-io/core/build/src/util/open-telemetry/tracing" import { withContextFromEnv } from "@garden-io/core/build/src/util/open-telemetry/propagation" import { wrapActiveSpan } from "@garden-io/core/build/src/util/open-telemetry/spans" +import { InternalError, explainGardenError } from "@garden-io/core/build/src/exceptions" // These plugins are always registered export const getBundledPlugins = (): GardenPluginReference[] => [ @@ -53,8 +54,7 @@ export async function runCli({ ) code = result.code } catch (err) { - // eslint-disable-next-line no-console - console.log(`Warning: Exiting with unhandled error\n${err.message}`) + logUnexpectedError(err, "Unhandled error") code = 1 } finally { if (cli?.processRecord) { @@ -65,8 +65,7 @@ export async function runCli({ try { await Promise.race([getOtelSDK().shutdown(), new Promise((resolve) => setTimeout(resolve, 3000))]) } catch (err) { - // eslint-disable-next-line no-console - console.log(`Debug: OTEL shutdown failed with error ${err.toString()}`) + logUnexpectedError(err, "OTEL shutdown failed") } await shutdown(code) @@ -74,3 +73,12 @@ export async function runCli({ return { cli, result } } + +function logUnexpectedError(error: Error, context: string) { + // NOTE: If this function is called, this is always a bug, because GardenCli.run is designed to return an error code. If it throws an error, something is wrong with our code and we need to fix it. + // This is why we wrap the error with InternalError here, even if it is a GardenBaseError. + // If an error hits this code path, it's definitely a crash and we need to fix that bug. + const wrappedError = InternalError.wrapError(error, {}, context) + // eslint-disable-next-line no-console + console.log(`${context}: ${explainGardenError(wrappedError, context)}`) +} diff --git a/core/src/actions/base.ts b/core/src/actions/base.ts index 7ee8cc5274..db16032115 100644 --- a/core/src/actions/base.ts +++ b/core/src/actions/base.ts @@ -37,7 +37,7 @@ import cloneDeep from "fast-copy" import { flatten, fromPairs, isString, memoize, omit, sortBy } from "lodash" import { ActionConfigContext, ActionSpecContext } from "../config/template-contexts/actions" import { relative } from "path" -import { InternalError } from "../exceptions" +import { ConfigurationError } from "../exceptions" import { Action, ActionConfig, @@ -731,7 +731,7 @@ export abstract class ResolvedRuntimeAction< const buildAction = this.getResolvedDependencies().find((a) => a.kind === "Build" && a.name === buildName) if (!buildAction) { - throw new InternalError({ + throw new ConfigurationError({ message: `Could not find build dependency '${buildName}' specified on the build field on ${this.longDescription()}.`, detail: { action: this.key(), buildName }, }) diff --git a/core/src/build-staging/helpers.ts b/core/src/build-staging/helpers.ts index 90600c5fa0..91573560e5 100644 --- a/core/src/build-staging/helpers.ts +++ b/core/src/build-staging/helpers.ts @@ -370,13 +370,7 @@ export class FileStatsHelper { // Symlink target not found, so we ignore it return cb(null, null) } else if (readlinkErr) { - return cb( - new InternalError({ - message: `Error reading symlink: ${readlinkErr.message}`, - detail: { path, readlinkErr }, - }), - null - ) + return cb(InternalError.wrapError(readlinkErr, { path, readlinkErr }, "Error reading symlink"), null) } // Ignore absolute symlinks unless specifically allowed diff --git a/core/src/cache.ts b/core/src/cache.ts index 33f213f8e1..c20966e988 100644 --- a/core/src/cache.ts +++ b/core/src/cache.ts @@ -172,7 +172,10 @@ export class TreeCache { pairs = Array.from(node.entries).map((stringKey) => { const entry = this.cache.get(stringKey) if (!entry) { - throw new InternalError({ message: `Invalid reference found in cache: ${stringKey}`, detail: { stringKey } }) + throw new InternalError({ + message: `Invalid reference found in cache: ${stringKey}`, + detail: { stringKey }, + }) } return <[CacheKey, CacheValue]>[entry.key, entry.value] }) diff --git a/core/src/cli/cli.ts b/core/src/cli/cli.ts index 2c4ecfd8f9..dc1286fb13 100644 --- a/core/src/cli/cli.ts +++ b/core/src/cli/cli.ts @@ -13,7 +13,7 @@ import { pathExists } from "fs-extra" import { getBuiltinCommands } from "../commands/commands" import { shutdown, getPackageVersion, getCloudDistributionName } from "../util/util" import { Command, CommandResult, CommandGroup, BuiltinArgs } from "../commands/base" -import { PluginError, toGardenError, GardenBaseError } from "../exceptions" +import { PluginError, toGardenError, GardenBaseError, explainGardenError } from "../exceptions" import { Garden, GardenOpts, makeDummyGarden } from "../garden" import { getRootLogger, getTerminalWriterType, LogLevel, parseLogLevel, RootLogger } from "../logger/logger" import { FileWriter, FileWriterConfig } from "../logger/writers/file-writer" @@ -402,6 +402,11 @@ ${renderCommands(commands)} processRecord?: GardenProcess cwd?: string }): Promise { + // for testing + if (args.includes("--test-cli-error")) { + throw new Error("Test") + } + let argv = parseCliArgs({ stringArgs: args, cli: true }) const errors: (GardenBaseError | Error)[] = [] @@ -443,14 +448,19 @@ ${renderCommands(commands)} "logger-type": loggerTypeOpt, "log-level": logLevelStr, } = argv - const logger = RootLogger.initialize({ - level: parseLogLevel(logLevelStr), - storeEntries: false, - displayWriterType: getTerminalWriterType({ silent, output, loggerTypeOpt, commandLoggerType: null }), - useEmoji: emoji, - showTimestamps, - force: this.initLogger, - }) + let logger: RootLogger + try { + logger = RootLogger.initialize({ + level: parseLogLevel(logLevelStr), + storeEntries: false, + displayWriterType: getTerminalWriterType({ silent, output, loggerTypeOpt, commandLoggerType: null }), + useEmoji: emoji, + showTimestamps, + force: this.initLogger, + }) + } catch (error) { + return done(1, explainGardenError(error, "Failed to initialize logger")) + } const log = logger.createLog() log.verbose(`garden version: ${getPackageVersion()}`) @@ -562,7 +572,10 @@ ${renderCommands(commands)} if (gardenErrors.length > 0 || (commandResult.exitCode && commandResult.exitCode !== 0)) { return done( commandResult.exitCode || 1, - renderer({ success: false, errors: gardenErrors }), + renderer({ + success: false, + errors: gardenErrors, + }), commandResult?.result ) } else { diff --git a/core/src/cli/helpers.ts b/core/src/cli/helpers.ts index 2b6074ecf6..8307e6011c 100644 --- a/core/src/cli/helpers.ts +++ b/core/src/cli/helpers.ts @@ -19,7 +19,7 @@ import { maxBy, zip } from "lodash" import { Logger } from "../logger/logger" import { ParameterValues, Parameter, Parameters, globalDisplayOptions } from "./params" -import { GardenBaseError, InternalError, ParameterError, toGardenError } from "../exceptions" +import { GardenBaseError, ParameterError, RuntimeError, toGardenError } from "../exceptions" import { getPackageVersion, removeSlice } from "../util/util" import { Log } from "../logger/log-entry" import { STATIC_DIR, VERSION_CHECK_URL, gardenEnv, ERROR_LOG_FILENAME } from "../constants" @@ -58,7 +58,8 @@ export function helpTextMaxWidth() { export async function checkForStaticDir() { if (!(await pathExists(STATIC_DIR))) { - throw new InternalError({ + // if this happens, this is most likely not a Garden bug but some kind of corrupted installation, and the user needs to do something about it. + throw new RuntimeError({ message: `Could not find the static data directory. Garden is packaged with a data directory ` + `called 'static', which should be located next to your garden binary. Please try reinstalling, ` + diff --git a/core/src/commands/base.ts b/core/src/commands/base.ts index 523fe68c5d..87d4e83cec 100644 --- a/core/src/commands/base.ts +++ b/core/src/commands/base.ts @@ -20,7 +20,7 @@ import { joiStringMap, joiVariables, } from "../config/common" -import { InternalError, RuntimeError, GardenBaseError, GardenError, isGardenError } from "../exceptions" +import { RuntimeError, GardenBaseError, GardenError, InternalError, toGardenError } from "../exceptions" import { Garden } from "../garden" import { Log } from "../logger/log-entry" import { LoggerType, LoggerBase, LoggerConfigBase, eventLogLevel, LogLevel } from "../logger/logger" @@ -1079,7 +1079,7 @@ export async function handleProcessResults( if (!success) { const wrappedErrors: GardenError[] = flatMap(failed, (f) => { - return f && f.error && isGardenError(f.error) ? [f.error as GardenError] : [] + return f && f.error ? [toGardenError(f.error)] : [] }) const error = new RuntimeError({ diff --git a/core/src/commands/custom.ts b/core/src/commands/custom.ts index 8f31f41e0c..d02e0c4978 100644 --- a/core/src/commands/custom.ts +++ b/core/src/commands/custom.ts @@ -24,7 +24,7 @@ import { import { joi } from "../config/common" import { CustomCommandContext } from "../config/template-contexts/custom-command" import { validateWithPath } from "../config/validation" -import { ConfigurationError, GardenBaseError, InternalError, RuntimeError, toGardenError } from "../exceptions" +import { ConfigurationError, GardenBaseError, RuntimeError, InternalError, toGardenError } from "../exceptions" import { resolveTemplateStrings } from "../template-string/template-string" import { listDirectory, isConfigFilename } from "../util/fs" import { Command, CommandParams, CommandResult, PrintHeaderParams } from "./base" diff --git a/core/src/commands/login.ts b/core/src/commands/login.ts index c8d34a9a10..5ef1681e11 100644 --- a/core/src/commands/login.ts +++ b/core/src/commands/login.ts @@ -11,7 +11,7 @@ import { printHeader } from "../logger/util" import dedent = require("dedent") import { AuthTokenResponse, CloudApi, getGardenCloudDomain } from "../cloud/api" import { Log } from "../logger/log-entry" -import { ConfigurationError, InternalError, TimeoutError } from "../exceptions" +import { ConfigurationError, TimeoutError, InternalError } from "../exceptions" import { AuthRedirectServer } from "../cloud/auth" import { EventBus } from "../events/events" import { getCloudDistributionName } from "../util/util" diff --git a/core/src/commands/version.ts b/core/src/commands/version.ts index 6cc4ce5578..1263f55367 100644 --- a/core/src/commands/version.ts +++ b/core/src/commands/version.ts @@ -7,7 +7,7 @@ */ import { getPackageVersion } from "../util/util" -import { Command, CommandResult } from "./base" +import { Command, CommandParams, CommandResult } from "./base" interface VersionCommandResult { version: string @@ -19,10 +19,15 @@ export class VersionCommand extends Command { help = "Shows the current garden version." override noProject = true - async action({ log }): Promise> { + override async action(params: CommandParams): Promise> { + const { log, opts } = params const version = getPackageVersion() log.info(`garden version: ${version}`) + if (opts.env === "test-command-error") { + throw new Error("Test") + } + return { result: { version }, } diff --git a/core/src/config-store/base.ts b/core/src/config-store/base.ts index 9439437262..a386d5bc76 100644 --- a/core/src/config-store/base.ts +++ b/core/src/config-store/base.ts @@ -9,9 +9,9 @@ import { ensureFile, readFile } from "fs-extra" import { z, ZodType } from "zod" import { lock } from "proper-lockfile" -import { InternalError } from "../exceptions" import { dump } from "js-yaml" import writeFileAtomic from "write-file-atomic" +import { InternalError } from "../exceptions" // Just a shorthand to make the code below a little more compact type I> = z.infer diff --git a/core/src/docs/template-strings.ts b/core/src/docs/template-strings.ts index a6f1d89921..6e3472a250 100644 --- a/core/src/docs/template-strings.ts +++ b/core/src/docs/template-strings.ts @@ -21,10 +21,10 @@ import { ModuleConfigContext, OutputConfigContext } from "../config/template-con import { WorkflowStepConfigContext } from "../config/template-contexts/workflow" import { getHelperFunctions } from "../template-string/functions" import { isEqual, kebabCase, sortBy } from "lodash" -import { InternalError } from "../exceptions" import { CustomCommandContext } from "../config/template-contexts/custom-command" import Joi from "@hapi/joi" import { ActionConfigContext, ActionSpecContext } from "../config/template-contexts/actions" +import { InternalError } from "../exceptions" interface ContextSpec { schema: Joi.ObjectSchema diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index afa5c97f2b..46e5955e02 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -9,7 +9,10 @@ import { isEmpty, isString } from "lodash" import { stringify } from "yaml" import { withoutInternalFields, sanitizeValue } from "./util/logging" -import { testFlags } from "./util/util" +import { getGitHubIssueLink, testFlags } from "./util/util" +import dedent from "dedent" +import chalk from "chalk" +import stripAnsi from "strip-ansi" export interface GardenError extends Error { type: string @@ -80,6 +83,16 @@ export abstract class GardenBaseError extends Error impl } } + toJSON() { + return { + type: this.type, + message: this.message, + stack: this.stack, + detail: this.detail, + wrappedErrors: this.wrappedErrors, + } + } + toSanitizedValue() { return { type: this.type, @@ -142,11 +155,37 @@ export class RuntimeError extends GardenBaseError { type = "runtime" } +/** + * Throw this error only when this error condition is definitely a Garden bug. + * + * Examples where throwing this error is appropriate: + * - A Javascript TypeError has occurred, e.g. reading property on undefined. + * - "This should not happen" kind of situations, e.g. internal data structures are in an invalid state. + * - An unhandled exception has been thrown by a library. If you don't know what to do with this exception and it is most likely not due to user error, wrap it with "InternalError". + * + * In case the network is involved, we should *not* use the "InternalError", because that's usually a situation that the user needs to resolve. + */ export class InternalError extends GardenBaseError { - type = "internal" + type = "unexpected-crash" + + // not using object destructuring here on purpose, because errors are of type any and then the error might be passed as the params object accidentally. + static wrapError(error: Error, detail?: unknown, prefix?: string): InternalError { + let message: string + let stack: string | undefined + + if (error instanceof Error) { + message = error.message + stack = error.stack + } else if (isString(error)) { + message = error + } else { + message = error["message"] + stack = error["stack"] + } - constructor({ message, detail }: { message: string; detail: any }) { - super({ message: message + "\nThis is a bug. Please report it!", detail }) + message = stripAnsi(message) + + return new InternalError({ message: prefix ? `${stripAnsi(prefix)}: ${message}` : message, stack, detail }) } } @@ -174,36 +213,47 @@ export class TemplateStringError extends GardenBaseError { type = "template-string" } -export class WrappedNativeError extends GardenBaseError { - type = "wrapped-native-error" - - constructor(error: Error) { - super({ message: error.message, stack: error.stack }) - } -} - -export function toGardenError(err: Error | GardenBaseError | string): GardenBaseError { +export function toGardenError(err: Error | GardenBaseError | string | any): GardenBaseError { if (err instanceof GardenBaseError) { return err - } else if (err instanceof Error) { - const wrappedError = new WrappedNativeError(err) - const out = new RuntimeError({ message: err.message, wrappedErrors: [wrappedError] }) - out.stack = err.stack - return out - } else if (isString(err)) { - return new RuntimeError({ message: err }) } else { - const msg = err["message"] - return new RuntimeError({ message: msg }) + return InternalError.wrapError(err) } } -function filterErrorDetail(detail: any) { +export function filterErrorDetail(detail: any) { return withoutInternalFields(sanitizeValue(detail)) } +export function explainGardenError(error: GardenError, context?: string) { + let errorMessage = error.message.trim() + + // If this is an unexpected error, we want to output more details by default and provide some guidance for the user. + if (error instanceof InternalError) { + let bugReportInformation = formatGardenErrorWithDetail(error) + + if (context) { + bugReportInformation = `${stripAnsi(context)}\n${bugReportInformation}` + } + + return chalk.red(dedent` + ${chalk.bold("Encountered an unexpected Garden error. We are sorry for this. This is likely a bug šŸ‚")} + + You can help by reporting this on GitHub: ${getGitHubIssueLink(`Unexpected Crash: ${errorMessage}`, "bug")} + + Please attach the following information to the bug report after making sure that the error message does not contain sensitive information: + + ${chalk.gray(bugReportInformation)} + `) + } + + // In case this is another Garden error, the error message is already designed to be digestable as-is for the user. + return chalk.red(error.message) +} + export function formatGardenErrorWithDetail(error: GardenError) { const { detail, message, stack } = error + let out = stack || message || "" // We sanitize and recursively filter out internal fields (i.e. having names starting with _). diff --git a/core/src/garden.ts b/core/src/garden.ts index 595a9f21c8..678d2bee69 100644 --- a/core/src/garden.ts +++ b/core/src/garden.ts @@ -40,7 +40,14 @@ import { getCloudDistributionName, getCloudLogSectionName, } from "./util/util" -import { ConfigurationError, isGardenError, GardenError, InternalError, PluginError, RuntimeError } from "./exceptions" +import { + ConfigurationError, + isGardenError, + GardenError, + PluginError, + RuntimeError, + InternalError, +} from "./exceptions" import { VcsHandler, ModuleVersion, getModuleVersionString, VcsInfo } from "./vcs/vcs" import { GitHandler } from "./vcs/git" import { BuildStaging } from "./build-staging/build-staging" diff --git a/core/src/graph/actions.ts b/core/src/graph/actions.ts index 7d8db47c36..3fad9fd38f 100644 --- a/core/src/graph/actions.ts +++ b/core/src/graph/actions.ts @@ -38,7 +38,7 @@ import { ActionReference, describeSchema, JoiDescription, parseActionReference } import type { GroupConfig } from "../config/group" import { ActionConfigContext } from "../config/template-contexts/actions" import { validateWithPath } from "../config/validation" -import { ConfigurationError, InternalError, PluginError, ValidationError } from "../exceptions" +import { ConfigurationError, PluginError, InternalError, ValidationError } from "../exceptions" import { overrideVariables, type Garden } from "../garden" import type { Log } from "../logger/log-entry" import type { ActionTypeDefinition } from "../plugin/action-types" diff --git a/core/src/graph/config-graph.ts b/core/src/graph/config-graph.ts index 4c66b90e8b..83435cfb83 100644 --- a/core/src/graph/config-graph.ts +++ b/core/src/graph/config-graph.ts @@ -55,6 +55,7 @@ interface GetActionsParams extends GetActionOpts { excludeNames?: string[] // Glob patterns to exclude. An action is returned if its name matches none of these. } +// TODO: There are two different GraphError classes export class GraphError extends GardenBaseError { type = "graph" } diff --git a/core/src/graph/nodes.ts b/core/src/graph/nodes.ts index f9e2fd2970..f40971891d 100644 --- a/core/src/graph/nodes.ts +++ b/core/src/graph/nodes.ts @@ -7,7 +7,7 @@ */ import { Task, ValidResultType } from "../tasks/base" -import { GardenBaseError, InternalError, isGardenError } from "../exceptions" +import { GardenBaseError, InternalError, toGardenError } from "../exceptions" import { GraphResult, GraphResultFromTask, GraphResults } from "./results" import type { GraphSolver } from "./solver" import { ValuesType } from "utility-types" @@ -345,12 +345,14 @@ export interface GraphNodeErrorDetail extends GraphResult { export interface GraphNodeErrorParams extends GraphNodeErrorDetail {} export class GraphNodeError extends GardenBaseError { + // TODO: type graph is already taken by the (two) GraphError(s) type = "graph" constructor(params: GraphNodeErrorParams) { const { node, failedDependency, error } = params let message = "" + let stack: string | undefined if (failedDependency) { message = `${node.describe()} aborted because a dependency could not be completed:` @@ -371,15 +373,17 @@ export class GraphNodeError extends GardenBaseError { } } else if (result?.error) { message += chalk.red.bold(`\nā†³ ${nextDep.describe()} [FAILED] - ${result.error.message}`) + stack = result.error.stack nextDep = null } } } else { message = `${node.describe()} failed: ${error}` + stack = error?.stack } - const wrappedErrors = isGardenError(error) ? [error] : [] - super({ message, detail: params, wrappedErrors, context: { taskType: node.task.type } }) + const wrappedErrors = [toGardenError(error)] + super({ message, stack, detail: params, wrappedErrors, context: { taskType: node.task.type } }) } aborted() { diff --git a/core/src/graph/results.ts b/core/src/graph/results.ts index 70c2f5ff17..7952f7760f 100644 --- a/core/src/graph/results.ts +++ b/core/src/graph/results.ts @@ -7,10 +7,10 @@ */ import { BaseTask, Task, ValidResultType } from "../tasks/base" -import { InternalError } from "../exceptions" import { fromPairs, omit, pick } from "lodash" import { toGraphResultEventPayload } from "../events/events" import CircularJSON from "circular-json" +import { GardenBaseError, GardenErrorParams, InternalError } from "../exceptions" export interface TaskEventBase { type: string @@ -258,9 +258,23 @@ function filterErrorForExport(error: any) { } }) - return { + // we can't just return a plain object here, because then it wouldn't be + // recognizable anymore as a GardenError further down the line and handled as an unexpected crash + return new FilteredError({ ...pick(error, "message", "type", "stack"), detail: filteredDetail, wrappedErrors: filteredWrappedErrors, + }) +} + +interface FilteredErrorParams extends GardenErrorParams { + readonly type: string +} +class FilteredError extends GardenBaseError { + type: string + + constructor(params: FilteredErrorParams) { + super(params) + this.type = params.type } } diff --git a/core/src/graph/solver.ts b/core/src/graph/solver.ts index ac030d773a..085179c317 100644 --- a/core/src/graph/solver.ts +++ b/core/src/graph/solver.ts @@ -8,7 +8,7 @@ import type { BaseTask, Task, ValidResultType } from "../tasks/base" import type { Log } from "../logger/log-entry" -import { GardenBaseError, GardenError, isGardenError, toGardenError } from "../exceptions" +import { GardenBaseError, GardenError, explainGardenError, isGardenError, toGardenError } from "../exceptions" import { uuidv4 } from "../util/random" import { DependencyGraph, metadataForLog } from "./common" import { Profile } from "../util/profiling" @@ -142,6 +142,7 @@ export class GraphSolver extends TypedEventEmitter { error: new GraphError({ message: `Failed to ${result.description}: ${result.error}`, detail: { results }, + wrappedErrors: [toGardenError(result.error)], }), }) return @@ -492,26 +493,29 @@ export class GraphSolver extends TypedEventEmitter { private logTaskError(node: TaskNode, err: Error) { const log = node.task.log - const prefix = `Failed ${node.describe()} ${renderDuration(log.getDuration())}. Here is the output:` + const prefix = `Failed ${node.describe()} ${renderDuration(log.getDuration())}. This is what happened:` this.logError(log, err, prefix) } private logInternalError(node: TaskNode, err: Error) { - const prefix = `An internal error occurred while ${node.describe()}. Here is the output:` + const prefix = `An internal error occurred while ${node.describe()}. This is what happened:` this.logError(node.task.log, err, prefix) } private logError(log: Log, err: Error, errMessagePrefix: string) { const error = toGardenError(err) - const errorMessage = error.message.trim() const msg = renderMessageWithDivider({ prefix: errMessagePrefix, - msg: errorMessage, + msg: explainGardenError(error, errMessagePrefix), isError: true, }) log.error({ msg, error, showDuration: false }) const divider = renderDivider() - log.silly(chalk.gray(`Full error with stack trace:\n${divider}\n${formatGardenErrorWithDetail(error)}\n${divider}`)) + log.silly( + chalk.gray( + `Full error with stack trace and details:\n${divider}\n${formatGardenErrorWithDetail(error)}\n${divider}` + ) + ) } } @@ -548,6 +552,7 @@ interface GraphErrorDetail { results: GraphResults } +// TODO: There are two different GraphError classes class GraphError extends GardenBaseError { type = "graph" } diff --git a/core/src/logger/logger.ts b/core/src/logger/logger.ts index d8fb0ef247..7e910d6848 100644 --- a/core/src/logger/logger.ts +++ b/core/src/logger/logger.ts @@ -8,7 +8,7 @@ import { LogMetadata, LogEntry, CoreLog, CoreLogContext } from "./log-entry" import { Writer } from "./writers/base" -import { CommandError, InternalError, ParameterError } from "../exceptions" +import { CommandError, ParameterError, InternalError } from "../exceptions" import { TerminalWriter } from "./writers/terminal-writer" import { JsonTerminalWriter } from "./writers/json-terminal-writer" import { EventBus } from "../events/events" @@ -51,7 +51,8 @@ export function parseLogLevel(level: string): LogLevel { lvl = LogLevel[level] } if (!getNumericLogLevels().includes(lvl)) { - throw new InternalError({ + // This should be validated on a different level + throw new ParameterError({ message: `Unexpected log level, expected one of ${getLogLevelChoices().join(", ")}, got ${level}`, detail: {}, }) diff --git a/core/src/logger/renderers.ts b/core/src/logger/renderers.ts index de5d696c7e..ce09ba4ce7 100644 --- a/core/src/logger/renderers.ts +++ b/core/src/logger/renderers.ts @@ -17,7 +17,7 @@ import { LogEntry } from "./log-entry" import { JsonLogEntry } from "./writers/json-terminal-writer" import { highlightYaml, safeDumpYaml } from "../util/serialization" import { Logger, logLevelMap, LogLevel } from "./logger" -import { toGardenError, formatGardenErrorWithDetail } from "../exceptions" +import { toGardenError, formatGardenErrorWithDetail, explainGardenError } from "../exceptions" type RenderFn = (entry: LogEntry, logger: Logger) => string @@ -48,7 +48,9 @@ export function renderError(entry: LogEntry): string { let out = "" - if (error) { + if (!msg && error) { + out = explainGardenError(error) + } else if (error) { const noAnsiErr = stripAnsi(error.message || "") const noAnsiMsg = stripAnsi(msg || "") // render error only if message doesn't already contain it diff --git a/core/src/plugins/kubernetes/kubernetes.ts b/core/src/plugins/kubernetes/kubernetes.ts index 41301b5fda..d49dc5e171 100644 --- a/core/src/plugins/kubernetes/kubernetes.ts +++ b/core/src/plugins/kubernetes/kubernetes.ts @@ -77,7 +77,7 @@ export async function configureProvider({ if (config.name !== "local-kubernetes" && !config.deploymentRegistry) { throw new ConfigurationError({ - message: `kubernetes: must specify deploymentRegistry in config`, + message: `Must specify deploymentRegistry in provider configuration`, detail: { config, }, diff --git a/core/src/plugins/kubernetes/util.ts b/core/src/plugins/kubernetes/util.ts index 01f7e2eccb..dd39ab0b3a 100644 --- a/core/src/plugins/kubernetes/util.ts +++ b/core/src/plugins/kubernetes/util.ts @@ -25,7 +25,7 @@ import { KubeApi, KubernetesError } from "./api" import { gardenAnnotationKey, base64, deline, stableStringify, splitLast, truncate } from "../../util/string" import { MAX_CONFIGMAP_DATA_SIZE } from "./constants" import { ContainerEnvVars } from "../container/moduleConfig" -import { ConfigurationError, DeploymentError, InternalError, PluginError } from "../../exceptions" +import { ConfigurationError, DeploymentError, PluginError, InternalError } from "../../exceptions" import { KubernetesProvider, KubernetesPluginContext, KubernetesTargetResourceSpec } from "./config" import { Log } from "../../logger/log-entry" import { PluginContext } from "../../plugin-context" @@ -726,7 +726,10 @@ export async function readTargetResource({ return api.apps.readNamespacedStatefulSet(targetName, namespace) } else { // This should be caught in config/schema validation - throw new InternalError({ message: `Unsupported kind specified in resource/target query`, detail: { query } }) + throw new InternalError({ + message: `Unsupported kind specified in resource/target query`, + detail: { query }, + }) } } diff --git a/core/src/router/base.ts b/core/src/router/base.ts index 47661b6a44..b854afdc1d 100644 --- a/core/src/router/base.ts +++ b/core/src/router/base.ts @@ -31,7 +31,7 @@ import { ActionTypeClasses, GetActionTypeParams, } from "../plugin/action-types" -import { InternalError, ParameterError, PluginError } from "../exceptions" +import { ParameterError, PluginError, InternalError } from "../exceptions" import { validateSchema } from "../config/validation" import { getActionTypeBases, getPluginBases, getPluginDependencies } from "../plugins" import { getNames, MaybeUndefined } from "../util/util" @@ -467,9 +467,9 @@ export abstract class BaseActionRouter extends BaseRouter // This should never happen throw new InternalError({ - message: - `Unable to find any matching configuration when selecting ${actionType}/${String(handlerType)} handler ` + - `(please report this as a bug).`, + message: `Unable to find any matching configuration when selecting ${actionType}/${String( + handlerType + )} handler.`, detail: { handlers, configs }, }) } else { diff --git a/core/src/router/module.ts b/core/src/router/module.ts index 76940b0387..cc7242fa6d 100644 --- a/core/src/router/module.ts +++ b/core/src/router/module.ts @@ -297,9 +297,7 @@ export class ModuleRouter extends BaseRouter { // This should never happen throw new InternalError({ - message: - `Unable to find any matching configuration when selecting ${moduleType}/${handlerType} handler ` + - `(please report this as a bug).`, + message: `Unable to find any matching configuration when selecting ${moduleType}/${handlerType} handler.`, detail: { handlers, configs }, }) } else { diff --git a/core/src/tasks/resolve-action.ts b/core/src/tasks/resolve-action.ts index 9cc83194e0..6c8dfd32ca 100644 --- a/core/src/tasks/resolve-action.ts +++ b/core/src/tasks/resolve-action.ts @@ -243,7 +243,10 @@ export class ResolveActionTask extends BaseActionTask extends Readable { this.lastValues[i] = value if (lastValue !== undefined && this.comparisonFn(lastValue, value) > 0) { - this.emit("error", new InternalError({ message: `Received unordered stream`, detail: { streamIndex: i } })) + this.emit( + "error", + new InternalError({ message: `Received unordered stream`, detail: { streamIndex: i } }) + ) return } diff --git a/core/src/util/util.ts b/core/src/util/util.ts index 0ae8c28807..d40fbfd2e0 100644 --- a/core/src/util/util.ts +++ b/core/src/util/util.ts @@ -21,6 +21,7 @@ import { range, some, trimEnd, + truncate, uniqBy, } from "lodash" import { asyncExitHook, gracefulExit } from "@scg82/exit-hook" @@ -840,10 +841,23 @@ export function userPrompt(params: { } export function getGitHubIssueLink(title: string, type: "bug" | "feature-request") { + try { + title = encodeURIComponent( + truncate(title, { + length: 80, + omission: encodeURIComponent("..."), + }) + ).replaceAll("'", "%27") + } catch (e) { + // encodeURIComponent might throw URIError with malformed unicode strings. + // The title is not that important, we can also leave it empty in that case. + title = "" + } + if (type === "feature-request") { - return `https://github.com/garden-io/garden/issues/new?assignees=&labels=feature+request&template=FEATURE_REQUEST.md&title=%5BFEATURE%5D%3A+${title}` + return `https://github.com/garden-io/garden/issues/new?labels=feature+request&template=FEATURE_REQUEST.md&title=%5BFEATURE%5D%3A+${title}` } else { - return `https://github.com/garden-io/garden/issues/new?assignees=&labels=&template=BUG_REPORT.md&title=${title}` + return `https://github.com/garden-io/garden/issues/new?template=BUG_REPORT.md&title=${title}` } } diff --git a/core/src/watch.ts b/core/src/watch.ts index 5966285840..8368530fc9 100644 --- a/core/src/watch.ts +++ b/core/src/watch.ts @@ -56,12 +56,7 @@ export class Watcher extends EventEmitter2 { try { require("fsevents") } catch (error) { - throw new InternalError({ - message: `Unable to load fsevents module: ${error}`, - detail: { - error, - }, - }) + throw InternalError.wrapError(error, {}, "Unable to load fsevents module") } } diff --git a/core/test/unit/src/commands/version.ts b/core/test/unit/src/commands/version.ts index 9c9297783b..e81be36d69 100644 --- a/core/test/unit/src/commands/version.ts +++ b/core/test/unit/src/commands/version.ts @@ -59,7 +59,7 @@ describe("VersionCommand", () => { const garden = await makeDummyGarden(tmpDir.path, { commandInfo: { name: "version", args: {}, opts: {} } }) const { result } = await command.action({ log: garden.log, - }) + } as any) expect(result).to.eql({ version: getPackageVersion(), }) From 241c6527c08e245a512b86f437d52c00ed7a6448 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 31 Aug 2023 11:59:48 +0200 Subject: [PATCH 02/17] improvement: new CRASH.md issue template --- .github/ISSUE_TEMPLATE/CRASH.md | 41 +++++++++++++++++++++++++++++++++ core/src/exceptions.ts | 5 ++-- core/src/graph/results.ts | 1 + core/src/util/util.ts | 13 +++++++---- 4 files changed, 53 insertions(+), 7 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/CRASH.md diff --git a/.github/ISSUE_TEMPLATE/CRASH.md b/.github/ISSUE_TEMPLATE/CRASH.md new file mode 100644 index 0000000000..c860f1dca7 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/CRASH.md @@ -0,0 +1,41 @@ +--- +name: Crash report +about: Report crashes +title: '' +labels: 'bug,crash' +assignees: '' + +--- + +## Crash report + + + +### Error message + + + + +### What did you do? + + + +### Your environment + + +* **OS:** +* **How I'm running Kubernetes:** +* **Garden version:** + +### Frequency + + + +### Workaround + + + +### Additional context + + + diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index 46e5955e02..bcca68d870 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -166,7 +166,8 @@ export class RuntimeError extends GardenBaseError { * In case the network is involved, we should *not* use the "InternalError", because that's usually a situation that the user needs to resolve. */ export class InternalError extends GardenBaseError { - type = "unexpected-crash" + // we want it to be obvious in amplitude data that this is not a normal error condition + type = "crash" // not using object destructuring here on purpose, because errors are of type any and then the error might be passed as the params object accidentally. static wrapError(error: Error, detail?: unknown, prefix?: string): InternalError { @@ -239,7 +240,7 @@ export function explainGardenError(error: GardenError, context?: string) { return chalk.red(dedent` ${chalk.bold("Encountered an unexpected Garden error. We are sorry for this. This is likely a bug šŸ‚")} - You can help by reporting this on GitHub: ${getGitHubIssueLink(`Unexpected Crash: ${errorMessage}`, "bug")} + You can help by reporting this on GitHub: ${getGitHubIssueLink(`Crash: ${errorMessage}`, "crash")} Please attach the following information to the bug report after making sure that the error message does not contain sensitive information: diff --git a/core/src/graph/results.ts b/core/src/graph/results.ts index 7952f7760f..3227fca423 100644 --- a/core/src/graph/results.ts +++ b/core/src/graph/results.ts @@ -274,6 +274,7 @@ class FilteredError extends GardenBaseError { type: string constructor(params: FilteredErrorParams) { + // TODO: strip ansi on the message? super(params) this.type = params.type } diff --git a/core/src/util/util.ts b/core/src/util/util.ts index d40fbfd2e0..cea4f81595 100644 --- a/core/src/util/util.ts +++ b/core/src/util/util.ts @@ -840,7 +840,7 @@ export function userPrompt(params: { return require("inquirer").prompt(params) } -export function getGitHubIssueLink(title: string, type: "bug" | "feature-request") { +export function getGitHubIssueLink(title: string, type: "bug" | "crash" | "feature-request") { try { title = encodeURIComponent( truncate(title, { @@ -854,10 +854,13 @@ export function getGitHubIssueLink(title: string, type: "bug" | "feature-request title = "" } - if (type === "feature-request") { - return `https://github.com/garden-io/garden/issues/new?labels=feature+request&template=FEATURE_REQUEST.md&title=%5BFEATURE%5D%3A+${title}` - } else { - return `https://github.com/garden-io/garden/issues/new?template=BUG_REPORT.md&title=${title}` + switch (type) { + case "feature-request": + return `https://github.com/garden-io/garden/issues/new?labels=feature+request&template=FEATURE_REQUEST.md&title=%5BFEATURE%5D%3A+${title}` + case "bug": + return `https://github.com/garden-io/garden/issues/new?labels=bug&template=BUG_REPORT.md&title=${title}` + case "crash": + return `https://github.com/garden-io/garden/issues/new?labels=bug,crashtemplate=CRASH.md&title=${title}` } } From d2ca0196dfde90cbf91a02267ed5d9b8de0409cc Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 31 Aug 2023 12:53:01 +0200 Subject: [PATCH 03/17] improvement: make GardenError recognition less error-prone Previously we had an exported interface called GardenError together with an exported abstract base class called GardenBaseError. We had a function called isGardenError that checked for interface conformance, and used it in some parts of the code base, and then we also had parts of the code base that checked for `instanceof GardenBaseError`. This commit eliminates the GardenError interface to eliminate a class of errors, where we fail to recognize the GardenError and treat it as a crash. --- cli/src/cli.ts | 4 +- core/src/actions/base.ts | 4 +- core/src/analytics/analytics.ts | 6 +- core/src/analytics/helpers.ts | 4 +- core/src/cli/cli.ts | 8 +- core/src/cli/helpers.ts | 4 +- core/src/commands/base.ts | 4 +- core/src/commands/create/create-project.ts | 4 +- core/src/commands/custom.ts | 6 +- core/src/exceptions.ts | 118 ++++++++++---------- core/src/garden.ts | 11 +- core/src/graph/config-graph.ts | 7 +- core/src/graph/nodes.ts | 7 +- core/src/graph/results.ts | 22 +--- core/src/graph/solver.ts | 27 ++--- core/src/mutagen.ts | 4 +- core/src/plugin/sdk.ts | 4 +- core/src/plugins/kubernetes/api.ts | 4 +- core/src/plugins/kubernetes/kubernetes.ts | 2 +- core/src/plugins/kubernetes/run.ts | 6 +- core/src/server/server.ts | 4 +- core/src/tasks/base.ts | 4 +- core/src/template-string/template-string.ts | 6 +- core/src/util/ext-tools.ts | 4 +- core/src/util/streams.ts | 5 +- core/src/util/testing.ts | 4 +- core/test/unit/src/exceptions.ts | 4 +- core/test/unit/src/logger/renderers.ts | 7 +- 28 files changed, 129 insertions(+), 165 deletions(-) diff --git a/cli/src/cli.ts b/cli/src/cli.ts index 94de879774..86d4fd9f94 100644 --- a/cli/src/cli.ts +++ b/cli/src/cli.ts @@ -76,9 +76,9 @@ export async function runCli({ function logUnexpectedError(error: Error, context: string) { // NOTE: If this function is called, this is always a bug, because GardenCli.run is designed to return an error code. If it throws an error, something is wrong with our code and we need to fix it. - // This is why we wrap the error with InternalError here, even if it is a GardenBaseError. - // If an error hits this code path, it's definitely a crash and we need to fix that bug. + // This is why we wrap the error with InternalError here, even if it is a GardenError already, because if an error hits this code path, it's definitely a crash and we need to fix that bug. const wrappedError = InternalError.wrapError(error, {}, context) + // eslint-disable-next-line no-console console.log(`${context}: ${explainGardenError(wrappedError, context)}`) } diff --git a/core/src/actions/base.ts b/core/src/actions/base.ts index db16032115..7ee8cc5274 100644 --- a/core/src/actions/base.ts +++ b/core/src/actions/base.ts @@ -37,7 +37,7 @@ import cloneDeep from "fast-copy" import { flatten, fromPairs, isString, memoize, omit, sortBy } from "lodash" import { ActionConfigContext, ActionSpecContext } from "../config/template-contexts/actions" import { relative } from "path" -import { ConfigurationError } from "../exceptions" +import { InternalError } from "../exceptions" import { Action, ActionConfig, @@ -731,7 +731,7 @@ export abstract class ResolvedRuntimeAction< const buildAction = this.getResolvedDependencies().find((a) => a.kind === "Build" && a.name === buildName) if (!buildAction) { - throw new ConfigurationError({ + throw new InternalError({ message: `Could not find build dependency '${buildName}' specified on the build field on ${this.longDescription()}.`, detail: { action: this.key(), buildName }, }) diff --git a/core/src/analytics/analytics.ts b/core/src/analytics/analytics.ts index 3bd5dcfe4c..59c3b76598 100644 --- a/core/src/analytics/analytics.ts +++ b/core/src/analytics/analytics.ts @@ -23,7 +23,7 @@ import { Profile } from "../util/profiling" import { ModuleConfig } from "../config/module" import { UserResult } from "@garden-io/platform-api-types" import { uuidv4 } from "../util/random" -import { GardenBaseError, GardenErrorContext, StackTraceMetadata } from "../exceptions" +import { GardenError, GardenErrorContext, StackTraceMetadata } from "../exceptions" import { ActionConfigMap } from "../actions/types" import { actionKinds } from "../actions/types" import { getResultErrorProperties } from "./helpers" @@ -163,7 +163,7 @@ export interface CommandResultEvent extends EventBase { name: string durationMsec: number result: AnalyticsCommandResult - errors: string[] // list of GardenBaseError types + errors: string[] // list of GardenError types lastError?: AnalyticsGardenError exitCode?: number } @@ -613,7 +613,7 @@ export class AnalyticsHandler { */ trackCommandResult( commandName: string, - errors: GardenBaseError[], + errors: GardenError[], startTime: Date, exitCode?: number, parentSessionId?: string diff --git a/core/src/analytics/helpers.ts b/core/src/analytics/helpers.ts index 4ffbba5f17..fde4e6c481 100644 --- a/core/src/analytics/helpers.ts +++ b/core/src/analytics/helpers.ts @@ -6,7 +6,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { GardenBaseError, GardenError, getStackTraceMetadata } from "../exceptions" +import { GardenError, getStackTraceMetadata } from "../exceptions" import { AnalyticsGardenError, AnalyticsGardenErrorDetail } from "./analytics" function getErrorDetail(error: GardenError): AnalyticsGardenErrorDetail { @@ -39,7 +39,7 @@ function getAnalyticsError(error: GardenError): AnalyticsGardenError { } } -export function getResultErrorProperties(errors: GardenBaseError[]): { +export function getResultErrorProperties(errors: GardenError[]): { errors: string[] lastError?: AnalyticsGardenError } { diff --git a/core/src/cli/cli.ts b/core/src/cli/cli.ts index dc1286fb13..cb76aaedd8 100644 --- a/core/src/cli/cli.ts +++ b/core/src/cli/cli.ts @@ -13,7 +13,7 @@ import { pathExists } from "fs-extra" import { getBuiltinCommands } from "../commands/commands" import { shutdown, getPackageVersion, getCloudDistributionName } from "../util/util" import { Command, CommandResult, CommandGroup, BuiltinArgs } from "../commands/base" -import { PluginError, toGardenError, GardenBaseError, explainGardenError } from "../exceptions" +import { PluginError, toGardenError, GardenError, explainGardenError } from "../exceptions" import { Garden, GardenOpts, makeDummyGarden } from "../garden" import { getRootLogger, getTerminalWriterType, LogLevel, parseLogLevel, RootLogger } from "../logger/logger" import { FileWriter, FileWriterConfig } from "../logger/writers/file-writer" @@ -55,7 +55,7 @@ import { JsonFileWriter } from "../logger/writers/json-file-writer" export interface RunOutput { argv: any code: number - errors: (GardenBaseError | Error)[] + errors: (GardenError | Error)[] result: any // Mainly used for testing consoleOutput?: string @@ -409,7 +409,7 @@ ${renderCommands(commands)} let argv = parseCliArgs({ stringArgs: args, cli: true }) - const errors: (GardenBaseError | Error)[] = [] + const errors: (GardenError | Error)[] = [] const _this = this async function done(abortCode: number, consoleOutput: string, result: any = {}) { @@ -560,7 +560,7 @@ ${renderCommands(commands)} errors.push(...(commandResult.errors || [])) - const gardenErrors: GardenBaseError[] = errors.map(toGardenError) + const gardenErrors: GardenError[] = errors.map(toGardenError) // Flushes the Analytics events queue in case there are some remaining events. await analytics?.flush() diff --git a/core/src/cli/helpers.ts b/core/src/cli/helpers.ts index 8307e6011c..e2541d8af8 100644 --- a/core/src/cli/helpers.ts +++ b/core/src/cli/helpers.ts @@ -19,7 +19,7 @@ import { maxBy, zip } from "lodash" import { Logger } from "../logger/logger" import { ParameterValues, Parameter, Parameters, globalDisplayOptions } from "./params" -import { GardenBaseError, ParameterError, RuntimeError, toGardenError } from "../exceptions" +import { GardenError, ParameterError, RuntimeError, toGardenError } from "../exceptions" import { getPackageVersion, removeSlice } from "../util/util" import { Log } from "../logger/log-entry" import { STATIC_DIR, VERSION_CHECK_URL, gardenEnv, ERROR_LOG_FILENAME } from "../constants" @@ -550,7 +550,7 @@ function renderParameters(params: Parameters, formatName: (name: string, param: } export function renderCommandErrors(logger: Logger, errors: Error[], log?: Log) { - const gardenErrors: GardenBaseError[] = errors.map(toGardenError) + const gardenErrors: GardenError[] = errors.map(toGardenError) const errorLog = log || logger.createLog() diff --git a/core/src/commands/base.ts b/core/src/commands/base.ts index 87d4e83cec..539ef7c797 100644 --- a/core/src/commands/base.ts +++ b/core/src/commands/base.ts @@ -20,7 +20,7 @@ import { joiStringMap, joiVariables, } from "../config/common" -import { RuntimeError, GardenBaseError, GardenError, InternalError, toGardenError } from "../exceptions" +import { RuntimeError, GardenError, InternalError, toGardenError } from "../exceptions" import { Garden } from "../garden" import { Log } from "../logger/log-entry" import { LoggerType, LoggerBase, LoggerConfigBase, eventLogLevel, LogLevel } from "../logger/logger" @@ -64,7 +64,7 @@ export interface CommandConstructor { export interface CommandResult { result?: T - errors?: GardenBaseError[] + errors?: GardenError[] exitCode?: number } diff --git a/core/src/commands/create/create-project.ts b/core/src/commands/create/create-project.ts index 0b9192a03e..a308cd2ed3 100644 --- a/core/src/commands/create/create-project.ts +++ b/core/src/commands/create/create-project.ts @@ -14,7 +14,7 @@ import { printHeader } from "../../logger/util" import { isDirectory } from "../../util/fs" import { loadConfigResources } from "../../config/base" import { resolve, basename, relative, join } from "path" -import { GardenBaseError, ParameterError } from "../../exceptions" +import { GardenError, ParameterError } from "../../exceptions" import { addConfig } from "./helpers" import { wordWrap } from "../../util/string" import { PathParameter, StringParameter, BooleanParameter, StringOption } from "../../cli/params" @@ -60,7 +60,7 @@ interface CreateProjectResult { name: string } -class CreateError extends GardenBaseError { +class CreateError extends GardenError { type: "create" } diff --git a/core/src/commands/custom.ts b/core/src/commands/custom.ts index d02e0c4978..0815d49727 100644 --- a/core/src/commands/custom.ts +++ b/core/src/commands/custom.ts @@ -24,7 +24,7 @@ import { import { joi } from "../config/common" import { CustomCommandContext } from "../config/template-contexts/custom-command" import { validateWithPath } from "../config/validation" -import { ConfigurationError, GardenBaseError, RuntimeError, InternalError, toGardenError } from "../exceptions" +import { ConfigurationError, GardenError, RuntimeError, InternalError, toGardenError } from "../exceptions" import { resolveTemplateStrings } from "../template-string/template-string" import { listDirectory, isConfigFilename } from "../util/fs" import { Command, CommandParams, CommandResult, PrintHeaderParams } from "./base" @@ -66,7 +66,7 @@ interface CustomCommandResult { completedAt: Date command: string[] result: any - errors: (Error | GardenBaseError)[] + errors: (Error | GardenError)[] } } @@ -117,7 +117,7 @@ export class CustomCommandWrapper extends Command { const commandContext = new CustomCommandContext({ ...garden, args, opts, variables, rest }) const result: CustomCommandResult = {} - const errors: GardenBaseError[] = [] + const errors: GardenError[] = [] // Run exec command if (this.spec.exec) { diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index bcca68d870..5d5a858ab3 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -14,19 +14,6 @@ import dedent from "dedent" import chalk from "chalk" import stripAnsi from "strip-ansi" -export interface GardenError extends Error { - type: string - message: string - detail?: D - stack?: string - wrappedErrors?: GardenError[] - context?: GardenErrorContext -} - -export function isGardenError(err: any): err is GardenError { - return "type" in err && "message" in err -} - export type StackTraceMetadata = { functionName: string relativeFileName?: string @@ -49,8 +36,7 @@ export interface GardenErrorParams { export type GardenErrorContext = { taskType?: string } - -export abstract class GardenBaseError extends Error implements GardenError { +export abstract class GardenError extends Error { abstract type: string public override message: string public detail?: D @@ -107,54 +93,94 @@ export abstract class GardenBaseError extends Error impl } } -export class AuthenticationError extends GardenBaseError { +export class AuthenticationError extends GardenError { type = "authentication" } -export class BuildError extends GardenBaseError { +export class BuildError extends GardenError { type = "build" } -export class ConfigurationError extends GardenBaseError { +export class ConfigurationError extends GardenError { type = "configuration" } -export class CommandError extends GardenBaseError { +export class CommandError extends GardenError { type = "command" } -export class FilesystemError extends GardenBaseError { +export class FilesystemError extends GardenError { type = "filesystem" } -export class LocalConfigError extends GardenBaseError { +export class LocalConfigError extends GardenError { type = "local-config" } -export class ValidationError extends GardenBaseError { +export class ValidationError extends GardenError { type = "validation" } -export class PluginError extends GardenBaseError { +export class PluginError extends GardenError { type = "plugin" } -export class ParameterError extends GardenBaseError { +export class ParameterError extends GardenError { type = "parameter" } -export class NotImplementedError extends GardenBaseError { +export class NotImplementedError extends GardenError { type = "not-implemented" } -export class DeploymentError extends GardenBaseError { +export class DeploymentError extends GardenError { type = "deployment" } -export class RuntimeError extends GardenBaseError { +export class RuntimeError extends GardenError { type = "runtime" } +export class GraphError extends GardenError { + type = "graph" +} + +export class TimeoutError extends GardenError { + type = "timeout" +} + +export class OutOfMemoryError extends GardenError { + type = "out-of-memory" +} + +export class NotFoundError extends GardenError { + type = "not-found" +} + +export class WorkflowScriptError extends GardenError { + type = "workflow-script" +} + +export class CloudApiError extends GardenError { + type = "cloud-api" +} + +export class TemplateStringError extends GardenError { + type = "template-string" +} + +interface GenericGardenErrorParams extends GardenErrorParams { + type: string +} +export class GenericGardenError extends GardenError { + type: string + + constructor(params: GenericGardenErrorParams) { + super(params) + this.type = params.type + } +} + /** * Throw this error only when this error condition is definitely a Garden bug. * @@ -165,12 +191,12 @@ export class RuntimeError extends GardenBaseError { * * In case the network is involved, we should *not* use the "InternalError", because that's usually a situation that the user needs to resolve. */ -export class InternalError extends GardenBaseError { +export class InternalError extends GardenError { // we want it to be obvious in amplitude data that this is not a normal error condition type = "crash" // not using object destructuring here on purpose, because errors are of type any and then the error might be passed as the params object accidentally. - static wrapError(error: Error, detail?: unknown, prefix?: string): InternalError { + static wrapError(error: Error | string | any, detail?: unknown, prefix?: string): InternalError { let message: string let stack: string | undefined @@ -190,32 +216,8 @@ export class InternalError extends GardenBaseError { } } -export class TimeoutError extends GardenBaseError { - type = "timeout" -} - -export class OutOfMemoryError extends GardenBaseError { - type = "out-of-memory" -} - -export class NotFoundError extends GardenBaseError { - type = "not-found" -} - -export class WorkflowScriptError extends GardenBaseError { - type = "workflow-script" -} - -export class CloudApiError extends GardenBaseError { - type = "cloud-api" -} - -export class TemplateStringError extends GardenBaseError { - type = "template-string" -} - -export function toGardenError(err: Error | GardenBaseError | string | any): GardenBaseError { - if (err instanceof GardenBaseError) { +export function toGardenError(err: Error | GardenError | string | any): GardenError { + if (err instanceof GardenError) { return err } else { return InternalError.wrapError(err) @@ -226,7 +228,9 @@ export function filterErrorDetail(detail: any) { return withoutInternalFields(sanitizeValue(detail)) } -export function explainGardenError(error: GardenError, context?: string) { +export function explainGardenError(rawError: GardenError | Error | string, context?: string) { + const error = toGardenError(rawError) + let errorMessage = error.message.trim() // If this is an unexpected error, we want to output more details by default and provide some guidance for the user. @@ -249,7 +253,7 @@ export function explainGardenError(error: GardenError, context?: string) { } // In case this is another Garden error, the error message is already designed to be digestable as-is for the user. - return chalk.red(error.message) + return chalk.red(errorMessage) } export function formatGardenErrorWithDetail(error: GardenError) { diff --git a/core/src/garden.ts b/core/src/garden.ts index 678d2bee69..dd12c188d6 100644 --- a/core/src/garden.ts +++ b/core/src/garden.ts @@ -40,14 +40,7 @@ import { getCloudDistributionName, getCloudLogSectionName, } from "./util/util" -import { - ConfigurationError, - isGardenError, - GardenError, - PluginError, - RuntimeError, - InternalError, -} from "./exceptions" +import { ConfigurationError, GardenError, PluginError, RuntimeError, InternalError } from "./exceptions" import { VcsHandler, ModuleVersion, getModuleVersionString, VcsInfo } from "./vcs/vcs" import { GitHandler } from "./vcs/git" import { BuildStaging } from "./build-staging/build-staging" @@ -817,7 +810,7 @@ export class Garden { const failedNames = failed.map((r) => r!.name) const wrappedErrors: GardenError[] = failed.flatMap((f) => { - return f && f.error && isGardenError(f.error) ? [f.error as GardenError] : [] + return f && f.error instanceof GardenError ? [f.error] : [] }) throw new PluginError({ diff --git a/core/src/graph/config-graph.ts b/core/src/graph/config-graph.ts index 83435cfb83..a9ac8c2fec 100644 --- a/core/src/graph/config-graph.ts +++ b/core/src/graph/config-graph.ts @@ -8,7 +8,6 @@ import toposort from "toposort" import { flatten, difference, mapValues, cloneDeep, find } from "lodash" -import { GardenBaseError } from "../exceptions" import { naturalList } from "../util/string" import { Action, ActionDependencyAttributes, ActionKind, Resolved, ResolvedAction } from "../actions/types" import { actionReferenceToString } from "../actions/base" @@ -23,6 +22,7 @@ import { RunAction } from "../actions/run" import { TestAction } from "../actions/test" import { GroupConfig } from "../config/group" import minimatch from "minimatch" +import { GraphError } from "../exceptions" export type DependencyRelationFilterFn = (node: ConfigGraphNode) => boolean @@ -55,11 +55,6 @@ interface GetActionsParams extends GetActionOpts { excludeNames?: string[] // Glob patterns to exclude. An action is returned if its name matches none of these. } -// TODO: There are two different GraphError classes -export class GraphError extends GardenBaseError { - type = "graph" -} - export type PickTypeByKind< K extends ActionKind, B extends BuildAction, diff --git a/core/src/graph/nodes.ts b/core/src/graph/nodes.ts index f40971891d..dd6256e647 100644 --- a/core/src/graph/nodes.ts +++ b/core/src/graph/nodes.ts @@ -7,7 +7,7 @@ */ import { Task, ValidResultType } from "../tasks/base" -import { GardenBaseError, InternalError, toGardenError } from "../exceptions" +import { GraphError, InternalError, toGardenError } from "../exceptions" import { GraphResult, GraphResultFromTask, GraphResults } from "./results" import type { GraphSolver } from "./solver" import { ValuesType } from "utility-types" @@ -344,10 +344,7 @@ export interface GraphNodeErrorDetail extends GraphResult { export interface GraphNodeErrorParams extends GraphNodeErrorDetail {} -export class GraphNodeError extends GardenBaseError { - // TODO: type graph is already taken by the (two) GraphError(s) - type = "graph" - +export class GraphNodeError extends GraphError { constructor(params: GraphNodeErrorParams) { const { node, failedDependency, error } = params diff --git a/core/src/graph/results.ts b/core/src/graph/results.ts index 3227fca423..eeed69bc3e 100644 --- a/core/src/graph/results.ts +++ b/core/src/graph/results.ts @@ -10,7 +10,7 @@ import { BaseTask, Task, ValidResultType } from "../tasks/base" import { fromPairs, omit, pick } from "lodash" import { toGraphResultEventPayload } from "../events/events" import CircularJSON from "circular-json" -import { GardenBaseError, GardenErrorParams, InternalError } from "../exceptions" +import { GardenError, GenericGardenError, InternalError } from "../exceptions" export interface TaskEventBase { type: string @@ -227,7 +227,7 @@ function filterOutputsForExport(outputs: any) { return omit(outputs, "resolvedAction", "executedAction") } -function filterErrorForExport(error: any) { +function filterErrorForExport(error: any): GardenError | null { if (!error) { return null } @@ -258,24 +258,10 @@ function filterErrorForExport(error: any) { } }) - // we can't just return a plain object here, because then it wouldn't be - // recognizable anymore as a GardenError further down the line and handled as an unexpected crash - return new FilteredError({ + // we can't just return a plain object here, because otherwise the error will not be recognizable as a GardenError and that means it's handled as InternalError (crash) + return new GenericGardenError({ ...pick(error, "message", "type", "stack"), detail: filteredDetail, wrappedErrors: filteredWrappedErrors, }) } - -interface FilteredErrorParams extends GardenErrorParams { - readonly type: string -} -class FilteredError extends GardenBaseError { - type: string - - constructor(params: FilteredErrorParams) { - // TODO: strip ansi on the message? - super(params) - this.type = params.type - } -} diff --git a/core/src/graph/solver.ts b/core/src/graph/solver.ts index 085179c317..bef8eb6274 100644 --- a/core/src/graph/solver.ts +++ b/core/src/graph/solver.ts @@ -8,7 +8,7 @@ import type { BaseTask, Task, ValidResultType } from "../tasks/base" import type { Log } from "../logger/log-entry" -import { GardenBaseError, GardenError, explainGardenError, isGardenError, toGardenError } from "../exceptions" +import { GardenError, GraphError, explainGardenError, toGardenError } from "../exceptions" import { uuidv4 } from "../util/random" import { DependencyGraph, metadataForLog } from "./common" import { Profile } from "../util/profiling" @@ -46,7 +46,7 @@ export interface SolveParams extends SolveOpts { } export interface SolveResult { - error: GraphError | null + error: GraphResultError | null results: GraphResults } @@ -139,7 +139,7 @@ export class GraphSolver extends TypedEventEmitter { if (throwOnError && result.error) { cleanup({ - error: new GraphError({ + error: new GraphResultError({ message: `Failed to ${result.description}: ${result.error}`, detail: { results }, wrappedErrors: [toGardenError(result.error)], @@ -158,7 +158,7 @@ export class GraphSolver extends TypedEventEmitter { } // All requested results have been filled (i.e. none are null) so we're done. - let error: GraphError | null = null + let error: GraphResultError | null = null const failed = Object.entries(results.getMap()).filter(([_, r]) => !!r?.error || !!r?.aborted) @@ -173,14 +173,14 @@ export class GraphSolver extends TypedEventEmitter { continue } - if (isGardenError(r.error)) { + if (r.error instanceof GardenError) { wrappedErrors.push(r.error) } msg += `\n ā†³ ${r.description}: ${r?.error ? r.error.message : "[ABORTED]"}` } - error = new GraphError({ message: msg, detail: { results }, wrappedErrors }) + error = new GraphResultError({ message: msg, detail: { results }, wrappedErrors }) } cleanup({ error: null }) @@ -194,7 +194,7 @@ export class GraphSolver extends TypedEventEmitter { resolve({ error, results }) } - function cleanup({ error }: { error: GraphError | null }) { + function cleanup({ error }: { error: GraphResultError | null }) { // TODO: abort remaining pending tasks? aborted = true delete _this.requestedTasks[batchId] @@ -525,7 +525,7 @@ interface TaskStartEvent extends TaskEventBase { interface SolverEvents { abort: { - error: GraphError | null + error: GraphResultError | null } loop: {} process: { @@ -548,15 +548,8 @@ interface WrappedNodes { [key: string]: TaskNode } -interface GraphErrorDetail { +interface GraphResultErrorDetail { results: GraphResults } -// TODO: There are two different GraphError classes -class GraphError extends GardenBaseError { - type = "graph" -} - -// class CircularDependenciesError extends GardenBaseError { -// type = "circular-dependencies" -// } +class GraphResultError extends GraphError {} diff --git a/core/src/mutagen.ts b/core/src/mutagen.ts index 1cff708db1..d63bd0ebec 100644 --- a/core/src/mutagen.ts +++ b/core/src/mutagen.ts @@ -18,7 +18,7 @@ import { join } from "path" import respawn from "respawn" import split2 from "split2" import { GARDEN_GLOBAL_PATH, MUTAGEN_DIR_NAME } from "./constants" -import { GardenBaseError } from "./exceptions" +import { GardenError } from "./exceptions" import pMemoize from "./lib/p-memoize" import { Log } from "./logger/log-entry" import { PluginContext } from "./plugin-context" @@ -128,7 +128,7 @@ interface ActiveSync { mutagenParameters: string[] } -export class MutagenError extends GardenBaseError { +export class MutagenError extends GardenError { type = "mutagen" } diff --git a/core/src/plugin/sdk.ts b/core/src/plugin/sdk.ts index f3bafceb79..2da2f738d5 100644 --- a/core/src/plugin/sdk.ts +++ b/core/src/plugin/sdk.ts @@ -15,7 +15,7 @@ import type { TestAction, TestActionConfig } from "../actions/test" import { joi, zodObjectToJoi } from "../config/common" import { BaseProviderConfig, baseProviderConfigSchemaZod } from "../config/provider" import { s } from "../config/zod" -import { GardenBaseError, ValidationError } from "../exceptions" +import { GardenError, ValidationError } from "../exceptions" import type { ActionKind, ActionTypeDefinition, @@ -52,7 +52,7 @@ type GardenSdkPluginSpec = Pick< "name" | "docs" | "dependencies" | "createModuleTypes" | "extendModuleTypes" > -class SdkError extends GardenBaseError { +class SdkError extends GardenError { type = "sdk" } diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index d565193c0c..d67aa0d378 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/core/src/plugins/kubernetes/api.ts @@ -34,7 +34,7 @@ import WebSocket from "isomorphic-ws" import pRetry from "p-retry" import { Omit, StringCollector } from "../../util/util" import { flatten, isObject, isPlainObject, keyBy, omitBy } from "lodash" -import { ConfigurationError, GardenBaseError, RuntimeError } from "../../exceptions" +import { ConfigurationError, GardenError, RuntimeError } from "../../exceptions" import { BaseResource, KubernetesList, @@ -133,7 +133,7 @@ const crudMap = { type CrudMap = typeof crudMap type CrudMapTypes = { [T in keyof CrudMap]: CrudMap[T]["cls"] } -export class KubernetesError extends GardenBaseError { +export class KubernetesError extends GardenError { type = "kubernetes" statusCode?: number diff --git a/core/src/plugins/kubernetes/kubernetes.ts b/core/src/plugins/kubernetes/kubernetes.ts index d49dc5e171..41301b5fda 100644 --- a/core/src/plugins/kubernetes/kubernetes.ts +++ b/core/src/plugins/kubernetes/kubernetes.ts @@ -77,7 +77,7 @@ export async function configureProvider({ if (config.name !== "local-kubernetes" && !config.deploymentRegistry) { throw new ConfigurationError({ - message: `Must specify deploymentRegistry in provider configuration`, + message: `kubernetes: must specify deploymentRegistry in config`, detail: { config, }, diff --git a/core/src/plugins/kubernetes/run.ts b/core/src/plugins/kubernetes/run.ts index 13f791815a..b23277ee8b 100644 --- a/core/src/plugins/kubernetes/run.ts +++ b/core/src/plugins/kubernetes/run.ts @@ -14,7 +14,7 @@ import { Log } from "../../logger/log-entry" import { CoreV1Event } from "@kubernetes/client-node" import { PluginError, - GardenBaseError, + GardenError, TimeoutError, RuntimeError, ConfigurationError, @@ -735,7 +735,7 @@ type RunParams = StartParams & { throwOnExitCode?: boolean } -class PodRunnerError extends GardenBaseError { +class PodRunnerError extends GardenError { type = "pod-runner" } @@ -1222,7 +1222,7 @@ export class PodRunner extends PodRunnerParams { // Some types and predicates to identify known errors const knownErrorTypes = ["out-of-memory", "not-found", "timeout", "pod-runner", "kubernetes"] as const type KnownErrorType = (typeof knownErrorTypes)[number] - // A known error is always an instance of a subclass of GardenBaseError + // A known error is always an instance of a subclass of GardenError type KnownError = Error & { message: string type: KnownErrorType diff --git a/core/src/server/server.ts b/core/src/server/server.ts index 825823ffff..27549abb5a 100644 --- a/core/src/server/server.ts +++ b/core/src/server/server.ts @@ -22,7 +22,7 @@ import { BaseServerRequest, resolveRequest, serverRequestSchema, shellCommandPar import { DEFAULT_GARDEN_DIR_NAME, gardenEnv } from "../constants" import { Log } from "../logger/log-entry" import { Command, CommandResult } from "../commands/base" -import { toGardenError, GardenError, GardenBaseError } from "../exceptions" +import { toGardenError, GardenError } from "../exceptions" import { EventName, Events, EventBus, shouldStreamWsEvent } from "../events/events" import type { ValueOf } from "../util/util" import { joi } from "../config/common" @@ -832,7 +832,7 @@ export class GardenServer extends EventEmitter { }) let graph: ConfigGraph | undefined - let errors: GardenBaseError[] = [] + let errors: GardenError[] = [] try { graph = await garden.getConfigGraph({ log, emit: true }) diff --git a/core/src/tasks/base.ts b/core/src/tasks/base.ts index 5b6ba30419..6d1b6c01e7 100644 --- a/core/src/tasks/base.ts +++ b/core/src/tasks/base.ts @@ -12,9 +12,9 @@ import { Garden } from "../garden" import { ActionLog, createActionLog, Log } from "../logger/log-entry" import { Profile } from "../util/profiling" import { type Action, type ActionState, type Executed, type Resolved } from "../actions/types" -import { ConfigGraph, GraphError } from "../graph/config-graph" +import { ConfigGraph } from "../graph/config-graph" import type { ActionReference } from "../config/common" -import { InternalError, RuntimeError } from "../exceptions" +import { GraphError, InternalError, RuntimeError } from "../exceptions" import type { DeleteDeployTask } from "./delete-deploy" import type { BuildTask } from "./build" import type { DeployTask } from "./deploy" diff --git a/core/src/template-string/template-string.ts b/core/src/template-string/template-string.ts index 00c848d03a..d764a10587 100644 --- a/core/src/template-string/template-string.ts +++ b/core/src/template-string/template-string.ts @@ -7,7 +7,7 @@ */ import chalk from "chalk" -import { ConfigurationError, GardenBaseError, TemplateStringError } from "../exceptions" +import { ConfigurationError, GardenError, TemplateStringError } from "../exceptions" import { ConfigContext, ContextKeySegment, @@ -48,11 +48,11 @@ const missingKeyExceptionType = "template-string-missing-key" const passthroughExceptionType = "template-string-passthrough" const escapePrefix = "$${" -export class TemplateStringMissingKeyException extends GardenBaseError { +export class TemplateStringMissingKeyException extends GardenError { type = missingKeyExceptionType } -export class TemplateStringPassthroughException extends GardenBaseError { +export class TemplateStringPassthroughException extends GardenError { type = passthroughExceptionType } diff --git a/core/src/util/ext-tools.ts b/core/src/util/ext-tools.ts index b7a38fad26..5336fb6267 100644 --- a/core/src/util/ext-tools.ts +++ b/core/src/util/ext-tools.ts @@ -7,7 +7,7 @@ */ import { pathExists, createWriteStream, ensureDir, chmod, remove, move, createReadStream } from "fs-extra" -import { ConfigurationError, ParameterError, GardenBaseError } from "../exceptions" +import { ConfigurationError, ParameterError, GardenError } from "../exceptions" import { join, dirname, basename, posix } from "path" import { hashString, exec, getPlatform, getArchitecture, isDarwinARM } from "./util" import tar from "tar" @@ -29,7 +29,7 @@ import { streamLogs, waitForProcess } from "./process" const toolsPath = join(GARDEN_GLOBAL_PATH, "tools") const lock = new AsyncLock() -export class DownloadError extends GardenBaseError { +export class DownloadError extends GardenError { type = "download" } diff --git a/core/src/util/streams.ts b/core/src/util/streams.ts index e9bfd5ea67..81ea4cfe93 100644 --- a/core/src/util/streams.ts +++ b/core/src/util/streams.ts @@ -54,10 +54,7 @@ export class SortedStreamIntersection extends Readable { this.lastValues[i] = value if (lastValue !== undefined && this.comparisonFn(lastValue, value) > 0) { - this.emit( - "error", - new InternalError({ message: `Received unordered stream`, detail: { streamIndex: i } }) - ) + this.emit("error", new InternalError({ message: `Received unordered stream`, detail: { streamIndex: i } })) return } diff --git a/core/src/util/testing.ts b/core/src/util/testing.ts index 0a7307c75c..68a9347687 100644 --- a/core/src/util/testing.ts +++ b/core/src/util/testing.ts @@ -16,7 +16,7 @@ import { WorkflowConfig } from "../config/workflow" import { Log, LogEntry } from "../logger/log-entry" import { GardenModule } from "../types/module" import { findByName, getNames } from "./util" -import { GardenBaseError, GardenError, InternalError } from "../exceptions" +import { GardenError, InternalError } from "../exceptions" import { EventBus, EventName, Events } from "../events/events" import { dedent } from "./string" import pathIsInside from "path-is-inside" @@ -37,7 +37,7 @@ import { mkdirp, remove } from "fs-extra" import { GlobalConfigStore } from "../config-store/global" import { isPromise } from "./objects" -export class TestError extends GardenBaseError { +export class TestError extends GardenError { type = "_test" } diff --git a/core/test/unit/src/exceptions.ts b/core/test/unit/src/exceptions.ts index c19ac007ea..08d85501be 100644 --- a/core/test/unit/src/exceptions.ts +++ b/core/test/unit/src/exceptions.ts @@ -9,7 +9,7 @@ import { expect } from "chai" import { ConfigurationError, - GardenBaseError, + GardenError, RuntimeError, StackTraceMetadata, getStackTraceMetadata, @@ -28,7 +28,7 @@ describe("GardenError", () => { } it("should return stack trace metadata", async () => { - let error: GardenBaseError + let error: GardenError try { throw new RuntimeError({ message: "test exception" }) diff --git a/core/test/unit/src/logger/renderers.ts b/core/test/unit/src/logger/renderers.ts index 8891fbcd09..09e3774ec3 100644 --- a/core/test/unit/src/logger/renderers.ts +++ b/core/test/unit/src/logger/renderers.ts @@ -22,7 +22,7 @@ import { renderSection, warningStyle, } from "../../../../src/logger/renderers" -import { GardenError } from "../../../../src/exceptions" +import { GenericGardenError } from "../../../../src/exceptions" import { createActionLog, TaskMetadata } from "../../../../src/logger/log-entry" import logSymbols = require("log-symbols") @@ -55,15 +55,14 @@ describe("renderers", () => { }) describe("renderError", () => { it("should render error object if present", () => { - const error: GardenError<{ foo: string; _internal: string }> = { - name: "test", + const error = new GenericGardenError({ message: "hello error", type: "a", detail: { foo: "bar", _internal: "no show", }, - } + }) const log = logger.createLog().info({ msg: "foo", error }) const rendered = renderError(log.entries[0]) expect(rendered).to.include("hello error") From b7a36aebf2f01950ee46ed382447334a91e13b23 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 31 Aug 2023 15:07:15 +0200 Subject: [PATCH 04/17] fix: tests --- core/test/unit/src/cli/helpers.ts | 6 +++--- core/test/unit/src/commands/version.ts | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/test/unit/src/cli/helpers.ts b/core/test/unit/src/cli/helpers.ts index 0cae9e38f7..f4c8b4fbc2 100644 --- a/core/test/unit/src/cli/helpers.ts +++ b/core/test/unit/src/cli/helpers.ts @@ -54,13 +54,13 @@ describe("parseLogLevel", () => { expect(parsed).to.eql([0, 1, 2, 3, 4, 5, 0, 1, 2, 3, 4, 5]) }) it("should throw if level is not valid", async () => { - await expectError(() => parseLogLevel("banana"), "internal") + await expectError(() => parseLogLevel("banana"), "parameter") }) it("should throw if level is not valid", async () => { - await expectError(() => parseLogLevel("-1"), "internal") + await expectError(() => parseLogLevel("-1"), "parameter") }) it("should throw if level is not valid", async () => { - await expectError(() => parseLogLevel(""), "internal") + await expectError(() => parseLogLevel(""), "parameter") }) }) diff --git a/core/test/unit/src/commands/version.ts b/core/test/unit/src/commands/version.ts index e81be36d69..835999a10f 100644 --- a/core/test/unit/src/commands/version.ts +++ b/core/test/unit/src/commands/version.ts @@ -59,6 +59,7 @@ describe("VersionCommand", () => { const garden = await makeDummyGarden(tmpDir.path, { commandInfo: { name: "version", args: {}, opts: {} } }) const { result } = await command.action({ log: garden.log, + opts: {} } as any) expect(result).to.eql({ version: getPackageVersion(), From 8eb68a049edb3ddcae0a9482d8c147b28482ae9b Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 31 Aug 2023 16:02:35 +0200 Subject: [PATCH 05/17] fix: don't throw on custom command validation fail --- core/src/cli/cli.ts | 16 ++++++++++------ core/test/unit/src/cli/cli.ts | 19 +++++++++---------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/core/src/cli/cli.ts b/core/src/cli/cli.ts index cb76aaedd8..70dcaace1b 100644 --- a/core/src/cli/cli.ts +++ b/core/src/cli/cli.ts @@ -467,13 +467,17 @@ ${renderCommands(commands)} // Load custom commands from current project (if applicable) and see if any match the arguments if (!command) { - projectConfig = await this.getProjectConfig(log, workingDir) + try { + projectConfig = await this.getProjectConfig(log, workingDir) - if (projectConfig) { - const customCommands = await this.getCustomCommands(log, workingDir) - const picked = pickCommand(customCommands, argv._) - command = picked.command - matchedPath = picked.matchedPath + if (projectConfig) { + const customCommands = await this.getCustomCommands(log, workingDir) + const picked = pickCommand(customCommands, argv._) + command = picked.command + matchedPath = picked.matchedPath + } + } catch (error) { + return done(1, explainGardenError(error, "Failed to get custom commands")) } } diff --git a/core/test/unit/src/cli/cli.ts b/core/test/unit/src/cli/cli.ts index 8b00eb05d5..b699bba74a 100644 --- a/core/test/unit/src/cli/cli.ts +++ b/core/test/unit/src/cli/cli.ts @@ -22,7 +22,7 @@ import { getRootLogger, RootLogger } from "../../../../src/logger/logger" import { load } from "js-yaml" import { startServer } from "../../../../src/server/server" import { envSupportsEmoji } from "../../../../src/logger/util" -import { expectError } from "../../../../src/util/testing" +import { expectError, expectFuzzyMatch } from "../../../../src/util/testing" import { GlobalConfigStore } from "../../../../src/config-store/global" import tmp from "tmp-promise" import { CloudCommand } from "../../../../src/commands/cloud/cloud" @@ -162,15 +162,14 @@ describe("cli", () => { }) it("errors if a Command resource is invalid", async () => { - return expectError( - () => - cli.run({ - args: ["echo", "foo"], - exitOnError: false, - cwd: getDataDir("test-projects", "custom-commands-invalid"), - }), - { contains: "Error validating custom Command 'invalid'" } - ) + // cli.run should never throw ā€“ if it throws, it's a bug + const res = await cli.run({ + args: ["echo", "foo"], + exitOnError: false, + cwd: getDataDir("test-projects", "custom-commands-invalid"), + }) + expect(res.code).to.not.equal(0) + expectFuzzyMatch(res.consoleOutput!, "Error validating custom Command 'invalid'") }) it("exits with code from exec command if it fails", async () => { From 8a5d227a17a672fd731ff6f7c82eb53d4146f6d8 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 31 Aug 2023 19:22:52 +0200 Subject: [PATCH 06/17] test: implement tests for command level errors --- core/src/commands/version.ts | 6 +- core/src/util/testing.ts | 24 ++++++++ core/src/util/util.ts | 2 +- core/test/unit/src/cli/cli.ts | 80 +++++++++++++++++++++++++- core/test/unit/src/commands/version.ts | 1 - 5 files changed, 104 insertions(+), 9 deletions(-) diff --git a/core/src/commands/version.ts b/core/src/commands/version.ts index 1263f55367..5f3891915e 100644 --- a/core/src/commands/version.ts +++ b/core/src/commands/version.ts @@ -20,14 +20,10 @@ export class VersionCommand extends Command { override noProject = true override async action(params: CommandParams): Promise> { - const { log, opts } = params + const { log } = params const version = getPackageVersion() log.info(`garden version: ${version}`) - if (opts.env === "test-command-error") { - throw new Error("Test") - } - return { result: { version }, } diff --git a/core/src/util/testing.ts b/core/src/util/testing.ts index 68a9347687..2476daa18d 100644 --- a/core/src/util/testing.ts +++ b/core/src/util/testing.ts @@ -492,3 +492,27 @@ export function expectError(fn: Function, assertion: ExpectErrorAssertion = {}) return handleNonError(false) } + +// adapted from https://stackoverflow.com/a/18543419/1518423 +export function captureStream(stream: NodeJS.WritableStream) { + const oldWrite = stream.write + let buf: string = "" + + class FakeWrite { + write(chunk, _callback) + write(chunk, _encoding?, _callback?) { + buf += chunk.toString() + } + } + + stream["write"] = FakeWrite.prototype.write + + return { + unhook: function unhook() { + stream.write = oldWrite + }, + captured: () => { + return buf + }, + } +} diff --git a/core/src/util/util.ts b/core/src/util/util.ts index cea4f81595..bcd53dad82 100644 --- a/core/src/util/util.ts +++ b/core/src/util/util.ts @@ -860,7 +860,7 @@ export function getGitHubIssueLink(title: string, type: "bug" | "crash" | "featu case "bug": return `https://github.com/garden-io/garden/issues/new?labels=bug&template=BUG_REPORT.md&title=${title}` case "crash": - return `https://github.com/garden-io/garden/issues/new?labels=bug,crashtemplate=CRASH.md&title=${title}` + return `https://github.com/garden-io/garden/issues/new?labels=bug,crash&template=CRASH.md&title=${title}` } } diff --git a/core/test/unit/src/cli/cli.ts b/core/test/unit/src/cli/cli.ts index b699bba74a..492d47d039 100644 --- a/core/test/unit/src/cli/cli.ts +++ b/core/test/unit/src/cli/cli.ts @@ -13,7 +13,7 @@ import { GardenCli, validateRuntimeRequirementsCached } from "../../../../src/cl import { getDataDir, projectRootA, initTestLogger } from "../../../helpers" import { gardenEnv, GARDEN_CORE_ROOT } from "../../../../src/constants" import { join, resolve } from "path" -import { Command, CommandGroup, CommandParams, PrepareParams } from "../../../../src/commands/base" +import { Command, CommandGroup, CommandParams, CommandResult, PrepareParams } from "../../../../src/commands/base" import { UtilCommand } from "../../../../src/commands/util/util" import { StringParameter } from "../../../../src/cli/params" import stripAnsi from "strip-ansi" @@ -22,7 +22,7 @@ import { getRootLogger, RootLogger } from "../../../../src/logger/logger" import { load } from "js-yaml" import { startServer } from "../../../../src/server/server" import { envSupportsEmoji } from "../../../../src/logger/util" -import { expectError, expectFuzzyMatch } from "../../../../src/util/testing" +import { captureStream, expectError, expectFuzzyMatch } from "../../../../src/util/testing" import { GlobalConfigStore } from "../../../../src/config-store/global" import tmp from "tmp-promise" import { CloudCommand } from "../../../../src/commands/cloud/cloud" @@ -33,6 +33,8 @@ import { mkdirp } from "fs-extra" import { uuidv4 } from "../../../../src/util/random" import { makeDummyGarden } from "../../../../src/garden" import { TestGardenCli } from "../../../helpers/cli" +import { NotImplementedError } from "../../../../src/exceptions" +import dedent from "dedent" describe("cli", () => { let cli: GardenCli @@ -910,6 +912,80 @@ describe("cli", () => { "Invalid value for option --env: Invalid environment specified ($.%): must be a valid environment name or ." ) }) + + describe("Command error handling", async () => { + let hook: ReturnType + + beforeEach(() => { + hook = captureStream(process.stdout) + }) + afterEach(() => { + hook.unhook() + }) + it("handles GardenError on the command level correctly", async () => { + class TestCommand extends Command { + name = "test-command" + help = "halp!" + override noProject = true + + override printHeader() {} + + async action({}): Promise { + throw new NotImplementedError({ message: "Error message" }) + } + } + + const cmd = new TestCommand() + cli.addCommand(cmd) + + const { code } = await cli.run({ args: ["test-command"], exitOnError: false }) + + expect(code).to.equal(1) + const output = stripAnsi(hook.captured()) + expect(output).to.eql(dedent` + Error message + + See .garden/error.log for detailed error message\n`) + }) + + it("handles crash on the command level correctly", async () => { + class TestCommand extends Command { + name = "test-command" + help = "halp!" + override noProject = true + + override printHeader() {} + + async action({}): Promise { + throw new TypeError("Cannot read property foo of undefined.") + } + } + + const cmd = new TestCommand() + cli.addCommand(cmd) + + const { code } = await cli.run({ args: ["test-command"], exitOnError: false }) + + expect(code).to.equal(1) + const outputLines = stripAnsi(hook.captured()).split("\n") + + const firstSevenLines = outputLines.slice(0, 7).join("\n") + expect(firstSevenLines).to.eql(dedent` + Encountered an unexpected Garden error. We are sorry for this. This is likely a bug šŸ‚ + + You can help by reporting this on GitHub: https://github.com/garden-io/garden/issues/new?labels=bug,crash&template=CRASH.md&title=Crash%3A%20Cannot%20read%20property%20foo%20of%20undefined. + + Please attach the following information to the bug report after making sure that the error message does not contain sensitive information: + + TypeError: Cannot read property foo of undefined.`) + + const firstStackTraceLine = outputLines[7] + expect(firstStackTraceLine).to.contain("at TestCommand.action (") + + const lastLine = outputLines[outputLines.length - 2] // the last line is empty due to trailing newline + expect(lastLine).to.eql("See .garden/error.log for detailed error message") + }) + }) }) describe("makeDummyGarden", () => { diff --git a/core/test/unit/src/commands/version.ts b/core/test/unit/src/commands/version.ts index 835999a10f..e81be36d69 100644 --- a/core/test/unit/src/commands/version.ts +++ b/core/test/unit/src/commands/version.ts @@ -59,7 +59,6 @@ describe("VersionCommand", () => { const garden = await makeDummyGarden(tmpDir.path, { commandInfo: { name: "version", args: {}, opts: {} } }) const { result } = await command.action({ log: garden.log, - opts: {} } as any) expect(result).to.eql({ version: getPackageVersion(), From d6b199d1081bd83a088a4735793fb3d9f166f103 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 31 Aug 2023 19:48:11 +0200 Subject: [PATCH 07/17] test: add yaml output crash test --- core/test/unit/src/cli/cli.ts | 37 ++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/core/test/unit/src/cli/cli.ts b/core/test/unit/src/cli/cli.ts index 492d47d039..e3c5a01c0d 100644 --- a/core/test/unit/src/cli/cli.ts +++ b/core/test/unit/src/cli/cli.ts @@ -848,7 +848,7 @@ describe("cli", () => { expect(JSON.parse(consoleOutput!)).to.eql({ result: { some: "output" }, success: true }) }) - it("should output YAML if --output=json", async () => { + it("should output YAML if --output=yaml", async () => { class TestCommand extends Command { name = "test-command" help = "halp!" @@ -985,6 +985,41 @@ describe("cli", () => { const lastLine = outputLines[outputLines.length - 2] // the last line is empty due to trailing newline expect(lastLine).to.eql("See .garden/error.log for detailed error message") }) + + it("Handles crash on the command level with --output=yaml correctly", async () => { + class TestCommand extends Command { + name = "test-command" + help = "halp!" + override noProject = true + + override printHeader() {} + + async action({}): Promise { + const err = new Error("Some unexpected error that leads to a crash") + // the stack makes this hard to compare below + err.stack = "stack" + throw err + } + } + + const cmd = new TestCommand() + cli.addCommand(cmd) + + const { code, consoleOutput } = await cli.run({ args: ["test-command", "-o", "yaml"], exitOnError: false }) + + expect(code).to.equal(1) + const resultData = load(consoleOutput!) as any + expect(resultData).to.eql({ + success: false, + errors: [ + { + type: "crash", + message: "Some unexpected error that leads to a crash", + stack: "stack", + }, + ], + }) + }) }) }) From 4b066c8ac8dd795be3985f971ac603a005544aaf Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 16:08:49 +0200 Subject: [PATCH 08/17] fix: filter error details early to avoid issues later Bugfix: Before this commit when running `garden deploy -o json` in the Garden root project, the json was several thousands lines long It was a little bit tricky to solve the error, because I ran into infinite recursion issues between `toSanitizedValue` and `sanitizeValue` Now the -o json output looks good to me. This commit also removes dead code in the GraphNodeError. --- core/src/exceptions.ts | 23 ++---------- core/src/garden.ts | 4 +- core/src/graph/config-graph.ts | 4 ++ core/src/graph/nodes.ts | 60 ++++++++---------------------- core/src/graph/solver.ts | 8 +++- core/src/tasks/base.ts | 4 ++ core/src/util/logging.ts | 22 ++++++++++- core/test/unit/src/util/logging.ts | 51 +++++++++++++++++++++++++ 8 files changed, 108 insertions(+), 68 deletions(-) diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index 5d5a858ab3..c309dbce11 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -45,7 +45,8 @@ export abstract class GardenError extends Error { constructor({ message, detail, stack, wrappedErrors, context }: GardenErrorParams) { super(message) - this.detail = detail + // We sanitize the details right here to avoid issues later down the line + this.detail = withoutInternalFields(sanitizeValue(detail)) this.stack = stack || this.stack this.wrappedErrors = wrappedErrors this.context = context @@ -79,15 +80,6 @@ export abstract class GardenError extends Error { } } - toSanitizedValue() { - return { - type: this.type, - message: this.message, - stack: this.stack, - detail: filterErrorDetail(this.detail), - } - } - formatWithDetail() { return formatGardenErrorWithDetail(this) } @@ -224,10 +216,6 @@ export function toGardenError(err: Error | GardenError | string | any): GardenEr } } -export function filterErrorDetail(detail: any) { - return withoutInternalFields(sanitizeValue(detail)) -} - export function explainGardenError(rawError: GardenError | Error | string, context?: string) { const error = toGardenError(rawError) @@ -261,12 +249,9 @@ export function formatGardenErrorWithDetail(error: GardenError) { let out = stack || message || "" - // We sanitize and recursively filter out internal fields (i.e. having names starting with _). - const filteredDetail = filterErrorDetail(detail) - - if (!isEmpty(filteredDetail)) { + if (!isEmpty(detail || {})) { try { - const yamlDetail = stringify(filteredDetail, { blockQuote: "literal", lineWidth: 0 }) + const yamlDetail = stringify(detail, { blockQuote: "literal", lineWidth: 0 }) out += `\n\nError Details:\n\n${yamlDetail}` } catch (err) { out += `\n\nUnable to render error details:\n${err.message}` diff --git a/core/src/garden.ts b/core/src/garden.ts index dd12c188d6..3aacea1441 100644 --- a/core/src/garden.ts +++ b/core/src/garden.ts @@ -40,7 +40,7 @@ import { getCloudDistributionName, getCloudLogSectionName, } from "./util/util" -import { ConfigurationError, GardenError, PluginError, RuntimeError, InternalError } from "./exceptions" +import { ConfigurationError, GardenError, PluginError, RuntimeError, InternalError, toGardenError } from "./exceptions" import { VcsHandler, ModuleVersion, getModuleVersionString, VcsInfo } from "./vcs/vcs" import { GitHandler } from "./vcs/git" import { BuildStaging } from "./build-staging/build-staging" @@ -810,7 +810,7 @@ export class Garden { const failedNames = failed.map((r) => r!.name) const wrappedErrors: GardenError[] = failed.flatMap((f) => { - return f && f.error instanceof GardenError ? [f.error] : [] + return f && f.error ? [toGardenError(f.error)] : [] }) throw new PluginError({ diff --git a/core/src/graph/config-graph.ts b/core/src/graph/config-graph.ts index a9ac8c2fec..70b21b57e8 100644 --- a/core/src/graph/config-graph.ts +++ b/core/src/graph/config-graph.ts @@ -624,4 +624,8 @@ export class ConfigGraphNode { return nodes } } + + toSanitizedValue() { + return ` { startedAt, aborted: true, result: null, - // Note: The error message is constructed in the error constructor - error: new GraphNodeError({ ...this.result, aborted: true, node: d }), + // If it was aborted without error, we don't need a GraphNodeError + error: error ? new GraphNodeError({ resultError: error, node: d }) : null, }) } } @@ -337,53 +337,25 @@ export interface CompleteTaskParams { aborted: boolean } -export interface GraphNodeErrorDetail extends GraphResult { +export interface GraphNodeErrorDetail { node: TaskNode - failedDependency?: TaskNode } -export interface GraphNodeErrorParams extends GraphNodeErrorDetail {} +export interface GraphNodeErrorParams { + resultError: Error + node: TaskNode +} export class GraphNodeError extends GraphError { constructor(params: GraphNodeErrorParams) { - const { node, failedDependency, error } = params - - let message = "" - let stack: string | undefined - - if (failedDependency) { - message = `${node.describe()} aborted because a dependency could not be completed:` - - let nextDep: TaskNode | null = failedDependency - - while (nextDep) { - const result = nextDep.getResult() - - if (!result) { - nextDep = null - } else if (result?.aborted) { - message += chalk.yellow(`\nā†³ ${nextDep.describe()} [ABORTED]`) - if (result.error instanceof GraphNodeError && result.error.detail?.failedDependency) { - nextDep = result.error.detail.failedDependency - } else { - nextDep = null - } - } else if (result?.error) { - message += chalk.red.bold(`\nā†³ ${nextDep.describe()} [FAILED] - ${result.error.message}`) - stack = result.error.stack - nextDep = null - } - } - } else { - message = `${node.describe()} failed: ${error}` - stack = error?.stack - } - - const wrappedErrors = [toGardenError(error)] - super({ message, stack, detail: params, wrappedErrors, context: { taskType: node.task.type } }) - } + const { resultError, node } = params - aborted() { - return this.detail?.aborted + const message = `${node.describe()} failed: ${resultError}` + const stack = resultError?.stack + const detail = { + node + } + const wrappedErrors = [toGardenError(resultError)] + super({ message, stack, detail, wrappedErrors, context: { taskType: node.task.type } }) } } diff --git a/core/src/graph/solver.ts b/core/src/graph/solver.ts index bef8eb6274..f893cafc6a 100644 --- a/core/src/graph/solver.ts +++ b/core/src/graph/solver.ts @@ -93,6 +93,10 @@ export class GraphSolver extends TypedEventEmitter { }) } + toSanitizedValue() { + return "" + } + async solve(params: SolveParams): Promise { const { statusOnly, tasks, throwOnError, log } = params @@ -173,8 +177,8 @@ export class GraphSolver extends TypedEventEmitter { continue } - if (r.error instanceof GardenError) { - wrappedErrors.push(r.error) + if (r.error) { + wrappedErrors.push(toGardenError(r.error)) } msg += `\n ā†³ ${r.description}: ${r?.error ? r.error.message : "[ABORTED]"}` diff --git a/core/src/tasks/base.ts b/core/src/tasks/base.ts index 6d1b6c01e7..eb16f01cf7 100644 --- a/core/src/tasks/base.ts +++ b/core/src/tasks/base.ts @@ -204,6 +204,10 @@ export abstract class BaseTask exte isExecuteTask(): this is ExecuteTask { return this.executeTask } + + toSanitizedValue() { + return `` + } } export interface ActionTaskStatusParams<_ extends Action> extends TaskProcessParams {} diff --git a/core/src/util/logging.ts b/core/src/util/logging.ts index ebd5b69c35..9554e9947f 100644 --- a/core/src/util/logging.ts +++ b/core/src/util/logging.ts @@ -11,10 +11,17 @@ import stripAnsi from "strip-ansi" import { isPrimitive } from "../config/common" import { deepFilter } from "./objects" +let _callingToSanitizedValueOrToJSON = false + /** * Strips undefined values, internal objects and circular references from an object. */ export function sanitizeValue(value: any, _parents?: WeakSet): any { + if (_callingToSanitizedValueOrToJSON) { + // I can't use InternalError here, because that calls sanitizeValue + throw new Error("`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion.") + } + if (!_parents) { _parents = new WeakSet() } else if (_parents.has(value)) { @@ -45,7 +52,20 @@ export function sanitizeValue(value: any, _parents?: WeakSet): any { // Looks to be a class instance if (value.toSanitizedValue) { // Special allowance for internal objects - return value.toSanitizedValue() + try { + _callingToSanitizedValueOrToJSON = true + return value.toSanitizedValue() + } finally { + _callingToSanitizedValueOrToJSON = false + } + } else if (value.toJSON) { + // Use existing JSON serialisation function + try { + _callingToSanitizedValueOrToJSON = true + return value.toJSON() + } finally { + _callingToSanitizedValueOrToJSON = false + } } else { // Any other class. Convert to plain object and sanitize attributes. _parents.add(value) diff --git a/core/test/unit/src/util/logging.ts b/core/test/unit/src/util/logging.ts index 3a386200a0..ffed761605 100644 --- a/core/test/unit/src/util/logging.ts +++ b/core/test/unit/src/util/logging.ts @@ -160,6 +160,57 @@ describe("sanitizeValue", () => { }) }) + it("calls toJSON method if present", async () => { + class Foo { + toJSON() { + return { + foo: "bar" + } + } + } + const obj = { + a: new Foo(), + } + const res = sanitizeValue(obj) + expect(res).to.eql({ + a: { + foo: "bar" + }, + }) + }) + + it("prevents calling sanitizeValue from toSanitizeValue to prevent infinite recursion", async () => { + class Foo { + toSanitizedValue() { + return sanitizeValue({ + foo: "bar" + }) + } + } + const obj = { + a: new Foo(), + } + expect(() => { + sanitizeValue(obj) + }).to.throw("`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion.") + }) + + it("prevents calling sanitizeValue from toJSON to prevent infinite recursion", async () => { + class Foo { + toJSON() { + return sanitizeValue({ + foo: "bar" + }) + } + } + const obj = { + a: new Foo(), + } + expect(() => { + sanitizeValue(obj) + }).to.throw("`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion.") + }) + it("replaces LogEntry instance on a class instance", async () => { class Foo { log: Log From 08349dc3fe33e782589f9dca02de65620fd33e68 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 16:25:02 +0200 Subject: [PATCH 09/17] test(solver): add test for wrapping behaviour --- core/test/unit/src/graph/solver.ts | 32 +++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/core/test/unit/src/graph/solver.ts b/core/test/unit/src/graph/solver.ts index 558d9f4a42..786a3648fc 100644 --- a/core/test/unit/src/graph/solver.ts +++ b/core/test/unit/src/graph/solver.ts @@ -12,6 +12,7 @@ import { makeTestGarden, freezeTime, TestGarden, getDataDir, expectError } from import { MakeOptional } from "../../../../src/util/util" import { SolveOpts } from "../../../../src/graph/solver" import { ActionState } from "../../../../src/actions/types" +import { GardenError, GenericGardenError } from "../../../../src/exceptions" const projectRoot = getDataDir("test-project-empty") @@ -191,7 +192,7 @@ describe("GraphSolver", () => { expect(resultB?.outputs.callbackResult).to.equal(taskA.getId()) }) - it("returns an error when task processing fails", async () => { + it("returns an error when task processing fails due a crash (Non-garden error)", async () => { const task = makeTask({}) task.process = async () => { @@ -203,6 +204,35 @@ describe("GraphSolver", () => { expect(result).to.exist expect(result!.error).to.exist expect(result!.error?.message).to.include("Throwing error in process method") + expect(result!.error).to.be.instanceOf(GardenError) + const error = result!.error as GardenError + expect(error.type).to.eql("graph") + expect(error.wrappedErrors?.length).to.eql(1) + const rootCause = error.wrappedErrors![0]! + expect(rootCause.type).to.eql("crash") + }) + + it("returns an error when task processing fails due a GardenError", async () => { + const task = makeTask({}) + + task.process = async () => { + throw new GenericGardenError({ + message: "non-crash error scenario", + type: "test" + }) + } + + const result = await processTask(task) + + expect(result).to.exist + expect(result!.error).to.exist + expect(result!.error?.message).to.include("non-crash error scenario") + expect(result!.error).to.be.instanceOf(GardenError) + const error = result!.error as GardenError + expect(error.type).to.eql("graph") + expect(error.wrappedErrors?.length).to.eql(1) + const rootCause = error.wrappedErrors![0]! + expect(rootCause.type).to.eql("test") }) it("throws an error when task processing fails and throwOnError=true", async () => { From 251c14d082074e8d295fbbe213d2ca11ab614cea Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 16:59:01 +0200 Subject: [PATCH 10/17] fix: do not crash when spawned child processes fail Usually spawned child processes are expected to fail for various reasons. Wrap them in ChildProcessError Also improved the error message and error type for Kubernetes apply through kubectl. --- core/src/exceptions.ts | 13 ++++++- core/src/plugins/kubernetes/kubectl.ts | 47 ++++++++++++++++++-------- core/src/util/util.ts | 31 +++++++++++------ 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index c309dbce11..9822f95f1a 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -9,7 +9,7 @@ import { isEmpty, isString } from "lodash" import { stringify } from "yaml" import { withoutInternalFields, sanitizeValue } from "./util/logging" -import { getGitHubIssueLink, testFlags } from "./util/util" +import { SpawnOpts, getGitHubIssueLink, testFlags } from "./util/util" import dedent from "dedent" import chalk from "chalk" import stripAnsi from "strip-ansi" @@ -161,6 +161,17 @@ export class TemplateStringError extends GardenError { type = "template-string" } +interface ChildProcessErrorDetails { + cmd: string + args: string[] + code: number + output: string + opts?: SpawnOpts +} +export class ChildProcessError extends GardenError { + type = "childprocess" +} + interface GenericGardenErrorParams extends GardenErrorParams { type: string } diff --git a/core/src/plugins/kubernetes/kubectl.ts b/core/src/plugins/kubernetes/kubectl.ts index 2af3758dea..9c612b6558 100644 --- a/core/src/plugins/kubernetes/kubectl.ts +++ b/core/src/plugins/kubernetes/kubectl.ts @@ -11,13 +11,13 @@ import { ExecParams, PluginTool } from "../../util/ext-tools" import { Log } from "../../logger/log-entry" import { KubernetesProvider } from "./config" import { KubernetesResource } from "./types" -import { gardenAnnotationKey } from "../../util/string" +import { dedent, gardenAnnotationKey } from "../../util/string" import { getResourceKey, hashManifest } from "./util" import { PluginToolSpec } from "../../plugin/tools" import { PluginContext } from "../../plugin-context" -import { KubeApi } from "./api" +import { KubeApi, KubernetesError } from "./api" import { pathExists } from "fs-extra" -import { ConfigurationError } from "../../exceptions" +import { ChildProcessError, ConfigurationError } from "../../exceptions" import { requestWithRetry, RetryOpts } from "./retry" // Corresponds to the default prune whitelist in `kubectl`. @@ -118,18 +118,35 @@ export async function apply({ args.push("--output=json", "-f", "-") !validate && args.push("--validate=false") - const result = await requestWithRetry( - log, - `kubectl ${args.join(" ")}`, - () => - kubectl(ctx, provider).stdout({ - log, - namespace, - args, - input, - }), - KUBECTL_DEFAULT_RETRY_OPTS - ) + let result: string + try { + result = await requestWithRetry( + log, + `kubectl ${args.join(" ")}`, + () => + kubectl(ctx, provider).stdout({ + log, + namespace, + args, + input, + }), + KUBECTL_DEFAULT_RETRY_OPTS + ) + } catch (e) { + if (e instanceof ChildProcessError) { + throw new KubernetesError({ + message: dedent` + Failed to apply Kubernetes manifests. This is the output of the kubectl command: + + ${e.detail?.output} + + Use the option "--log-level verbose" to see the kubernetes manifests that we attempted to apply through "kubectl apply". + `, + detail: e.detail + }) + } + throw e + } if (namespace && resourcesToPrune.length > 0) { await deleteResources({ diff --git a/core/src/util/util.ts b/core/src/util/util.ts index bcd53dad82..c862a83ab5 100644 --- a/core/src/util/util.ts +++ b/core/src/util/util.ts @@ -27,7 +27,7 @@ import { import { asyncExitHook, gracefulExit } from "@scg82/exit-hook" import _spawn from "cross-spawn" import { readFile } from "fs-extra" -import { GardenError, ParameterError, RuntimeError, TimeoutError } from "../exceptions" +import { ChildProcessError, GardenError, ParameterError, RuntimeError, TimeoutError } from "../exceptions" import { load } from "js-yaml" import { createHash } from "crypto" import { dedent, tailString } from "./string" @@ -253,15 +253,21 @@ export async function exec(cmd: string, args: string[], opts: ExecOpts = {}) { } const error = err - const message = makeErrorMsg({ - cmd, - args, - code: error.exitCode || err.code || err.errno, - output: error.all || error.stdout || error.stderr || "", - error: error.stderr, + throw new ChildProcessError({ + message: makeErrorMsg({ + cmd, + args, + code: error.exitCode || err.code || err.errno, + output: error.all || error.stdout || error.stderr || "", + error: error.stderr, + }), + detail: { + cmd, + args, + code: error.exitCode || err.code || err.errno, + output: error.all || error.stdout || error.stderr || "", + }, }) - error.message = message - throw error } } @@ -396,7 +402,12 @@ export function spawn(cmd: string, args: string[], opts: SpawnOpts = {}) { output: result.all || result.stdout || result.stderr || "", error: result.stderr || "", }) - _reject(new RuntimeError({ message: msg, detail: { cmd, args, opts, result } })) + _reject( + new ChildProcessError({ + message: msg, + detail: { cmd, args, opts, code: result.code, output: result.all }, + }) + ) } }) }) From f1841918a30c21e79821ea25bceebb11eff5cdfe Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 17:09:55 +0200 Subject: [PATCH 11/17] refactor: remove --- core/src/cli/cli.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/core/src/cli/cli.ts b/core/src/cli/cli.ts index 70dcaace1b..04759d7f0d 100644 --- a/core/src/cli/cli.ts +++ b/core/src/cli/cli.ts @@ -402,11 +402,6 @@ ${renderCommands(commands)} processRecord?: GardenProcess cwd?: string }): Promise { - // for testing - if (args.includes("--test-cli-error")) { - throw new Error("Test") - } - let argv = parseCliArgs({ stringArgs: args, cli: true }) const errors: (GardenError | Error)[] = [] From 9b0742fe71376a64c224b50aadc6c4b51b44aa1e Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 17:13:22 +0200 Subject: [PATCH 12/17] fix: handle undefined message in wrapError --- core/src/exceptions.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index 9822f95f1a..a0e77c82c3 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -200,7 +200,7 @@ export class InternalError extends GardenError { // not using object destructuring here on purpose, because errors are of type any and then the error might be passed as the params object accidentally. static wrapError(error: Error | string | any, detail?: unknown, prefix?: string): InternalError { - let message: string + let message: string | undefined let stack: string | undefined if (error instanceof Error) { @@ -208,12 +208,12 @@ export class InternalError extends GardenError { stack = error.stack } else if (isString(error)) { message = error - } else { + } else if (error) { message = error["message"] stack = error["stack"] } - message = stripAnsi(message) + message = message ? stripAnsi(message) : "" return new InternalError({ message: prefix ? `${stripAnsi(prefix)}: ${message}` : message, stack, detail }) } From 586f1dca8f18dcaf9cbbe058bbd4e8a57c035522 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 17:18:46 +0200 Subject: [PATCH 13/17] fix: lint errors --- core/src/graph/nodes.ts | 2 +- core/src/plugins/kubernetes/kubectl.ts | 2 +- core/src/util/logging.ts | 4 +++- core/test/unit/src/graph/solver.ts | 2 +- core/test/unit/src/util/logging.ts | 16 ++++++++++------ 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/core/src/graph/nodes.ts b/core/src/graph/nodes.ts index 089fda48d5..89951beef0 100644 --- a/core/src/graph/nodes.ts +++ b/core/src/graph/nodes.ts @@ -353,7 +353,7 @@ export class GraphNodeError extends GraphError { const message = `${node.describe()} failed: ${resultError}` const stack = resultError?.stack const detail = { - node + node, } const wrappedErrors = [toGardenError(resultError)] super({ message, stack, detail, wrappedErrors, context: { taskType: node.task.type } }) diff --git a/core/src/plugins/kubernetes/kubectl.ts b/core/src/plugins/kubernetes/kubectl.ts index 9c612b6558..28f8944faf 100644 --- a/core/src/plugins/kubernetes/kubectl.ts +++ b/core/src/plugins/kubernetes/kubectl.ts @@ -142,7 +142,7 @@ export async function apply({ Use the option "--log-level verbose" to see the kubernetes manifests that we attempted to apply through "kubectl apply". `, - detail: e.detail + detail: e.detail, }) } throw e diff --git a/core/src/util/logging.ts b/core/src/util/logging.ts index 9554e9947f..ee3f07c539 100644 --- a/core/src/util/logging.ts +++ b/core/src/util/logging.ts @@ -19,7 +19,9 @@ let _callingToSanitizedValueOrToJSON = false export function sanitizeValue(value: any, _parents?: WeakSet): any { if (_callingToSanitizedValueOrToJSON) { // I can't use InternalError here, because that calls sanitizeValue - throw new Error("`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion.") + throw new Error( + "`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion." + ) } if (!_parents) { diff --git a/core/test/unit/src/graph/solver.ts b/core/test/unit/src/graph/solver.ts index 786a3648fc..d48ac09a1f 100644 --- a/core/test/unit/src/graph/solver.ts +++ b/core/test/unit/src/graph/solver.ts @@ -218,7 +218,7 @@ describe("GraphSolver", () => { task.process = async () => { throw new GenericGardenError({ message: "non-crash error scenario", - type: "test" + type: "test", }) } diff --git a/core/test/unit/src/util/logging.ts b/core/test/unit/src/util/logging.ts index ffed761605..2ae49b9b0c 100644 --- a/core/test/unit/src/util/logging.ts +++ b/core/test/unit/src/util/logging.ts @@ -164,7 +164,7 @@ describe("sanitizeValue", () => { class Foo { toJSON() { return { - foo: "bar" + foo: "bar", } } } @@ -174,7 +174,7 @@ describe("sanitizeValue", () => { const res = sanitizeValue(obj) expect(res).to.eql({ a: { - foo: "bar" + foo: "bar", }, }) }) @@ -183,7 +183,7 @@ describe("sanitizeValue", () => { class Foo { toSanitizedValue() { return sanitizeValue({ - foo: "bar" + foo: "bar", }) } } @@ -192,14 +192,16 @@ describe("sanitizeValue", () => { } expect(() => { sanitizeValue(obj) - }).to.throw("`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion.") + }).to.throw( + "`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion." + ) }) it("prevents calling sanitizeValue from toJSON to prevent infinite recursion", async () => { class Foo { toJSON() { return sanitizeValue({ - foo: "bar" + foo: "bar", }) } } @@ -208,7 +210,9 @@ describe("sanitizeValue", () => { } expect(() => { sanitizeValue(obj) - }).to.throw("`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion.") + }).to.throw( + "`toSanitizedValue` and `toJSON` are not allowed to call `sanitizeValue` because that can cause infinite recursion." + ) }) it("replaces LogEntry instance on a class instance", async () => { From f7a4ce9e70485736c8c79874e587f4b139b59c26 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 17:42:53 +0200 Subject: [PATCH 14/17] fix: avoid throwing non-garden errors --- core/src/commands/workflow.ts | 10 +++++----- core/src/plugins/kubernetes/api.ts | 8 ++++++-- core/src/plugins/kubernetes/container/deployment.ts | 6 ++++-- core/src/plugins/kubernetes/status/status.ts | 2 +- core/src/plugins/openshift/build.ts | 3 ++- core/src/util/open-telemetry/decorators.ts | 3 ++- core/test/unit/src/commands/workflow.ts | 1 + 7 files changed, 21 insertions(+), 12 deletions(-) diff --git a/core/src/commands/workflow.ts b/core/src/commands/workflow.ts index 82ef285ec7..c5dbef5e6f 100644 --- a/core/src/commands/workflow.ts +++ b/core/src/commands/workflow.ts @@ -15,7 +15,7 @@ import { dedent, wordWrap, deline } from "../util/string" import { Garden } from "../garden" import { WorkflowStepSpec, WorkflowConfig, WorkflowFileSpec } from "../config/workflow" import { Log } from "../logger/log-entry" -import { formatGardenErrorWithDetail, GardenError, WorkflowScriptError } from "../exceptions" +import { formatGardenErrorWithDetail, GardenError, RuntimeError, WorkflowScriptError } from "../exceptions" import { WorkflowConfigContext, WorkflowStepConfigContext, @@ -206,11 +206,11 @@ export class WorkflowCommand extends Command { const finalError = opts.output ? errors : [ - new Error( - `workflow failed with ${errors.length} ${ + new RuntimeError({ + message: `workflow failed with ${errors.length} ${ errors.length > 1 ? "errors" : "error" - }, see logs above for more info` - ), + }, see logs above for more info`, + }), ] return { result, errors: finalError } } diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index d67aa0d378..34deddfac5 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/core/src/plugins/kubernetes/api.ts @@ -917,7 +917,9 @@ function attachWebsocketKeepalive(ws: WebSocket): WebSocket { pingTimeout = setTimeout(() => { ws.emit( "error", - new Error(`Lost connection to the Kubernetes WebSocket API (Timed out after ${WEBSOCKET_PING_TIMEOUT / 1000}s)`) + new KubernetesError({ + message: `Lost connection to the Kubernetes WebSocket API (Timed out after ${WEBSOCKET_PING_TIMEOUT / 1000}s)` + }) ) ws.terminate() }, WEBSOCKET_PING_TIMEOUT) @@ -1000,7 +1002,9 @@ async function getContextConfig(log: Log, ctx: PluginContext, provider: Kubernet kc.loadFromString(safeDumpYaml(rawConfig)) kc.setCurrentContext(context) } catch (err) { - throw new Error("Could not parse kubeconfig, " + err) + throw new KubernetesError({ + message: `Could not parse kubeconfig: ${err}` + }) } cachedConfigs[cacheKey] = kc diff --git a/core/src/plugins/kubernetes/container/deployment.ts b/core/src/plugins/kubernetes/container/deployment.ts index d1a3d66beb..e2dd847d53 100644 --- a/core/src/plugins/kubernetes/container/deployment.ts +++ b/core/src/plugins/kubernetes/container/deployment.ts @@ -583,7 +583,9 @@ function configureHealthCheck(container: V1Container, spec: ContainerDeploySpec, } container.livenessProbe.tcpSocket = container.readinessProbe.tcpSocket } else { - throw new Error("Must specify type of health check when configuring health check.") + throw new ConfigurationError({ + message: "Must specify type of health check when configuring health check." + }) } } @@ -599,7 +601,7 @@ export function configureVolumes( const volumeName = volume.name if (!volumeName) { - throw new Error("Must specify volume name") + throw new ConfigurationError({ message: "Must specify volume name" }) } volumeMounts.push({ diff --git a/core/src/plugins/kubernetes/status/status.ts b/core/src/plugins/kubernetes/status/status.ts index a7bcf1ef77..cb0745c387 100644 --- a/core/src/plugins/kubernetes/status/status.ts +++ b/core/src/plugins/kubernetes/status/status.ts @@ -525,7 +525,7 @@ export async function compareDeployedResources({ // console.log(JSON.stringify(resource, null, 4)) // console.log(JSON.stringify(existingSpec, null, 4)) // console.log("----------------------------------------------------") - // throw new Error("bla") + // throw new InternalError("bla") result.state = "outdated" return result } diff --git a/core/src/plugins/openshift/build.ts b/core/src/plugins/openshift/build.ts index 5ff175bf37..fb614d2675 100644 --- a/core/src/plugins/openshift/build.ts +++ b/core/src/plugins/openshift/build.ts @@ -15,6 +15,7 @@ import { k8sPublishContainerBuild } from "../kubernetes/container/publish" import { BuildHandler, BuildStatusHandler } from "../kubernetes/container/build/common" import { getLocalBuildStatus, localBuild } from "../kubernetes/container/build/local" import { getKanikoBuildStatus, kanikoBuild } from "../kubernetes/container/build/kaniko" +import { NotImplementedError } from "../../exceptions" export const openshiftContainerBuildExtension = (): BuildActionExtension => ({ name: "container", @@ -50,7 +51,7 @@ export const openshiftContainerBuildExtension = (): BuildActionExtension { - throw new Error("Unimplemented handler called in OpenShift Build") + throw new NotImplementedError({ message: "Unimplemented handler called in OpenShift Build" }) } const buildStatusHandlers: { [mode in ContainerBuildMode]: BuildStatusHandler } = { diff --git a/core/src/util/open-telemetry/decorators.ts b/core/src/util/open-telemetry/decorators.ts index 3f039f989a..4954563989 100644 --- a/core/src/util/open-telemetry/decorators.ts +++ b/core/src/util/open-telemetry/decorators.ts @@ -10,6 +10,7 @@ import * as opentelemetry from "@opentelemetry/sdk-node" import { tracer } from "./tracing" import { getSessionContext } from "./context" import { prefixWithGardenNamespace } from "./util" +import { InternalError } from "../../exceptions" type GetAttributesCallback = (this: C, ...args: T) => opentelemetry.api.Attributes type GetNameCallback = (this: C, ...args: T) => string @@ -43,7 +44,7 @@ export function OtelTraced({ const method = descriptor.value if (!method) { - throw new Error("No method to decorate") + throw new InternalError({ message: "No method to decorate" }) } descriptor.value = async function (this: C, ...args: T) { diff --git a/core/test/unit/src/commands/workflow.ts b/core/test/unit/src/commands/workflow.ts index 8bf7ca9740..50c6f199e2 100644 --- a/core/test/unit/src/commands/workflow.ts +++ b/core/test/unit/src/commands/workflow.ts @@ -813,6 +813,7 @@ describe("RunWorkflowCommand", () => { const { errors } = await cmd.action({ ...defaultParams, args: { workflow: "workflow-a" } }) + expect(errors![0].type).to.equal("runtime") expect(errors![0].message).to.equal("workflow failed with 1 error, see logs above for more info") // no details because log is set to human-readable output and details are logged above expect(errors![0].detail).to.equal(undefined) From 1edcd7381ddf04260a0da76de2bc201e7d1dc05d Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 17:46:08 +0200 Subject: [PATCH 15/17] fix: lint --- core/src/graph/nodes.ts | 2 +- core/src/plugins/kubernetes/api.ts | 6 ++++-- core/src/plugins/kubernetes/container/deployment.ts | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/graph/nodes.ts b/core/src/graph/nodes.ts index 89951beef0..4cbd477340 100644 --- a/core/src/graph/nodes.ts +++ b/core/src/graph/nodes.ts @@ -7,7 +7,7 @@ */ import { Task, ValidResultType } from "../tasks/base" -import { GardenError, GraphError, InternalError, toGardenError } from "../exceptions" +import { GraphError, InternalError, toGardenError } from "../exceptions" import { GraphResult, GraphResultFromTask, GraphResults } from "./results" import type { GraphSolver } from "./solver" import { ValuesType } from "utility-types" diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index 34deddfac5..13af988108 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/core/src/plugins/kubernetes/api.ts @@ -918,7 +918,9 @@ function attachWebsocketKeepalive(ws: WebSocket): WebSocket { ws.emit( "error", new KubernetesError({ - message: `Lost connection to the Kubernetes WebSocket API (Timed out after ${WEBSOCKET_PING_TIMEOUT / 1000}s)` + message: `Lost connection to the Kubernetes WebSocket API (Timed out after ${ + WEBSOCKET_PING_TIMEOUT / 1000 + }s)`, }) ) ws.terminate() @@ -1003,7 +1005,7 @@ async function getContextConfig(log: Log, ctx: PluginContext, provider: Kubernet kc.setCurrentContext(context) } catch (err) { throw new KubernetesError({ - message: `Could not parse kubeconfig: ${err}` + message: `Could not parse kubeconfig: ${err}`, }) } diff --git a/core/src/plugins/kubernetes/container/deployment.ts b/core/src/plugins/kubernetes/container/deployment.ts index e2dd847d53..7914c47875 100644 --- a/core/src/plugins/kubernetes/container/deployment.ts +++ b/core/src/plugins/kubernetes/container/deployment.ts @@ -584,7 +584,7 @@ function configureHealthCheck(container: V1Container, spec: ContainerDeploySpec, container.livenessProbe.tcpSocket = container.readinessProbe.tcpSocket } else { throw new ConfigurationError({ - message: "Must specify type of health check when configuring health check." + message: "Must specify type of health check when configuring health check.", }) } } From 2c73917244072f8fde2ec007e700d8370ea693fb Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 1 Sep 2023 19:00:44 +0200 Subject: [PATCH 16/17] fix: error handling issues --- core/src/exceptions.ts | 6 ++- .../kubernetes/container/build/common.ts | 33 +++++++++------- core/src/plugins/kubernetes/local/config.ts | 6 ++- core/src/plugins/kubernetes/local/microk8s.ts | 12 +++--- core/src/plugins/kubernetes/local/minikube.ts | 7 +++- core/src/plugins/kubernetes/run.ts | 2 +- core/src/router/build.ts | 20 ++++------ core/src/util/ext-tools.ts | 30 +++++++++----- core/src/util/util.ts | 18 ++++++++- core/src/vcs/git.ts | 39 ++++++++++++------- core/test/unit/src/util/util.ts | 7 ++++ 11 files changed, 117 insertions(+), 63 deletions(-) diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index a0e77c82c3..8c8bd5041b 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -36,10 +36,10 @@ export interface GardenErrorParams { export type GardenErrorContext = { taskType?: string } -export abstract class GardenError extends Error { +export abstract class GardenError extends Error { abstract type: string public override message: string - public detail?: D + public detail: D public wrappedErrors?: GardenError[] public context?: GardenErrorContext @@ -166,6 +166,8 @@ interface ChildProcessErrorDetails { args: string[] code: number output: string + stderr: string + stdout: string opts?: SpawnOpts } export class ChildProcessError extends GardenError { diff --git a/core/src/plugins/kubernetes/container/build/common.ts b/core/src/plugins/kubernetes/container/build/common.ts index d55ec1cb4d..0b57abf395 100644 --- a/core/src/plugins/kubernetes/container/build/common.ts +++ b/core/src/plugins/kubernetes/container/build/common.ts @@ -18,7 +18,7 @@ import { } from "../../constants" import { KubeApi } from "../../api" import { KubernetesPluginContext, KubernetesProvider } from "../../config" -import { PodRunner } from "../../run" +import { PodRunner, PodRunnerError } from "../../run" import { PluginContext } from "../../../../plugin-context" import { hashString, sleep } from "../../../../util/util" import { InternalError, RuntimeError } from "../../../../exceptions" @@ -230,20 +230,25 @@ export async function skopeoBuildStatus({ }) return { state: "ready", outputs, detail: {} } } catch (err) { - const res = err.detail?.result || {} - - // Non-zero exit code can both mean the manifest is not found, and any other unexpected error - if (res.exitCode !== 0 && !skopeoManifestUnknown(res.stderr)) { - const output = res.allLogs || err.message - - throw new RuntimeError({ - message: `Unable to query registry for image status: ${output}`, - detail: { - command: skopeoCommand, - output, - }, - }) + if (err instanceof PodRunnerError) { + const res = err.detail.result + + // Non-zero exit code can both mean the manifest is not found, and any other unexpected error + if (res?.exitCode !== 0 && !skopeoManifestUnknown(res?.stderr)) { + const output = res?.allLogs || err.message + + throw new RuntimeError({ + message: `Unable to query registry for image status: ${output}`, + detail: { + command: skopeoCommand, + output, + }, + }) + } } + + // TODO: Do we really want to return state: "unknown" with no details on any error, even on TypeError etc? + // NOTE(steffen): I'd have expected us to throw here return { state: "unknown", outputs, detail: {} } } } diff --git a/core/src/plugins/kubernetes/local/config.ts b/core/src/plugins/kubernetes/local/config.ts index 6f225166e4..4778521671 100644 --- a/core/src/plugins/kubernetes/local/config.ts +++ b/core/src/plugins/kubernetes/local/config.ts @@ -24,6 +24,7 @@ import chalk from "chalk" import { isKindCluster } from "./kind" import { isK3sFamilyCluster } from "./k3s" import { getK8sClientServerVersions, K8sClientServerVersions } from "../util" +import { ChildProcessError } from "../../../exceptions" // TODO: split this into separate plugins to handle Docker for Mac and Minikube // note: this is in order of preference, in case neither is set as the current kubectl context @@ -154,7 +155,10 @@ export async function configureProvider(params: ConfigureProviderParams nginxServices.includes(s)) } diff --git a/core/src/plugins/kubernetes/local/microk8s.ts b/core/src/plugins/kubernetes/local/microk8s.ts index ae68d20b36..6605619037 100644 --- a/core/src/plugins/kubernetes/local/microk8s.ts +++ b/core/src/plugins/kubernetes/local/microk8s.ts @@ -7,7 +7,7 @@ */ import tmp from "tmp-promise" -import { RuntimeError } from "../../../exceptions" +import { ChildProcessError, RuntimeError } from "../../../exceptions" import { Log } from "../../../logger/log-entry" import { exec } from "../../../util/util" import { containerHelpers } from "../../container/helpers" @@ -20,7 +20,7 @@ import { parse as parsePath } from "path" // TODO: Pass the correct log context instead of creating it here. export async function configureMicrok8sAddons(log: Log, addons: string[]) { - let statusCommandResult: ExecaReturnValue | undefined = undefined + let statusCommandResult: ExecaReturnValue | ChildProcessError | undefined = undefined let status = "" const microK8sLog = log.createLog({ name: "microk8s" }) @@ -28,7 +28,10 @@ export async function configureMicrok8sAddons(log: Log, addons: string[]) { statusCommandResult = await exec("microk8s", ["status"]) status = statusCommandResult.stdout } catch (err) { - if (err.all?.includes("permission denied") || err.all?.includes("Insufficient permissions")) { + if (!(err instanceof ChildProcessError)) { + throw err + } + if (err.detail.output.includes("permission denied") || err.detail.output.includes("Insufficient permissions")) { microK8sLog.warn( chalk.yellow( deline`Unable to get microk8s status and manage addons. You may need to add the current user to the microk8s @@ -147,9 +150,6 @@ export async function loadImageToMicrok8s({ } catch (err) { throw new RuntimeError({ message: `An attempt to load image ${imageId} into the microk8s cluster failed: ${err.message}`, - detail: { - err, - }, }) } } diff --git a/core/src/plugins/kubernetes/local/minikube.ts b/core/src/plugins/kubernetes/local/minikube.ts index 24cc50dc7c..328f3e16b0 100644 --- a/core/src/plugins/kubernetes/local/minikube.ts +++ b/core/src/plugins/kubernetes/local/minikube.ts @@ -6,8 +6,8 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import execa from "execa" import { exec } from "../../../util/util" +import { ChildProcessError } from "../../../exceptions" /** * Automatically set docker environment variables for minikube @@ -19,7 +19,10 @@ export async function setMinikubeDockerEnv() { try { minikubeEnv = (await exec("minikube", ["docker-env", "--shell=bash"])).stdout } catch (err) { - if ((err).stderr?.includes("driver does not support")) { + if (!(err instanceof ChildProcessError)) { + throw err + } + if (err.detail.output?.includes("driver does not support")) { return } throw err diff --git a/core/src/plugins/kubernetes/run.ts b/core/src/plugins/kubernetes/run.ts index b23277ee8b..acaa67abe5 100644 --- a/core/src/plugins/kubernetes/run.ts +++ b/core/src/plugins/kubernetes/run.ts @@ -735,7 +735,7 @@ type RunParams = StartParams & { throwOnExitCode?: boolean } -class PodRunnerError extends GardenError { +export class PodRunnerError extends GardenError { type = "pod-runner" } diff --git a/core/src/router/build.ts b/core/src/router/build.ts index 11ece6ac38..9d28d2a1f8 100644 --- a/core/src/router/build.ts +++ b/core/src/router/build.ts @@ -54,20 +54,16 @@ export const buildRouter = (baseParams: BaseRouterParams) => }) }) - try { - const output = await router.callHandler({ - params, - handlerType: "build", - defaultHandler: async () => ({ state: "unknown" as const, outputs: {}, detail: {} }), - }) - const { result } = output + const output = await router.callHandler({ + params, + handlerType: "build", + defaultHandler: async () => ({ state: "unknown" as const, outputs: {}, detail: {} }), + }) + const { result } = output - await router.validateActionOutputs(action, "runtime", result.outputs) + await router.validateActionOutputs(action, "runtime", result.outputs) - return output - } catch (err) { - throw err - } + return output }, publish: async (params) => { diff --git a/core/src/util/ext-tools.ts b/core/src/util/ext-tools.ts index 5336fb6267..ab1a01363b 100644 --- a/core/src/util/ext-tools.ts +++ b/core/src/util/ext-tools.ts @@ -63,6 +63,10 @@ export class CliWrapper { return this.toolPath } + /** + * @throws RuntimeError on EMFILE (Too many open files) + * @throws ChildProcessError on any other error condition + */ async exec({ args, cwd, env, log, timeoutSec, input, ignoreError, stdout, stderr }: ExecParams) { const path = await this.getPath(log) @@ -86,19 +90,19 @@ export class CliWrapper { }) } + /** + * @throws RuntimeError on EMFILE (Too many open files) + * @throws ChildProcessError on any other error condition + */ async stdout(params: ExecParams) { - try { - const res = await this.exec(params) - return res.stdout - } catch (err) { - // Add log output to error - if (err.all) { - err.message += "\n\n" + err.all - } - throw err - } + const res = await this.exec(params) + return res.stdout } + /** + * @throws RuntimeError on EMFILE (Too many open files) + * @throws ChildProcessError on any other error condition + */ async json(params: ExecParams) { const out = await this.stdout(params) return JSON.parse(out) @@ -122,6 +126,8 @@ export class CliWrapper { * Helper for using spawn with live log streaming. Waits for the command to finish before returning. * * If an error occurs and no output has been written to stderr, we use stdout for the error message instead. + * + * @throws RuntimeError */ async spawnAndStreamLogs({ args, @@ -145,6 +151,10 @@ export class CliWrapper { }) } + /** + * @throws RuntimeError on ENOENT (command not found) + * @throws ChildProcessError on any other error condition + */ async spawnAndWait({ args, cwd, env, log, ignoreError, rawMode, stdout, stderr, timeoutSec, tty }: SpawnParams) { const path = await this.getPath(log) diff --git a/core/src/util/util.ts b/core/src/util/util.ts index c862a83ab5..a640159514 100644 --- a/core/src/util/util.ts +++ b/core/src/util/util.ts @@ -225,6 +225,9 @@ export interface ExecOpts extends execa.Options { * Enforces `buffer: true` (which is the default execa behavior). * * Also adds the ability to pipe stdout|stderr to an output stream. + * + * @throws RuntimeError on EMFILE (Too many open files) + * @throws ChildProcessError on any other error condition */ export async function exec(cmd: string, args: string[], opts: ExecOpts = {}) { // Ensure buffer is always set to true so that we can read the error output @@ -266,6 +269,8 @@ export async function exec(cmd: string, args: string[], opts: ExecOpts = {}) { args, code: error.exitCode || err.code || err.errno, output: error.all || error.stdout || error.stderr || "", + stderr: error.stderr || "", + stdout: error.stdout || "", }, }) } @@ -297,6 +302,9 @@ export interface SpawnOutput { /** * Note: For line-by-line stdout/stderr streaming, both `opts.stdout` and `opts.stderr` must be defined. This is a * result of how Node's child process spawning works (which this function wraps). + * + * @throws RuntimeError on ENOENT (command not found) + * @throws ChildProcessError on any other error condition */ export function spawn(cmd: string, args: string[], opts: SpawnOpts = {}) { const { @@ -405,7 +413,15 @@ export function spawn(cmd: string, args: string[], opts: SpawnOpts = {}) { _reject( new ChildProcessError({ message: msg, - detail: { cmd, args, opts, code: result.code, output: result.all }, + detail: { + cmd, + args, + opts, + code: result.code, + output: result.all, + stderr: result.stderr, + stdout: result.stdout, + }, }) ) } diff --git a/core/src/vcs/git.ts b/core/src/vcs/git.ts index a130b874b2..bd6a4bca5b 100644 --- a/core/src/vcs/git.ts +++ b/core/src/vcs/git.ts @@ -12,7 +12,7 @@ import { isString } from "lodash" import { createReadStream, ensureDir, lstat, pathExists, readlink, realpath, stat, Stats } from "fs-extra" import { PassThrough } from "stream" import { GetFilesParams, RemoteSourceParams, VcsFile, VcsHandler, VcsHandlerParams, VcsInfo } from "./vcs" -import { ConfigurationError, RuntimeError } from "../exceptions" +import { ChildProcessError, ConfigurationError, RuntimeError } from "../exceptions" import { getStatsType, joinWithPosix, matchPath } from "../util/fs" import { dedent, deline, splitLast } from "../util/string" import { defer, exec } from "../util/util" @@ -88,6 +88,9 @@ export class GitHandler extends VcsHandler { } gitCli(log: Log, cwd: string, failOnPrompt = false): GitCli { + /** + * @throws ChildProcessError + */ return async (...args: (string | undefined)[]) => { log.silly(`Calling git with args '${args.join(" ")}' in ${cwd}`) const { stdout } = await exec("git", args.filter(isString), { @@ -103,7 +106,7 @@ export class GitHandler extends VcsHandler { try { return await git("diff-index", "--name-only", "HEAD", path) } catch (err) { - if (err.exitCode === 128) { + if (err instanceof ChildProcessError && err.detail.code === 128) { // no commit in repo return [] } else { @@ -165,8 +168,12 @@ export class GitHandler extends VcsHandler { await git("status") gitSafeDirs.add(path) } catch (err) { + if (!(err instanceof ChildProcessError)) { + throw err + } + // Git has stricter repo ownerships checks since 2.36.0 - if (err.exitCode === 128 && err.stderr?.toLowerCase().includes("fatal: unsafe repository")) { + if (err.detail.code === 128 && err.detail.stderr.toLowerCase().includes("fatal: unsafe repository")) { log.warn( chalk.yellow( `It looks like you're using Git 2.36.0 or newer and the directory "${path}" is owned by someone else. It will be added to safe.directory list in the .gitconfig.` @@ -187,11 +194,11 @@ export class GitHandler extends VcsHandler { } return - } else if (err.exitCode === 128 && err.stderr?.toLowerCase().includes("fatal: not a git repository")) { + } else if (err.detail.code === 128 && err.detail.stderr.toLowerCase().includes("fatal: not a git repository")) { throw new RuntimeError({ message: notInRepoRootErrorMessage(path), detail: { path } }) } else { log.error( - `Unexpected Git error occurred while running 'git status' from path "${path}". Exit code: ${err.exitCode}. Error message: ${err.stderr}` + `Unexpected Git error occurred while running 'git status' from path "${path}". Exit code: ${err.detail.code}. Error message: ${err.detail.stderr}` ) throw err } @@ -224,17 +231,23 @@ export class GitHandler extends VcsHandler { this.repoRoots.set(path, repoRoot) return repoRoot } catch (err) { - if (err.exitCode === 128 && err.stderr?.toLowerCase().includes("fatal: unsafe repository")) { + if (!(err instanceof ChildProcessError)) { + throw err + } + if (err.detail.code === 128 && err.detail.stderr.toLowerCase().includes("fatal: unsafe repository")) { // Throw nice error when we detect that we're not in a repo root throw new RuntimeError({ message: - err.stderr + + err.detail.stderr + `\nIt looks like you're using Git 2.36.0 or newer and the repo directory containing "${path}" is owned by someone else. If this is intentional you can run "git config --global --add safe.directory ''" and try again.`, detail: { path }, }) - } else if (err.exitCode === 128) { + } else if (err.detail.code === 128) { // Throw nice error when we detect that we're not in a repo root - throw new RuntimeError({ message: notInRepoRootErrorMessage(path), detail: { path, exitCode: err.exitCode } }) + throw new RuntimeError({ + message: notInRepoRootErrorMessage(path), + detail: { path, exitCode: err.detail.code }, + }) } else { throw err } @@ -291,8 +304,7 @@ export class GitHandler extends VcsHandler { return [] } } catch (err) { - // 128 = File no longer exists - if (err.exitCode === 128 || err.code === "ENOENT") { + if (err.code === "ENOENT") { gitLog.warn(`Attempted to scan directory at ${path}, but it does not exist.`) return [] } else { @@ -386,8 +398,7 @@ export class GitHandler extends VcsHandler { return [] } } catch (err) { - // 128 = File no longer exists - if (err.exitCode === 128 || err.code === "ENOENT") { + if (err.code === "ENOENT") { gitLog.warn( `Found reference to submodule at ${submoduleRelPath}, but the path could not be found. ${submoduleErrorSuggestion}` ) @@ -783,7 +794,7 @@ export class GitHandler extends VcsHandler { output.branch = (await git("rev-parse", "--abbrev-ref", "HEAD"))[0] output.commitHash = (await git("rev-parse", "HEAD"))[0] } catch (err) { - if (err.exitCode !== 128) { + if (err instanceof ChildProcessError && err.detail.code !== 128) { throw err } } diff --git a/core/test/unit/src/util/util.ts b/core/test/unit/src/util/util.ts index e11009dde9..f166e27756 100644 --- a/core/test/unit/src/util/util.ts +++ b/core/test/unit/src/util/util.ts @@ -24,6 +24,7 @@ import { splitLast, splitFirst } from "../../../../src/util/string" import { getRootLogger } from "../../../../src/logger/logger" import { dedent } from "../../../../src/util/string" import { safeDumpYaml } from "../../../../src/util/serialization" +import { ChildProcessError } from "../../../../src/exceptions" function isLinuxOrDarwin() { return process.platform === "darwin" || process.platform === "linux" @@ -153,6 +154,8 @@ describe("util", () => { // Using "sh -c" to get consistent output between operating systems await exec(`sh -c "echo hello error; exit 1"`, [], { shell: true }) } catch (err) { + expect(err).to.be.instanceOf(ChildProcessError) + expect(err.type).to.eql("childprocess") expect(err.message).to.equal( makeErrorMsg({ code: 1, @@ -178,6 +181,10 @@ describe("util", () => { try { await spawn("ls", ["scottiepippen"]) } catch (err) { + // Spawn does not throw ChildProcessError at the moment. + expect(err).to.be.instanceOf(ChildProcessError) + expect(err.type).to.eql("childprocess") + // We're not using "sh -c" here since the output is not added to stdout|stderr if `tty: true` and // we therefore can't test the entire error message. if (process.platform === "darwin") { From b26541a8b35891234d27ae0246699893e9585c22 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 5 Sep 2023 18:24:26 +0200 Subject: [PATCH 17/17] improvement: remove "sorry" --- .github/ISSUE_TEMPLATE/CRASH.md | 2 +- core/src/exceptions.ts | 2 +- core/test/unit/src/cli/cli.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/CRASH.md b/.github/ISSUE_TEMPLATE/CRASH.md index c860f1dca7..22bc536637 100644 --- a/.github/ISSUE_TEMPLATE/CRASH.md +++ b/.github/ISSUE_TEMPLATE/CRASH.md @@ -9,7 +9,7 @@ assignees: '' ## Crash report - + ### Error message diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts index 8c8bd5041b..d3b6023651 100644 --- a/core/src/exceptions.ts +++ b/core/src/exceptions.ts @@ -243,7 +243,7 @@ export function explainGardenError(rawError: GardenError | Error | string, conte } return chalk.red(dedent` - ${chalk.bold("Encountered an unexpected Garden error. We are sorry for this. This is likely a bug šŸ‚")} + ${chalk.bold("Encountered an unexpected Garden error. This is likely a bug šŸ‚")} You can help by reporting this on GitHub: ${getGitHubIssueLink(`Crash: ${errorMessage}`, "crash")} diff --git a/core/test/unit/src/cli/cli.ts b/core/test/unit/src/cli/cli.ts index e3c5a01c0d..0d505284cd 100644 --- a/core/test/unit/src/cli/cli.ts +++ b/core/test/unit/src/cli/cli.ts @@ -971,7 +971,7 @@ describe("cli", () => { const firstSevenLines = outputLines.slice(0, 7).join("\n") expect(firstSevenLines).to.eql(dedent` - Encountered an unexpected Garden error. We are sorry for this. This is likely a bug šŸ‚ + Encountered an unexpected Garden error. This is likely a bug šŸ‚ You can help by reporting this on GitHub: https://github.com/garden-io/garden/issues/new?labels=bug,crash&template=CRASH.md&title=Crash%3A%20Cannot%20read%20property%20foo%20of%20undefined.