From b4e0d62658f13010f0574c5b40f5b8fb317c2e7d Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Mon, 23 Mar 2020 11:48:16 -0700 Subject: [PATCH 1/9] stage --- src/commands/infra/generate.ts | 7 +- src/commands/infra/scaffold.ts | 53 +++++++------- src/lib/errorBuilder.ts | 96 +++++++++++++++++++++++++ src/lib/errorStatusCode.ts | 14 ++++ src/lib/i18n.json | 11 +++ src/lib/pipelines/variableGroup.test.ts | 5 +- 6 files changed, 153 insertions(+), 33 deletions(-) create mode 100644 src/lib/errorBuilder.ts create mode 100644 src/lib/errorStatusCode.ts diff --git a/src/commands/infra/generate.ts b/src/commands/infra/generate.ts index 10421a9d8..9a1cc60c6 100644 --- a/src/commands/infra/generate.ts +++ b/src/commands/infra/generate.ts @@ -22,6 +22,7 @@ import { spkTemplatesPath, } from "./infra_common"; import { copyTfTemplate } from "./scaffold"; +import { build as buildError } from "../../lib/errorBuilder"; interface CommandOptions { project: string | undefined; @@ -287,12 +288,10 @@ export const validateRemoteSource = async ( version ); } else { - throw new Error( - `Unable to determine error from supported retry cases ${err.message}` - ); + throw buildError(1100, "infra-103", err); } } catch (retryError) { - throw new Error(`Failure error thrown during retry ${retryError}`); + throw buildError(1100, "infra-104", err); } } } diff --git a/src/commands/infra/scaffold.ts b/src/commands/infra/scaffold.ts index 682a2cffe..2b5fdefe7 100644 --- a/src/commands/infra/scaffold.ts +++ b/src/commands/infra/scaffold.ts @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/no-non-null-assertion */ import commander from "commander"; import fs from "fs"; import fsextra from "fs-extra"; @@ -19,6 +18,7 @@ import { VARIABLES_TF, } from "./infra_common"; import decorator from "./scaffold.decorator.json"; +import { build as buildError, log as logError } from "../../lib/errorBuilder"; export interface CommandOptions { name: string; @@ -47,11 +47,11 @@ template repo and access token was not specified in spk-config.yml. Checking pas if (!opts.source) { // since access_token and infra_repository are missing, we cannot construct source for them - throw new Error("Value for source is missing."); + throw buildError(1001, "infra-101"); } } if (!opts.name || !opts.version || !opts.template) { - throw new Error("Values for name, version and/or 'template are missing."); + throw buildError(1001, "infra-102"); } logger.info(`All required options are configured via command line for \ scaffolding, expecting public remote repository for terraform templates \ @@ -60,6 +60,8 @@ or PAT embedded in source URL.`); // Construct the source based on the the passed configurations of spk-config.yaml export const constructSource = (config: ConfigYaml): string => { + // config.azure_devops exists because validateValues function checks it + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const devops = config.azure_devops!; const source = `https://spk:${devops.access_token}@${devops.infra_repository}`; logger.info( @@ -93,10 +95,14 @@ export const copyTfTemplate = async ( } logger.info(`Terraform template files copied from ${templatePath}`); } catch (err) { - logger.error( - `Unable to find Terraform environment. Please check template path.` + throw buildError( + 1010, + { + errorKey: "infra-105", + values: [templatePath], + }, + err ); - throw err; } }; @@ -106,18 +112,15 @@ export const copyTfTemplate = async ( * @param templatePath Path to the variables.tf file */ export const validateVariablesTf = (templatePath: string): void => { - try { - if (!fs.existsSync(templatePath)) { - throw new Error( - `Provided Terraform ${VARIABLES_TF} path is invalid or cannot be found: ${templatePath}` - ); - } - logger.info( - `Terraform ${VARIABLES_TF} file found. Attempting to generate ${DEFINITION_YAML} file.` - ); - } catch (_) { - throw new Error(`Unable to validate Terraform ${VARIABLES_TF}.`); + if (!fs.existsSync(templatePath)) { + throw buildError(1010, { + errorKey: "infra-106", + values: [VARIABLES_TF, templatePath], + }); } + logger.info( + `Terraform ${VARIABLES_TF} file found. Attempting to generate ${DEFINITION_YAML} file.` + ); }; /** @@ -281,15 +284,14 @@ export const scaffold = (values: CommandOptions): void => { }); fs.writeFileSync(confPath, definitionYaml, "utf8"); } else { - logger.error(`Unable to generate cluster definition.`); + throw Error(`Unable to generate cluster definition.`); } } else { - logger.error(`Unable to read variable file: ${tfVariableFile}.`); + throw Error(`Unable to read variable file: ${tfVariableFile}.`); } } } catch (err) { - logger.warn("Unable to create scaffold"); - throw err; + throw buildError(1002, "infra-107", err); } }; @@ -308,8 +310,10 @@ export const removeTemplateFiles = (envPath: string): void => { fs.unlinkSync(path.join(envPath, f)); }); } catch (e) { - logger.error(`cannot read ${envPath}`); - // TOFIX: I guess we are ok with files not removed. + throw buildError(1002, { + errorKey: "infra-108", + values: [envPath], + }); } }; @@ -342,8 +346,7 @@ export const execute = async ( removeTemplateFiles(opts.name); await exitFn(0); } catch (err) { - logger.error("Error occurred while generating scaffold"); - logger.error(err); + logError(buildError(1000, "infra-100", err)); await exitFn(1); } }; diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts new file mode 100644 index 000000000..332e5fa03 --- /dev/null +++ b/src/lib/errorBuilder.ts @@ -0,0 +1,96 @@ +import i18n from "./i18n.json"; +import { isValid } from "./errorStatusCode"; +import { logger } from "../logger"; + +const errors: { [key: string]: string } = i18n.errors; + +interface ErrorChain { + spkStatusCode: number; + message: string; + details?: string; + parent?: ErrorChain; +} + +interface ErrorParam { + errorKey: string; + values: string[]; +} + +export const getErrorMessage = (errorInstance: string | ErrorParam): string => { + let key = ""; + let values: string[] | undefined = undefined; + + if (typeof errorInstance === "object") { + key = errorInstance.errorKey; + values = errorInstance.values; + } else { + key = errorInstance; + } + + if (key in errors) { + let results = errors[key]; + if (values) { + values.forEach((val, i) => { + const re = new RegExp("\\{" + i + "}", "g"); + results = results.replace(re, val); + }); + } + return `${key}: ${results}`; + } + return key; +}; + +export const build = ( + code: number, + errorKey: string | ErrorParam, + error?: Error | ErrorChain +): ErrorChain => { + if (!isValid(code)) { + throw Error(`invalid ststus code, ${code}`); + } + + const oError: ErrorChain = { + spkStatusCode: code, + message: getErrorMessage(errorKey), + }; + + if (error) { + if ("spkStatusCode" in error) { + oError.parent = error as ErrorChain; + } else { + const e = error as Error; + oError.details = e ? e.message : ""; + } + } + + return oError; +}; + +export const message = ( + error: ErrorChain, + messages: string[], + padding?: string +): void => { + padding = padding || ""; + let mes = + `${padding}code: ${error.spkStatusCode}\n` + + `${padding}message: ${error.message}`; + if (error.details) { + mes += `\n${padding}details: ${error.details}`; + } + + messages.push(mes); + if (error.parent) { + message(error.parent, messages, padding + " "); + } +}; + +export const log = (err: Error | ErrorChain): void => { + if ("spkStatusCode" in err) { + const messages: string[] = []; + message(err as ErrorChain, messages); + logger.error("\n" + messages.join("\n")); + } else { + logger.error(err); + } +}; diff --git a/src/lib/errorStatusCode.ts b/src/lib/errorStatusCode.ts new file mode 100644 index 000000000..e615266e0 --- /dev/null +++ b/src/lib/errorStatusCode.ts @@ -0,0 +1,14 @@ +// please do not change the status code numbers +// you can add new ones but not changing the existing ones + +export const errorStatusCode = { + 1000: "command fails to execute", + 1001: "validation error", + 1002: "execution error", + 1010: "environment related error", + 1100: "git operation related error", +}; + +export const isValid = (code: number): boolean => { + return code in errorStatusCode; +}; diff --git a/src/lib/i18n.json b/src/lib/i18n.json index f39b6e95c..b254d0c74 100644 --- a/src/lib/i18n.json +++ b/src/lib/i18n.json @@ -14,5 +14,16 @@ "storagePartitionKey": "Enter storage partition key", "storageAccessKey": "Enter storage access key", "storageKeVaultName": "Enter key vault name (have the value as empty and hit enter key to skip)" + }, + "errors": { + "infra-100": "Scaffold Command was not successfully executed.", + "infra-101": "Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", + "infra-102": "Values for name, version and/or 'template were missing. Provide value for values for them.", + "infra-103": "Unable to determine error when validating remote git source.", + "infra-104": "Failure error thrown during retrying validating remote git source.", + "infra-105": "Unable to find Terraform environment. Ensure template path {0} exists.", + "infra-106": "Provided Terraform {0} path is invalid or cannot be found: {1}", + "infra-107": "Unable to create scaffold", + "infra-108": "Unable to read {0}" } } diff --git a/src/lib/pipelines/variableGroup.test.ts b/src/lib/pipelines/variableGroup.test.ts index bd0568506..970759018 100644 --- a/src/lib/pipelines/variableGroup.test.ts +++ b/src/lib/pipelines/variableGroup.test.ts @@ -1,8 +1,5 @@ /* eslint-disable @typescript-eslint/camelcase */ -import { - VariableGroup, - VariableGroupParameters, -} from "azure-devops-node-api/interfaces/TaskAgentInterfaces"; +import { VariableGroupParameters } from "azure-devops-node-api/interfaces/TaskAgentInterfaces"; import uuid from "uuid/v4"; import * as azdoClient from "../azdoClient"; import { readYaml } from "../../config"; From 593780d2d989a27204407d61ddecbca6964c48f9 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Mon, 23 Mar 2020 13:30:47 -0700 Subject: [PATCH 2/9] fix tests --- src/commands/infra/generate.test.ts | 4 +--- src/commands/infra/scaffold.test.ts | 11 +++-------- src/commands/infra/scaffold.ts | 6 ++---- src/lib/i18n.json | 3 +-- 4 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/commands/infra/generate.test.ts b/src/commands/infra/generate.test.ts index a7142bbf3..0fba66378 100644 --- a/src/commands/infra/generate.test.ts +++ b/src/commands/infra/generate.test.ts @@ -396,9 +396,7 @@ describe("test validateRemoteSource function", () => { }); expect(true).toBe(false); } catch (err) { - expect(err.message).toBe( - "Failure error thrown during retry Error: Unable to determine error from supported retry cases other error" - ); + expect(err.spkStatusCode).toBe(1100); } }); }); diff --git a/src/commands/infra/scaffold.test.ts b/src/commands/infra/scaffold.test.ts index 943e6c438..0dc0bf2ec 100644 --- a/src/commands/infra/scaffold.test.ts +++ b/src/commands/infra/scaffold.test.ts @@ -171,12 +171,12 @@ describe("test validate function", () => { ); expect(true).toBe(false); } catch (err) { - expect(err.message).toBe("Value for source is missing."); + expect(err.spkStatusCode).toBe(1001); } }); it("name, template, version is missing", () => { ["name", "template", "version"].forEach((key) => { - try { + expect(() => { validateValues( {}, { @@ -186,12 +186,7 @@ describe("test validate function", () => { version: key === "version" ? "" : uuid(), } ); - expect(true).toBe(false); - } catch (err) { - expect(err.message).toBe( - "Values for name, version and/or 'template are missing." - ); - } + }).toThrow(); }); }); }); diff --git a/src/commands/infra/scaffold.ts b/src/commands/infra/scaffold.ts index 2b5fdefe7..df5853cda 100644 --- a/src/commands/infra/scaffold.ts +++ b/src/commands/infra/scaffold.ts @@ -310,10 +310,8 @@ export const removeTemplateFiles = (envPath: string): void => { fs.unlinkSync(path.join(envPath, f)); }); } catch (e) { - throw buildError(1002, { - errorKey: "infra-108", - values: [envPath], - }); + logger.warn(`cannot read ${envPath}`); + // TOFIX: I guess we are ok with files not removed. } }; diff --git a/src/lib/i18n.json b/src/lib/i18n.json index b254d0c74..65ec0ff9e 100644 --- a/src/lib/i18n.json +++ b/src/lib/i18n.json @@ -23,7 +23,6 @@ "infra-104": "Failure error thrown during retrying validating remote git source.", "infra-105": "Unable to find Terraform environment. Ensure template path {0} exists.", "infra-106": "Provided Terraform {0} path is invalid or cannot be found: {1}", - "infra-107": "Unable to create scaffold", - "infra-108": "Unable to read {0}" + "infra-107": "Unable to create scaffold" } } From 06d125e65f1af42123aec7ea1f0f3eb8d2680939 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Mon, 23 Mar 2020 14:33:49 -0700 Subject: [PATCH 3/9] adding tests --- src/lib/errorBuilder.test.ts | 107 +++++++++++++++++++++++++++++++ src/lib/errorBuilder.ts | 40 +++++++++++- src/lib/setup/pipelineService.ts | 4 +- 3 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 src/lib/errorBuilder.test.ts diff --git a/src/lib/errorBuilder.test.ts b/src/lib/errorBuilder.test.ts new file mode 100644 index 000000000..010251d4c --- /dev/null +++ b/src/lib/errorBuilder.test.ts @@ -0,0 +1,107 @@ +import { build, getErrorMessage, log, message } from "./errorBuilder"; +import * as errorBuilder from "./errorBuilder"; + +describe("test getErrorMessage function", () => { + it("positive test: string", () => { + expect(getErrorMessage("infra-100")).toBe( + "infra-100: Scaffold Command was not successfully executed." + ); + }); + it("positive test: object", () => { + expect( + getErrorMessage({ + errorKey: "infra-105", + values: ["test"], + }) + ).toBe( + "infra-105: Unable to find Terraform environment. Ensure template path test exists." + ); + }); + it("negative test: invalid test", () => { + expect(getErrorMessage("someunknownkey")).toBe("someunknownkey"); + }); +}); + +describe("test build function", () => { + it("negative test: invalid code", () => { + expect(() => { + build(-1, "infra-1000"); + }).toThrow(); + }); + it("positive test: without error", () => { + const err = build(1000, "infra-100"); + expect(err.spkStatusCode).toBe(1000); + expect(err.message).toBe( + "infra-100: Scaffold Command was not successfully executed." + ); + expect(err.details).toBeUndefined(); + expect(err.parent).toBeUndefined(); + }); + it("positive test: with Error", () => { + const err = build(1000, "infra-100", Error("test")); + expect(err.spkStatusCode).toBe(1000); + expect(err.message).toBe( + "infra-100: Scaffold Command was not successfully executed." + ); + expect(err.details).toBe("test"); + expect(err.parent).toBeUndefined(); + }); + it("positive test: with ErrorChain", () => { + const e = build(1000, "infra-101"); + const err = build(1000, "infra-100", e); + expect(err.spkStatusCode).toBe(1000); + expect(err.message).toBe( + "infra-100: Scaffold Command was not successfully executed." + ); + expect(err.details).toBeUndefined(); + expect(err.parent).toStrictEqual(e); + }); +}); + +describe("test message function", () => { + it("positive test: one error chain", () => { + const messages: string[] = []; + const e = build(1000, "infra-101"); + message(e, messages); + expect(messages).toStrictEqual([ + "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", + ]); + }); + it("positive test: one error chain with details", () => { + const messages: string[] = []; + const e = build(1000, "infra-101", Error("test message")); + message(e, messages); + expect(messages).toStrictEqual([ + "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.\ndetails: test message", + ]); + }); + it("positive test: multiple error chains", () => { + const messages: string[] = []; + const e = build( + 1000, + "infra-101", + build(1001, "infra-102", build(1010, "infra-103")) + ); + message(e, messages); + expect(messages).toStrictEqual([ + "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", + " code: 1001\n message: infra-102: Values for name, version and/or 'template were missing. Provide value for values for them.", + " code: 1010\n message: infra-103: Unable to determine error when validating remote git source.", + ]); + }); +}); + +describe("test log function", () => { + it("test: Error chain object", () => { + const fnMessage = jest.spyOn(errorBuilder, "message"); + fnMessage.mockReset(); + log(build(1000, "infra=100")); + expect(fnMessage).toBeCalledTimes(1); + }); + it("test: Error object", () => { + const fnMessage = jest.spyOn(errorBuilder, "message"); + fnMessage.mockReset(); + log(Error("test message")); + expect(fnMessage).toBeCalledTimes(0); + }); +}); diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts index 332e5fa03..b0ae11a8d 100644 --- a/src/lib/errorBuilder.ts +++ b/src/lib/errorBuilder.ts @@ -16,6 +16,16 @@ interface ErrorParam { values: string[]; } +const isErrorChainObject = (o: Error | ErrorChain): boolean => { + return "spkStatusCode" in o; +}; + +/** + * Returns formatted error message. + * + * @param errorInstance string or ErrorParam. latter is for + * string substitution in error message + */ export const getErrorMessage = (errorInstance: string | ErrorParam): string => { let key = ""; let values: string[] | undefined = undefined; @@ -40,13 +50,25 @@ export const getErrorMessage = (errorInstance: string | ErrorParam): string => { return key; }; +/** + * Builds an error object + * + * @param code spk error code + * @param errorKey Error key. e.g. "infra-101" or can be an object + * to support string substitution in error message + * { + * errorKey: "infra-105", + * values: ["someValue"] + * } + * @param error: Parent error object. + */ export const build = ( code: number, errorKey: string | ErrorParam, error?: Error | ErrorChain ): ErrorChain => { if (!isValid(code)) { - throw Error(`invalid ststus code, ${code}`); + throw Error(`Invalid status code, ${code}`); } const oError: ErrorChain = { @@ -55,7 +77,7 @@ export const build = ( }; if (error) { - if ("spkStatusCode" in error) { + if (isErrorChainObject(error)) { oError.parent = error as ErrorChain; } else { const e = error as Error; @@ -66,6 +88,13 @@ export const build = ( return oError; }; +/** + * Generates error messages and have them in messages array. + * + * @param error Error object + * @param messages string of messages + * @param padding Padding to be added to the beginning of messages + */ export const message = ( error: ErrorChain, messages: string[], @@ -85,8 +114,13 @@ export const message = ( } }; +/** + * Writing error log. + * + * @param err Error object + */ export const log = (err: Error | ErrorChain): void => { - if ("spkStatusCode" in err) { + if (isErrorChainObject(err)) { const messages: string[] = []; message(err as ErrorChain, messages); logger.error("\n" + messages.join("\n")); diff --git a/src/lib/setup/pipelineService.ts b/src/lib/setup/pipelineService.ts index 6f668f427..972edcf51 100644 --- a/src/lib/setup/pipelineService.ts +++ b/src/lib/setup/pipelineService.ts @@ -109,7 +109,7 @@ export const deletePipeline = async ( }; /** - * Returns latest build ststus of pipeline. + * Returns latest build status of pipeline. * * @param buildApi Build API client * @param projectName Project name @@ -130,7 +130,7 @@ export const getPipelineBuild = async ( }; /** - * Polls build ststus of pipeline. + * Polls build status of pipeline. * * @param buildApi Build API client * @param projectName Project name From cbf443bea2cd00f01af4272071f69f76612ef740 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Mon, 23 Mar 2020 16:56:14 -0700 Subject: [PATCH 4/9] extending of Error class --- src/commands/infra/generate.test.ts | 2 +- src/commands/infra/generate.ts | 10 ++- src/commands/infra/scaffold.test.ts | 2 +- src/lib/errorBuilder.test.ts | 51 ++++++----- src/lib/errorBuilder.ts | 126 +++++++++++++--------------- src/lib/i18n.json | 4 +- 6 files changed, 95 insertions(+), 100 deletions(-) diff --git a/src/commands/infra/generate.test.ts b/src/commands/infra/generate.test.ts index 0fba66378..974d13f9e 100644 --- a/src/commands/infra/generate.test.ts +++ b/src/commands/infra/generate.test.ts @@ -396,7 +396,7 @@ describe("test validateRemoteSource function", () => { }); expect(true).toBe(false); } catch (err) { - expect(err.spkStatusCode).toBe(1100); + expect(err.errorCode).toBe(1100); } }); }); diff --git a/src/commands/infra/generate.ts b/src/commands/infra/generate.ts index 9a1cc60c6..c42a26850 100644 --- a/src/commands/infra/generate.ts +++ b/src/commands/infra/generate.ts @@ -183,14 +183,16 @@ export const checkRemoteGitExist = async ( ): Promise => { // Checking for git remote if (!fs.existsSync(sourcePath)) { - throw new Error(`${sourcePath} does not exist`); + throw buildError(1100, { + errorKey: "infra-109", + values: [sourcePath], + }); } const result = await simpleGit(sourcePath).listRemote([source]); if (!result) { logger.error(result); - throw new Error(`Unable to clone the source remote repository. \ -The remote repo may not exist or you do not have the rights to access it`); + throw buildError(1100, "infra-108"); } logger.info(`Remote source repo: ${safeLoggingUrl} exists.`); @@ -293,6 +295,8 @@ export const validateRemoteSource = async ( } catch (retryError) { throw buildError(1100, "infra-104", err); } + } else { + throw err; } } }; diff --git a/src/commands/infra/scaffold.test.ts b/src/commands/infra/scaffold.test.ts index 0dc0bf2ec..90cc57d73 100644 --- a/src/commands/infra/scaffold.test.ts +++ b/src/commands/infra/scaffold.test.ts @@ -171,7 +171,7 @@ describe("test validate function", () => { ); expect(true).toBe(false); } catch (err) { - expect(err.spkStatusCode).toBe(1001); + expect(err.errorCode).toBe(1001); } }); it("name, template, version is missing", () => { diff --git a/src/lib/errorBuilder.test.ts b/src/lib/errorBuilder.test.ts index 010251d4c..44feafc9a 100644 --- a/src/lib/errorBuilder.test.ts +++ b/src/lib/errorBuilder.test.ts @@ -1,24 +1,24 @@ -import { build, getErrorMessage, log, message } from "./errorBuilder"; -import * as errorBuilder from "./errorBuilder"; +import { build, log } from "./errorBuilder"; describe("test getErrorMessage function", () => { it("positive test: string", () => { - expect(getErrorMessage("infra-100")).toBe( + const oErr = build(1000, "infra-100"); + expect(oErr.message).toBe( "infra-100: Scaffold Command was not successfully executed." ); }); it("positive test: object", () => { - expect( - getErrorMessage({ - errorKey: "infra-105", - values: ["test"], - }) - ).toBe( + const oErr = build(1000, { + errorKey: "infra-105", + values: ["test"], + }); + expect(oErr.message).toBe( "infra-105: Unable to find Terraform environment. Ensure template path test exists." ); }); it("negative test: invalid test", () => { - expect(getErrorMessage("someunknownkey")).toBe("someunknownkey"); + const oErr = build(1000, "infra-100xxxxx"); + expect(oErr.message).toBe("infra-100xxxxx"); }); }); @@ -30,7 +30,7 @@ describe("test build function", () => { }); it("positive test: without error", () => { const err = build(1000, "infra-100"); - expect(err.spkStatusCode).toBe(1000); + expect(err.errorCode).toBe(1000); expect(err.message).toBe( "infra-100: Scaffold Command was not successfully executed." ); @@ -39,7 +39,7 @@ describe("test build function", () => { }); it("positive test: with Error", () => { const err = build(1000, "infra-100", Error("test")); - expect(err.spkStatusCode).toBe(1000); + expect(err.errorCode).toBe(1000); expect(err.message).toBe( "infra-100: Scaffold Command was not successfully executed." ); @@ -49,7 +49,7 @@ describe("test build function", () => { it("positive test: with ErrorChain", () => { const e = build(1000, "infra-101"); const err = build(1000, "infra-100", e); - expect(err.spkStatusCode).toBe(1000); + expect(err.errorCode).toBe(1000); expect(err.message).toBe( "infra-100: Scaffold Command was not successfully executed." ); @@ -61,28 +61,28 @@ describe("test build function", () => { describe("test message function", () => { it("positive test: one error chain", () => { const messages: string[] = []; - const e = build(1000, "infra-101"); - message(e, messages); + const oError = build(1000, "infra-101"); + oError.messages(messages); expect(messages).toStrictEqual([ "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", ]); }); it("positive test: one error chain with details", () => { const messages: string[] = []; - const e = build(1000, "infra-101", Error("test message")); - message(e, messages); + const oError = build(1000, "infra-101", Error("test message")); + oError.messages(messages); expect(messages).toStrictEqual([ "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.\ndetails: test message", ]); }); it("positive test: multiple error chains", () => { const messages: string[] = []; - const e = build( + const oError = build( 1000, "infra-101", build(1001, "infra-102", build(1010, "infra-103")) ); - message(e, messages); + oError.messages(messages); expect(messages).toStrictEqual([ "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", " code: 1001\n message: infra-102: Values for name, version and/or 'template were missing. Provide value for values for them.", @@ -93,15 +93,12 @@ describe("test message function", () => { describe("test log function", () => { it("test: Error chain object", () => { - const fnMessage = jest.spyOn(errorBuilder, "message"); - fnMessage.mockReset(); - log(build(1000, "infra=100")); - expect(fnMessage).toBeCalledTimes(1); + const oError = build(1000, "infra-100"); + expect(log(oError)).toBe( + "\ncode: 1000\nmessage: infra-100: Scaffold Command was not successfully executed." + ); }); it("test: Error object", () => { - const fnMessage = jest.spyOn(errorBuilder, "message"); - fnMessage.mockReset(); - log(Error("test message")); - expect(fnMessage).toBeCalledTimes(0); + expect(log(Error("test message"))).toBe("test message"); }); }); diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts index b0ae11a8d..348362959 100644 --- a/src/lib/errorBuilder.ts +++ b/src/lib/errorBuilder.ts @@ -4,50 +4,68 @@ import { logger } from "../logger"; const errors: { [key: string]: string } = i18n.errors; -interface ErrorChain { - spkStatusCode: number; - message: string; - details?: string; - parent?: ErrorChain; -} - interface ErrorParam { errorKey: string; values: string[]; } +class ErrorChain extends Error { + errorCode: number; + details: string | undefined; + parent: ErrorChain | undefined; -const isErrorChainObject = (o: Error | ErrorChain): boolean => { - return "spkStatusCode" in o; -}; + constructor(code: number, errorInstance: string | ErrorParam) { + super(""); + this.errorCode = code; + this.message = this.getErrorMessage(errorInstance); + } + getErrorMessage(errorInstance: string | ErrorParam): string { + let key = ""; + let values: string[] | undefined = undefined; -/** - * Returns formatted error message. - * - * @param errorInstance string or ErrorParam. latter is for - * string substitution in error message - */ -export const getErrorMessage = (errorInstance: string | ErrorParam): string => { - let key = ""; - let values: string[] | undefined = undefined; + if (typeof errorInstance === "object") { + key = errorInstance.errorKey; + values = errorInstance.values; + } else { + key = errorInstance; + } - if (typeof errorInstance === "object") { - key = errorInstance.errorKey; - values = errorInstance.values; - } else { - key = errorInstance; + if (key in errors) { + let results = errors[key]; + if (values) { + values.forEach((val, i) => { + const re = new RegExp("\\{" + i + "}", "g"); + results = results.replace(re, val); + }); + } + return `${key}: ${results}`; + } + return key; } + /** + * Generates error messages and have them in messages array. + * + * @param error Error object + * @param messages string of messages + * @param padding Padding to be added to the beginning of messages + */ + messages(results: string[], padding?: string): void { + padding = padding || ""; + let mes = + `${padding}code: ${this.errorCode}\n` + + `${padding}message: ${this.message}`; + if (this.details) { + mes += `\n${padding}details: ${this.details}`; + } - if (key in errors) { - let results = errors[key]; - if (values) { - values.forEach((val, i) => { - const re = new RegExp("\\{" + i + "}", "g"); - results = results.replace(re, val); - }); + results.push(mes); + if (this.parent) { + this.parent.messages(results, padding + " "); } - return `${key}: ${results}`; } - return key; +} + +const isErrorChainObject = (o: Error | ErrorChain): boolean => { + return o instanceof ErrorChain; }; /** @@ -71,10 +89,7 @@ export const build = ( throw Error(`Invalid status code, ${code}`); } - const oError: ErrorChain = { - spkStatusCode: code, - message: getErrorMessage(errorKey), - }; + const oError = new ErrorChain(code, errorKey); if (error) { if (isErrorChainObject(error)) { @@ -88,43 +103,20 @@ export const build = ( return oError; }; -/** - * Generates error messages and have them in messages array. - * - * @param error Error object - * @param messages string of messages - * @param padding Padding to be added to the beginning of messages - */ -export const message = ( - error: ErrorChain, - messages: string[], - padding?: string -): void => { - padding = padding || ""; - let mes = - `${padding}code: ${error.spkStatusCode}\n` + - `${padding}message: ${error.message}`; - if (error.details) { - mes += `\n${padding}details: ${error.details}`; - } - - messages.push(mes); - if (error.parent) { - message(error.parent, messages, padding + " "); - } -}; - /** * Writing error log. * * @param err Error object */ -export const log = (err: Error | ErrorChain): void => { +export const log = (err: Error | ErrorChain): string => { if (isErrorChainObject(err)) { const messages: string[] = []; - message(err as ErrorChain, messages); - logger.error("\n" + messages.join("\n")); + (err as ErrorChain).messages(messages); + const msg = "\n" + messages.join("\n"); + logger.error(msg); + return msg; } else { - logger.error(err); + logger.error(err.message); + return err.message; } }; diff --git a/src/lib/i18n.json b/src/lib/i18n.json index 65ec0ff9e..ad472ca5e 100644 --- a/src/lib/i18n.json +++ b/src/lib/i18n.json @@ -23,6 +23,8 @@ "infra-104": "Failure error thrown during retrying validating remote git source.", "infra-105": "Unable to find Terraform environment. Ensure template path {0} exists.", "infra-106": "Provided Terraform {0} path is invalid or cannot be found: {1}", - "infra-107": "Unable to create scaffold" + "infra-107": "Unable to create scaffold", + "infra-108": "Unable to clone the source remote repository. The remote repo may not exist or you do not have the rights to access it", + "infra-109": "Source path, {0} did not exist." } } From 95b7b0586e9a7f98430d97de30b557fca75c6b1d Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Tue, 24 Mar 2020 12:24:13 -0700 Subject: [PATCH 5/9] change err numbering to err human readable words --- src/commands/infra/generate.ts | 8 ++--- src/commands/infra/scaffold.ts | 12 +++---- src/lib/errorBuilder.test.ts | 64 +++++++++++++++++++++------------- src/lib/errorBuilder.ts | 4 +-- src/lib/i18n.json | 20 +++++------ 5 files changed, 61 insertions(+), 47 deletions(-) diff --git a/src/commands/infra/generate.ts b/src/commands/infra/generate.ts index c42a26850..d4fee4c01 100644 --- a/src/commands/infra/generate.ts +++ b/src/commands/infra/generate.ts @@ -184,7 +184,7 @@ export const checkRemoteGitExist = async ( // Checking for git remote if (!fs.existsSync(sourcePath)) { throw buildError(1100, { - errorKey: "infra-109", + errorKey: "infra-git-source-no-exist", values: [sourcePath], }); } @@ -192,7 +192,7 @@ export const checkRemoteGitExist = async ( const result = await simpleGit(sourcePath).listRemote([source]); if (!result) { logger.error(result); - throw buildError(1100, "infra-108"); + throw buildError(1100, "infra-err-git-clone-failed"); } logger.info(`Remote source repo: ${safeLoggingUrl} exists.`); @@ -290,10 +290,10 @@ export const validateRemoteSource = async ( version ); } else { - throw buildError(1100, "infra-103", err); + throw buildError(1100, "infra-err-validating-remote-git", err); } } catch (retryError) { - throw buildError(1100, "infra-104", err); + throw buildError(1100, "infra-err-retry-validating-remote-git", err); } } else { throw err; diff --git a/src/commands/infra/scaffold.ts b/src/commands/infra/scaffold.ts index df5853cda..119e0105c 100644 --- a/src/commands/infra/scaffold.ts +++ b/src/commands/infra/scaffold.ts @@ -47,11 +47,11 @@ template repo and access token was not specified in spk-config.yml. Checking pas if (!opts.source) { // since access_token and infra_repository are missing, we cannot construct source for them - throw buildError(1001, "infra-101"); + throw buildError(1001, "infra-scaffold-cmd-src-missing"); } } if (!opts.name || !opts.version || !opts.template) { - throw buildError(1001, "infra-102"); + throw buildError(1001, "infra-scaffold-cmd-values-missing"); } logger.info(`All required options are configured via command line for \ scaffolding, expecting public remote repository for terraform templates \ @@ -98,7 +98,7 @@ export const copyTfTemplate = async ( throw buildError( 1010, { - errorKey: "infra-105", + errorKey: "infra-err-locate-tf-env", values: [templatePath], }, err @@ -114,7 +114,7 @@ export const copyTfTemplate = async ( export const validateVariablesTf = (templatePath: string): void => { if (!fs.existsSync(templatePath)) { throw buildError(1010, { - errorKey: "infra-106", + errorKey: "infra-err-tf-path-not-found", values: [VARIABLES_TF, templatePath], }); } @@ -291,7 +291,7 @@ export const scaffold = (values: CommandOptions): void => { } } } catch (err) { - throw buildError(1002, "infra-107", err); + throw buildError(1002, "infra-err-create-scaffold", err); } }; @@ -344,7 +344,7 @@ export const execute = async ( removeTemplateFiles(opts.name); await exitFn(0); } catch (err) { - logError(buildError(1000, "infra-100", err)); + logError(buildError(1000, "infra-scaffold-cmd-failed", err)); await exitFn(1); } }; diff --git a/src/lib/errorBuilder.test.ts b/src/lib/errorBuilder.test.ts index 44feafc9a..c3c9d1c0e 100644 --- a/src/lib/errorBuilder.test.ts +++ b/src/lib/errorBuilder.test.ts @@ -1,57 +1,63 @@ import { build, log } from "./errorBuilder"; +import i18n from "./i18n.json"; + +const errors = i18n.errors; describe("test getErrorMessage function", () => { it("positive test: string", () => { - const oErr = build(1000, "infra-100"); + const oErr = build(1000, "infra-scaffold-cmd-failed"); expect(oErr.message).toBe( - "infra-100: Scaffold Command was not successfully executed." + `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); }); it("positive test: object", () => { const oErr = build(1000, { - errorKey: "infra-105", + errorKey: "infra-err-locate-tf-env", values: ["test"], }); expect(oErr.message).toBe( - "infra-105: Unable to find Terraform environment. Ensure template path test exists." + `infra-err-locate-tf-env: ${errors["infra-err-locate-tf-env"].replace( + "{0}", + "test" + )}` ); }); it("negative test: invalid test", () => { - const oErr = build(1000, "infra-100xxxxx"); - expect(oErr.message).toBe("infra-100xxxxx"); + const oErr = build(1000, "infra-scaffold-cmd-failedxxxxx"); + expect(oErr.message).toBe("infra-scaffold-cmd-failedxxxxx"); }); }); describe("test build function", () => { it("negative test: invalid code", () => { expect(() => { - build(-1, "infra-1000"); + build(-1, "infra-scaffold-cmd-failed"); }).toThrow(); }); it("positive test: without error", () => { - const err = build(1000, "infra-100"); + const err = build(1000, "infra-scaffold-cmd-failed"); expect(err.errorCode).toBe(1000); expect(err.message).toBe( - "infra-100: Scaffold Command was not successfully executed." + `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); expect(err.details).toBeUndefined(); expect(err.parent).toBeUndefined(); }); it("positive test: with Error", () => { - const err = build(1000, "infra-100", Error("test")); + const err = build(1000, "infra-scaffold-cmd-failed", Error("test")); expect(err.errorCode).toBe(1000); expect(err.message).toBe( - "infra-100: Scaffold Command was not successfully executed." + `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); expect(err.details).toBe("test"); expect(err.parent).toBeUndefined(); }); it("positive test: with ErrorChain", () => { - const e = build(1000, "infra-101"); - const err = build(1000, "infra-100", e); + const e = build(1000, "infra-scaffold-cmd-src-missing"); + const err = build(1000, "infra-scaffold-cmd-failed", e); expect(err.errorCode).toBe(1000); expect(err.message).toBe( - "infra-100: Scaffold Command was not successfully executed." + `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); expect(err.details).toBeUndefined(); expect(err.parent).toStrictEqual(e); @@ -61,41 +67,49 @@ describe("test build function", () => { describe("test message function", () => { it("positive test: one error chain", () => { const messages: string[] = []; - const oError = build(1000, "infra-101"); + const oError = build(1000, "infra-scaffold-cmd-src-missing"); oError.messages(messages); expect(messages).toStrictEqual([ - "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", + `code: 1000\nmessage: infra-scaffold-cmd-src-missing: ${errors["infra-scaffold-cmd-src-missing"]}`, ]); }); it("positive test: one error chain with details", () => { const messages: string[] = []; - const oError = build(1000, "infra-101", Error("test message")); + const oError = build( + 1000, + "infra-scaffold-cmd-src-missing", + Error("test message") + ); oError.messages(messages); expect(messages).toStrictEqual([ - "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.\ndetails: test message", + `code: 1000\nmessage: infra-scaffold-cmd-src-missing: ${errors["infra-scaffold-cmd-src-missing"]}\ndetails: test message`, ]); }); it("positive test: multiple error chains", () => { const messages: string[] = []; const oError = build( 1000, - "infra-101", - build(1001, "infra-102", build(1010, "infra-103")) + "infra-scaffold-cmd-src-missing", + build( + 1001, + "infra-scaffold-cmd-values-missing", + build(1010, "infra-err-validating-remote-git") + ) ); oError.messages(messages); expect(messages).toStrictEqual([ - "code: 1000\nmessage: infra-101: Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", - " code: 1001\n message: infra-102: Values for name, version and/or 'template were missing. Provide value for values for them.", - " code: 1010\n message: infra-103: Unable to determine error when validating remote git source.", + `code: 1000\nmessage: infra-scaffold-cmd-src-missing: ${errors["infra-scaffold-cmd-src-missing"]}`, + ` code: 1001\n message: infra-scaffold-cmd-values-missing: ${errors["infra-scaffold-cmd-values-missing"]}`, + ` code: 1010\n message: infra-err-validating-remote-git: ${errors["infra-err-validating-remote-git"]}`, ]); }); }); describe("test log function", () => { it("test: Error chain object", () => { - const oError = build(1000, "infra-100"); + const oError = build(1000, "infra-scaffold-cmd-failed"); expect(log(oError)).toBe( - "\ncode: 1000\nmessage: infra-100: Scaffold Command was not successfully executed." + `\ncode: 1000\nmessage: infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); }); it("test: Error object", () => { diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts index 348362959..2c746efa0 100644 --- a/src/lib/errorBuilder.ts +++ b/src/lib/errorBuilder.ts @@ -72,10 +72,10 @@ const isErrorChainObject = (o: Error | ErrorChain): boolean => { * Builds an error object * * @param code spk error code - * @param errorKey Error key. e.g. "infra-101" or can be an object + * @param errorKey Error key. e.g. "infra-scaffold-cmd-src-missing" or can be an object * to support string substitution in error message * { - * errorKey: "infra-105", + * errorKey: "infra-scaffold-cmd-src-missing", * values: ["someValue"] * } * @param error: Parent error object. diff --git a/src/lib/i18n.json b/src/lib/i18n.json index ad472ca5e..586e26afa 100644 --- a/src/lib/i18n.json +++ b/src/lib/i18n.json @@ -16,15 +16,15 @@ "storageKeVaultName": "Enter key vault name (have the value as empty and hit enter key to skip)" }, "errors": { - "infra-100": "Scaffold Command was not successfully executed.", - "infra-101": "Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", - "infra-102": "Values for name, version and/or 'template were missing. Provide value for values for them.", - "infra-103": "Unable to determine error when validating remote git source.", - "infra-104": "Failure error thrown during retrying validating remote git source.", - "infra-105": "Unable to find Terraform environment. Ensure template path {0} exists.", - "infra-106": "Provided Terraform {0} path is invalid or cannot be found: {1}", - "infra-107": "Unable to create scaffold", - "infra-108": "Unable to clone the source remote repository. The remote repo may not exist or you do not have the rights to access it", - "infra-109": "Source path, {0} did not exist." + "infra-scaffold-cmd-failed": "Scaffold Command was not successfully executed.", + "infra-scaffold-cmd-src-missing": "Value for source is required because it cannot be constructed with properties in spk-config.yaml. Provide value for source.", + "infra-scaffold-cmd-values-missing": "Values for name, version and/or 'template were missing. Provide value for values for them.", + "infra-err-validating-remote-git": "Could not determine error when validating remote git source.", + "infra-err-retry-validating-remote-git": "Failure error thrown during retrying validating remote git source.", + "infra-err-locate-tf-env": "Could not find Terraform environment. Ensure template path {0} exists.", + "infra-err-tf-path-not-found": "Provided Terraform {0} path is invalid or cannot be found: {1}", + "infra-err-create-scaffold": "Could not create scaffold", + "infra-err-git-clone-failed": "Could not clone the source remote repository. The remote repo might not exist or you did not have the rights to access it", + "infra-git-source-no-exist": "Source path, {0} did not exist." } } From 90270845db924dead4044247ce9a5ffcd082a9b7 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Tue, 24 Mar 2020 12:25:54 -0700 Subject: [PATCH 6/9] jsdoc fixes --- src/lib/errorBuilder.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts index 2c746efa0..7a8ce3fe5 100644 --- a/src/lib/errorBuilder.ts +++ b/src/lib/errorBuilder.ts @@ -18,6 +18,12 @@ class ErrorChain extends Error { this.errorCode = code; this.message = this.getErrorMessage(errorInstance); } + + /** + * Returns error message + * + * @param errorInstance Error instance + */ getErrorMessage(errorInstance: string | ErrorParam): string { let key = ""; let values: string[] | undefined = undefined; @@ -44,7 +50,6 @@ class ErrorChain extends Error { /** * Generates error messages and have them in messages array. * - * @param error Error object * @param messages string of messages * @param padding Padding to be added to the beginning of messages */ From edba7611c040ad948aa0cc059a994e3233ad1da2 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Tue, 24 Mar 2020 12:29:57 -0700 Subject: [PATCH 7/9] code cleanup --- src/lib/errorBuilder.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts index 7a8ce3fe5..9ee0918d7 100644 --- a/src/lib/errorBuilder.ts +++ b/src/lib/errorBuilder.ts @@ -28,13 +28,14 @@ class ErrorChain extends Error { let key = ""; let values: string[] | undefined = undefined; - if (typeof errorInstance === "object") { + if (typeof errorInstance === "string") { + key = errorInstance; + } else { key = errorInstance.errorKey; values = errorInstance.values; - } else { - key = errorInstance; } + // if key is found in i18n json if (key in errors) { let results = errors[key]; if (values) { From 06f88eec3dc86bf75ab6e1d7749122f71cf6bea1 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Tue, 24 Mar 2020 13:45:17 -0700 Subject: [PATCH 8/9] Update generate.test.ts --- src/commands/infra/generate.test.ts | 76 +++++++++-------------------- 1 file changed, 22 insertions(+), 54 deletions(-) diff --git a/src/commands/infra/generate.test.ts b/src/commands/infra/generate.test.ts index 974d13f9e..ea95b1b12 100644 --- a/src/commands/infra/generate.test.ts +++ b/src/commands/infra/generate.test.ts @@ -197,11 +197,9 @@ describe("test gitClone function", () => { describe("Validate remote git source", () => { test("Validating that a git source is cloned to .spk/templates", async () => { - jest - .spyOn(generate, "checkRemoteGitExist") - .mockReturnValueOnce(Promise.resolve()); - jest.spyOn(generate, "gitFetchPull").mockReturnValueOnce(Promise.resolve()); - jest.spyOn(generate, "gitCheckout").mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "checkRemoteGitExist").mockResolvedValueOnce(); + jest.spyOn(generate, "gitFetchPull").mockResolvedValueOnce(); + jest.spyOn(generate, "gitCheckout").mockResolvedValueOnce(); const mockParentPath = "src/commands/infra/mocks/discovery-service"; const mockProjectPath = "src/commands/infra/mocks/discovery-service/west"; @@ -272,13 +270,11 @@ describe("fetch execute function", () => { .spyOn(generate, "validateDefinition") .mockReturnValueOnce(DefinitionYAMLExistence.PARENT_ONLY); jest.spyOn(generate, "validateTemplateSources").mockReturnValueOnce({}); - jest - .spyOn(generate, "validateRemoteSource") - .mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "validateRemoteSource").mockResolvedValueOnce(); jest .spyOn(infraCommon, "getSourceFolderNameFromURL") .mockImplementationOnce(() => { - throw new Error("Fake"); + throw Error("Fake"); }); const exitFn = jest.fn(); await execute( @@ -296,9 +292,7 @@ describe("fetch execute function", () => { jest .spyOn(generate, "validateDefinition") .mockReturnValueOnce(DefinitionYAMLExistence.BOTH_EXIST); - jest - .spyOn(generate, "validateRemoteSource") - .mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "validateRemoteSource").mockResolvedValueOnce(); jest.spyOn(generate, "validateTemplateSources").mockReturnValueOnce({}); jest .spyOn(generate, "generateConfig") @@ -324,11 +318,9 @@ describe("test validateRemoteSource function", () => { jest .spyOn(infraCommon, "getSourceFolderNameFromURL") .mockReturnValueOnce("sourceFolder"); - jest - .spyOn(generate, "checkRemoteGitExist") - .mockReturnValueOnce(Promise.resolve()); - jest.spyOn(generate, "gitClone").mockReturnValueOnce(Promise.resolve()); - jest.spyOn(generate, "gitCheckout").mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "checkRemoteGitExist").mockResolvedValueOnce(); + jest.spyOn(generate, "gitClone").mockResolvedValueOnce(); + jest.spyOn(generate, "gitCheckout").mockResolvedValueOnce(); await validateRemoteSource({ source: "source", @@ -339,17 +331,11 @@ describe("test validateRemoteSource function", () => { jest .spyOn(infraCommon, "getSourceFolderNameFromURL") .mockReturnValueOnce("sourceFolder"); - jest - .spyOn(generate, "checkRemoteGitExist") - .mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "checkRemoteGitExist").mockResolvedValueOnce(); jest .spyOn(generate, "gitClone") - .mockReturnValueOnce( - Promise.reject(new Error("refusing to merge unrelated histories")) - ); - jest - .spyOn(generate, "retryRemoteValidate") - .mockReturnValueOnce(Promise.resolve()); + .mockRejectedValueOnce(Error("refusing to merge unrelated histories")); + jest.spyOn(generate, "retryRemoteValidate").mockResolvedValueOnce(); await validateRemoteSource({ source: "source", @@ -360,15 +346,11 @@ describe("test validateRemoteSource function", () => { jest .spyOn(infraCommon, "getSourceFolderNameFromURL") .mockReturnValueOnce("sourceFolder"); - jest - .spyOn(generate, "checkRemoteGitExist") - .mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "checkRemoteGitExist").mockResolvedValueOnce(); jest .spyOn(generate, "gitClone") - .mockReturnValueOnce(Promise.reject(new Error("Authentication failed"))); - jest - .spyOn(generate, "retryRemoteValidate") - .mockReturnValueOnce(Promise.resolve()); + .mockRejectedValueOnce(Error("Authentication failed")); + jest.spyOn(generate, "retryRemoteValidate").mockResolvedValueOnce(); await validateRemoteSource({ source: "source", @@ -379,15 +361,11 @@ describe("test validateRemoteSource function", () => { jest .spyOn(infraCommon, "getSourceFolderNameFromURL") .mockReturnValueOnce("sourceFolder"); - jest - .spyOn(generate, "checkRemoteGitExist") - .mockReturnValueOnce(Promise.resolve()); + jest.spyOn(generate, "checkRemoteGitExist").mockResolvedValueOnce(); jest .spyOn(generate, "gitClone") - .mockReturnValueOnce(Promise.reject(new Error("other error"))); - jest - .spyOn(generate, "retryRemoteValidate") - .mockReturnValueOnce(Promise.resolve()); + .mockRejectedValueOnce(Error("other error")); + jest.spyOn(generate, "retryRemoteValidate").mockResolvedValueOnce(); try { await validateRemoteSource({ @@ -411,21 +389,11 @@ describe("test retryRemoteValidate function", () => { }); it("negative test", async () => { jest.spyOn(fsExtra, "removeSync").mockReturnValueOnce(); - jest - .spyOn(generate, "gitClone") - .mockReturnValueOnce(Promise.reject(new Error("error"))); + jest.spyOn(generate, "gitClone").mockRejectedValueOnce(Error("error")); - try { - await retryRemoteValidate( - "source", - "sourcePath", - "safeLoggingUrl", - "0.1" - ); - expect(true).toBe(false); - } catch (err) { - expect(err).toBeDefined(); - } + await expect( + retryRemoteValidate("source", "sourcePath", "safeLoggingUrl", "0.1") + ).rejects.toThrow(); }); }); From 7d347c8d1376d7293dac60409ad10ab2a50a88c8 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Thu, 26 Mar 2020 11:30:24 -0700 Subject: [PATCH 9/9] change error status code to enum --- src/commands/infra/generate.ts | 17 +++++++--- src/commands/infra/scaffold.ts | 25 +++++++++++---- src/lib/errorBuilder.test.ts | 58 +++++++++++++++++++++++----------- src/lib/errorBuilder.ts | 8 ++--- src/lib/errorStatusCode.ts | 18 ++++------- 5 files changed, 81 insertions(+), 45 deletions(-) diff --git a/src/commands/infra/generate.ts b/src/commands/infra/generate.ts index d4fee4c01..8a8aff8d6 100644 --- a/src/commands/infra/generate.ts +++ b/src/commands/infra/generate.ts @@ -23,6 +23,7 @@ import { } from "./infra_common"; import { copyTfTemplate } from "./scaffold"; import { build as buildError } from "../../lib/errorBuilder"; +import { errorStatusCode } from "../../lib/errorStatusCode"; interface CommandOptions { project: string | undefined; @@ -183,7 +184,7 @@ export const checkRemoteGitExist = async ( ): Promise => { // Checking for git remote if (!fs.existsSync(sourcePath)) { - throw buildError(1100, { + throw buildError(errorStatusCode.GIT_OPS_ERR, { errorKey: "infra-git-source-no-exist", values: [sourcePath], }); @@ -192,7 +193,7 @@ export const checkRemoteGitExist = async ( const result = await simpleGit(sourcePath).listRemote([source]); if (!result) { logger.error(result); - throw buildError(1100, "infra-err-git-clone-failed"); + throw buildError(errorStatusCode.GIT_OPS_ERR, "infra-err-git-clone-failed"); } logger.info(`Remote source repo: ${safeLoggingUrl} exists.`); @@ -290,10 +291,18 @@ export const validateRemoteSource = async ( version ); } else { - throw buildError(1100, "infra-err-validating-remote-git", err); + throw buildError( + errorStatusCode.GIT_OPS_ERR, + "infra-err-validating-remote-git", + err + ); } } catch (retryError) { - throw buildError(1100, "infra-err-retry-validating-remote-git", err); + throw buildError( + errorStatusCode.GIT_OPS_ERR, + "infra-err-retry-validating-remote-git", + err + ); } } else { throw err; diff --git a/src/commands/infra/scaffold.ts b/src/commands/infra/scaffold.ts index 119e0105c..c1d8ef09e 100644 --- a/src/commands/infra/scaffold.ts +++ b/src/commands/infra/scaffold.ts @@ -19,6 +19,7 @@ import { } from "./infra_common"; import decorator from "./scaffold.decorator.json"; import { build as buildError, log as logError } from "../../lib/errorBuilder"; +import { errorStatusCode } from "../../lib/errorStatusCode"; export interface CommandOptions { name: string; @@ -47,11 +48,17 @@ template repo and access token was not specified in spk-config.yml. Checking pas if (!opts.source) { // since access_token and infra_repository are missing, we cannot construct source for them - throw buildError(1001, "infra-scaffold-cmd-src-missing"); + throw buildError( + errorStatusCode.VALIDATION_ERR, + "infra-scaffold-cmd-src-missing" + ); } } if (!opts.name || !opts.version || !opts.template) { - throw buildError(1001, "infra-scaffold-cmd-values-missing"); + throw buildError( + errorStatusCode.VALIDATION_ERR, + "infra-scaffold-cmd-values-missing" + ); } logger.info(`All required options are configured via command line for \ scaffolding, expecting public remote repository for terraform templates \ @@ -96,7 +103,7 @@ export const copyTfTemplate = async ( logger.info(`Terraform template files copied from ${templatePath}`); } catch (err) { throw buildError( - 1010, + errorStatusCode.ENV_SETTING_ERR, { errorKey: "infra-err-locate-tf-env", values: [templatePath], @@ -113,7 +120,7 @@ export const copyTfTemplate = async ( */ export const validateVariablesTf = (templatePath: string): void => { if (!fs.existsSync(templatePath)) { - throw buildError(1010, { + throw buildError(errorStatusCode.ENV_SETTING_ERR, { errorKey: "infra-err-tf-path-not-found", values: [VARIABLES_TF, templatePath], }); @@ -291,7 +298,11 @@ export const scaffold = (values: CommandOptions): void => { } } } catch (err) { - throw buildError(1002, "infra-err-create-scaffold", err); + throw buildError( + errorStatusCode.EXE_FLOW_ERR, + "infra-err-create-scaffold", + err + ); } }; @@ -344,7 +355,9 @@ export const execute = async ( removeTemplateFiles(opts.name); await exitFn(0); } catch (err) { - logError(buildError(1000, "infra-scaffold-cmd-failed", err)); + logError( + buildError(errorStatusCode.CMD_EXE_ERR, "infra-scaffold-cmd-failed", err) + ); await exitFn(1); } }; diff --git a/src/lib/errorBuilder.test.ts b/src/lib/errorBuilder.test.ts index c3c9d1c0e..43f8c8ede 100644 --- a/src/lib/errorBuilder.test.ts +++ b/src/lib/errorBuilder.test.ts @@ -1,17 +1,21 @@ import { build, log } from "./errorBuilder"; import i18n from "./i18n.json"; +import { errorStatusCode } from "./errorStatusCode"; const errors = i18n.errors; describe("test getErrorMessage function", () => { it("positive test: string", () => { - const oErr = build(1000, "infra-scaffold-cmd-failed"); + const oErr = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-failed" + ); expect(oErr.message).toBe( `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); }); it("positive test: object", () => { - const oErr = build(1000, { + const oErr = build(errorStatusCode.CMD_EXE_ERR, { errorKey: "infra-err-locate-tf-env", values: ["test"], }); @@ -23,20 +27,18 @@ describe("test getErrorMessage function", () => { ); }); it("negative test: invalid test", () => { - const oErr = build(1000, "infra-scaffold-cmd-failedxxxxx"); + const oErr = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-failedxxxxx" + ); expect(oErr.message).toBe("infra-scaffold-cmd-failedxxxxx"); }); }); describe("test build function", () => { - it("negative test: invalid code", () => { - expect(() => { - build(-1, "infra-scaffold-cmd-failed"); - }).toThrow(); - }); it("positive test: without error", () => { - const err = build(1000, "infra-scaffold-cmd-failed"); - expect(err.errorCode).toBe(1000); + const err = build(errorStatusCode.CMD_EXE_ERR, "infra-scaffold-cmd-failed"); + expect(err.errorCode).toBe(errorStatusCode.CMD_EXE_ERR); expect(err.message).toBe( `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); @@ -44,8 +46,12 @@ describe("test build function", () => { expect(err.parent).toBeUndefined(); }); it("positive test: with Error", () => { - const err = build(1000, "infra-scaffold-cmd-failed", Error("test")); - expect(err.errorCode).toBe(1000); + const err = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-failed", + Error("test") + ); + expect(err.errorCode).toBe(errorStatusCode.CMD_EXE_ERR); expect(err.message).toBe( `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); @@ -53,9 +59,16 @@ describe("test build function", () => { expect(err.parent).toBeUndefined(); }); it("positive test: with ErrorChain", () => { - const e = build(1000, "infra-scaffold-cmd-src-missing"); - const err = build(1000, "infra-scaffold-cmd-failed", e); - expect(err.errorCode).toBe(1000); + const e = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-src-missing" + ); + const err = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-failed", + e + ); + expect(err.errorCode).toBe(errorStatusCode.CMD_EXE_ERR); expect(err.message).toBe( `infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); @@ -67,7 +80,10 @@ describe("test build function", () => { describe("test message function", () => { it("positive test: one error chain", () => { const messages: string[] = []; - const oError = build(1000, "infra-scaffold-cmd-src-missing"); + const oError = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-src-missing" + ); oError.messages(messages); expect(messages).toStrictEqual([ `code: 1000\nmessage: infra-scaffold-cmd-src-missing: ${errors["infra-scaffold-cmd-src-missing"]}`, @@ -93,7 +109,10 @@ describe("test message function", () => { build( 1001, "infra-scaffold-cmd-values-missing", - build(1010, "infra-err-validating-remote-git") + build( + errorStatusCode.ENV_SETTING_ERR, + "infra-err-validating-remote-git" + ) ) ); oError.messages(messages); @@ -107,7 +126,10 @@ describe("test message function", () => { describe("test log function", () => { it("test: Error chain object", () => { - const oError = build(1000, "infra-scaffold-cmd-failed"); + const oError = build( + errorStatusCode.CMD_EXE_ERR, + "infra-scaffold-cmd-failed" + ); expect(log(oError)).toBe( `\ncode: 1000\nmessage: infra-scaffold-cmd-failed: ${errors["infra-scaffold-cmd-failed"]}` ); diff --git a/src/lib/errorBuilder.ts b/src/lib/errorBuilder.ts index 9ee0918d7..69521376a 100644 --- a/src/lib/errorBuilder.ts +++ b/src/lib/errorBuilder.ts @@ -1,5 +1,5 @@ import i18n from "./i18n.json"; -import { isValid } from "./errorStatusCode"; +import { errorStatusCode } from "./errorStatusCode"; import { logger } from "../logger"; const errors: { [key: string]: string } = i18n.errors; @@ -87,14 +87,10 @@ const isErrorChainObject = (o: Error | ErrorChain): boolean => { * @param error: Parent error object. */ export const build = ( - code: number, + code: errorStatusCode, errorKey: string | ErrorParam, error?: Error | ErrorChain ): ErrorChain => { - if (!isValid(code)) { - throw Error(`Invalid status code, ${code}`); - } - const oError = new ErrorChain(code, errorKey); if (error) { diff --git a/src/lib/errorStatusCode.ts b/src/lib/errorStatusCode.ts index e615266e0..675b662ea 100644 --- a/src/lib/errorStatusCode.ts +++ b/src/lib/errorStatusCode.ts @@ -1,14 +1,10 @@ // please do not change the status code numbers // you can add new ones but not changing the existing ones -export const errorStatusCode = { - 1000: "command fails to execute", - 1001: "validation error", - 1002: "execution error", - 1010: "environment related error", - 1100: "git operation related error", -}; - -export const isValid = (code: number): boolean => { - return code in errorStatusCode; -}; +export enum errorStatusCode { + CMD_EXE_ERR = 1000, + VALIDATION_ERR = 1001, + EXE_FLOW_ERR = 1002, + ENV_SETTING_ERR = 1010, + GIT_OPS_ERR = 1100, +}