From 6c06c964d88d1c98af9d06aadd21e7054b99f64f Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Thu, 30 Jan 2020 09:37:31 -0800 Subject: [PATCH 1/6] [REFACTOR] Service create command --- src/commands/service/create.decorator.json | 67 +++ src/commands/service/create.test.ts | 301 ++++++++------ src/commands/service/create.ts | 452 +++++++-------------- src/lib/ioUtil.test.ts | 4 + src/lib/ioUtil.ts | 20 +- src/lib/validator.test.ts | 32 +- src/lib/validator.ts | 26 ++ 7 files changed, 454 insertions(+), 448 deletions(-) create mode 100644 src/commands/service/create.decorator.json diff --git a/src/commands/service/create.decorator.json b/src/commands/service/create.decorator.json new file mode 100644 index 000000000..33d2988a0 --- /dev/null +++ b/src/commands/service/create.decorator.json @@ -0,0 +1,67 @@ +{ + "command": "create ", + "alias": "c", + "description": "Add a new service into this initialized spk project repository", + "options": [ + { + "arg": "-c, --helm-chart-chart ", + "description": "bedrock helm chart name. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", + "defaultValue": "" + }, + { + "arg": "-r, --helm-chart-repository ", + "description": "bedrock helm chart repository. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", + "defaultValue": "" + }, + { + "arg": "-b, --helm-config-branch ", + "description": "bedrock custom helm chart configuration branch. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", + "defaultValue": "" + }, + { + "arg": "-p, --helm-config-path ", + "description": "bedrock custom helm chart configuration path. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", + "defaultValue": "" + }, + { + "arg": "-g, --helm-config-git ", + "description": "bedrock helm chart configuration git repository. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", + "defaultValue": "" + }, + { + "arg": "-d, --packages-dir ", + "description": "The directory containing the mono-repo packages.", + "defaultValue": "" + }, + { + "arg": "-n, --display-name ", + "description": "Display name of the service.", + "defaultValue": "" + }, + { + "arg": "-m, --maintainer-name ", + "description": "The name of the primary maintainer for this service.", + "defaultValue": "maintainer name" + }, + { + "arg": "-e, --maintainer-email ", + "description": "The email of the primary maintainer for this service.", + "defaultValue": "maintainer email" + }, + { + "arg": "--git-push", + "description": "SPK CLI will try to commit and push these changes to a new origin/branch named after the service.", + "defaultValue": false + }, + { + "arg": "--middlewares ", + "description": "Traefik2 middlewares you wish to to be injected into your Traefik2 IngressRoutes", + "defaultValue": "" + }, + { + "arg": "--k8s-service-port ", + "description": "Kubernetes service port which this service is exposed with; will be used to configure Traefik2 IngressRoutes", + "defaultValue": "80" + } + ] +} diff --git a/src/commands/service/create.test.ts b/src/commands/service/create.test.ts index acf18d360..b5eb6576b 100644 --- a/src/commands/service/create.test.ts +++ b/src/commands/service/create.test.ts @@ -1,10 +1,11 @@ import fs from "fs"; -import os from "os"; import path from "path"; import { promisify } from "util"; import uuid from "uuid/v4"; import { Bedrock } from "../../config"; +import * as config from "../../config"; import { checkoutCommitPushCreatePRLink } from "../../lib/gitutils"; +import { createTempDir, removeDir } from "../../lib/ioUtil"; import { disableVerboseLogging, enableVerboseLogging, @@ -14,7 +15,7 @@ import { createTestBedrockYaml, createTestMaintainersYaml } from "../../test/mockFactory"; -import { createService, isValidConfig } from "./create"; +import { createService, execute, fetchValues, ICommandValues } from "./create"; jest.mock("../../lib/gitutils"); beforeAll(() => { @@ -25,35 +26,150 @@ afterAll(() => { disableVerboseLogging(); }); -describe("validate pipeline config", () => { - const configValues: any[] = [ - "testHelmChart", - "testHelmRepo", - "testHelmConfigBranch", - "testHelmConfigGit", - "/test/path", - "testService", - "test/packages", - "test-maintainer", - "test@maintainer.com", - "my,middleware,string", - true, - "testDisplayName", - 80 - ]; - - it("config is valid", () => { - expect(isValidConfig.apply(undefined, configValues as any)).toBe(true); +beforeEach(() => { + jest.clearAllMocks(); + jest.resetAllMocks(); +}); + +const mockValues: ICommandValues = { + displayName: "", + gitPush: false, + helmChartChart: "", + helmChartRepository: "", + helmConfigBranch: "", + helmConfigGit: "", + helmConfigPath: "", + k8sPort: 80, + k8sServicePort: "80", + maintainerEmail: "", + maintainerName: "", + middlewares: "", + middlewaresArray: [], + packagesDir: "", + variableGroups: [] +}; + +const getMockValues = (): ICommandValues => { + // TOFIX: if possible, can we use lodash? + return JSON.parse(JSON.stringify(mockValues)); +}; + +const validateDirNFiles = ( + dir: string, + serviceName: string, + values: ICommandValues +) => { + // Check temp test directory exists + expect(fs.existsSync(dir)).toBe(true); + + // Check service directory exists + const serviceDirPath = path.join(dir, values.packagesDir, serviceName); + expect(fs.existsSync(serviceDirPath)).toBe(true); + + // Verify new azure-pipelines created + const filepaths = ["azure-pipelines.yaml", "Dockerfile"].map(filename => + path.join(serviceDirPath, filename) + ); + + for (const filepath of filepaths) { + expect(fs.existsSync(filepath)).toBe(true); + } +}; + +describe("Test fetchValues function", () => { + it("Negative test: invalid port", () => { + const values = getMockValues(); + values.k8sServicePort = "abc"; + try { + fetchValues(values); + expect(true).toBe(false); + } catch (err) { + expect(err).toBeDefined(); + } + }); + it("Postive test: with middlewares value", () => { + const mocked = getMockValues(); + mocked.middlewares = "mid1, mid2"; // space after comma is intentional, expecting trimming to happen + const result = fetchValues(mocked); + expect(result.middlewaresArray).toEqual(["mid1", "mid2"]); + }); + it("Postive test", () => { + const mocked = getMockValues(); + const result = fetchValues(mocked); + expect(result).toEqual(mocked); }); +}); - it("undefined parameters", () => { - for (const i of configValues.keys()) { - const configValuesWithInvalidValue = configValues.map((value, j) => - i === j ? undefined : value - ); - expect( - isValidConfig.apply(undefined, configValuesWithInvalidValue as any) - ).toBe(false); +describe("Test execute function", () => { + it("Negative test: without service name", async () => { + const exitFn = jest.fn(); + await execute("", getMockValues(), exitFn); + expect(exitFn).toBeCalledTimes(1); + expect(exitFn.mock.calls).toEqual([[1]]); + }); + it("Negative test: missing bedrock file", async () => { + const testServiceName = uuid(); + const exitFn = jest.fn(); + jest.spyOn(config, "bedrockFileInfo").mockImplementation(() => + Promise.resolve({ + exist: false, + hasVariableGroups: false + }) + ); + try { + await execute(testServiceName, getMockValues(), exitFn); + expect(exitFn).toBeCalledTimes(1); + expect(exitFn.mock.calls).toEqual([[1]]); + } finally { + removeDir(testServiceName); // housekeeping + } + }); + it("Negative test: missing bedrock variable groups", async () => { + const testServiceName = uuid(); + const exitFn = jest.fn(); + jest.spyOn(config, "bedrockFileInfo").mockImplementation(() => + Promise.resolve({ + exist: true, + hasVariableGroups: false + }) + ); + try { + await execute(testServiceName, getMockValues(), exitFn); + expect(exitFn).toBeCalledTimes(1); + expect(exitFn.mock.calls).toEqual([[1]]); + } finally { + removeDir(testServiceName); // housekeeping + } + }); + it("Negative test: simulated exception thrown", async () => { + const testServiceName = uuid(); + const exitFn = jest.fn(); + jest + .spyOn(config, "bedrockFileInfo") + .mockImplementation(() => Promise.reject()); + try { + await execute(testServiceName, getMockValues(), exitFn); + expect(exitFn).toBeCalledTimes(1); + expect(exitFn.mock.calls).toEqual([[1]]); + } finally { + removeDir(testServiceName); // housekeeping + } + }); + it("Positive test", async () => { + const testServiceName = uuid(); + const exitFn = jest.fn(); + jest.spyOn(config, "bedrockFileInfo").mockImplementation(() => + Promise.resolve({ + exist: true, + hasVariableGroups: true + }) + ); + try { + await execute(testServiceName, getMockValues(), exitFn); + expect(exitFn).toBeCalledTimes(1); + expect(exitFn.mock.calls).toEqual([[0]]); + } finally { + removeDir(testServiceName); // housekeeping } }); }); @@ -62,8 +178,7 @@ describe("Adding a service to a repo directory", () => { let randomTmpDir: string = ""; beforeEach(async () => { // Create random directory to initialize - randomTmpDir = path.join(os.tmpdir(), uuid()); - fs.mkdirSync(randomTmpDir); + randomTmpDir = createTempDir(); }); test("New directory is created under root directory with required service files.", async () => { @@ -72,45 +187,23 @@ describe("Adding a service to a repo directory", () => { ); await writeSampleBedrockFileToDir(path.join(randomTmpDir, "bedrock.yaml")); - const packageDir = ""; - + const values = getMockValues(); + values.packagesDir = ""; + values.k8sPort = 1337; const serviceName = uuid(); logger.info( `creating randomTmpDir ${randomTmpDir} and service ${serviceName}` ); - // addService call - const k8sServicePort = 1337; - await createService( - randomTmpDir, - serviceName, - packageDir, - false, - k8sServicePort - ); - - // Check temp test directory exists - expect(fs.existsSync(randomTmpDir)).toBe(true); - - // Check service directory exists - const serviceDirPath = path.join(randomTmpDir, packageDir, serviceName); - expect(fs.existsSync(serviceDirPath)).toBe(true); - - // Verify new azure-pipelines created - const filepaths = ["azure-pipelines.yaml", "Dockerfile"].map(filename => - path.join(serviceDirPath, filename) - ); - - for (const filepath of filepaths) { - expect(fs.existsSync(filepath)).toBe(true); - } + await createService(randomTmpDir, serviceName, values); + validateDirNFiles(randomTmpDir, serviceName, values); // TODO: Verify root project bedrock.yaml and maintainers.yaml has been changed too. const bedrock = Bedrock(randomTmpDir); const newService = bedrock.services["./" + serviceName]; expect(newService).toBeDefined(); - expect(newService.k8sServicePort).toBe(k8sServicePort); + expect(newService.k8sServicePort).toBe(values.k8sPort); }); test("New directory is created under '/packages' directory with required service files.", async () => { @@ -119,32 +212,18 @@ describe("Adding a service to a repo directory", () => { ); await writeSampleBedrockFileToDir(path.join(randomTmpDir, "bedrock.yaml")); - const packageDir = "packages"; + const values = getMockValues(); + values.packagesDir = "packages"; + values.k8sPort = 1337; const serviceName = uuid(); - const variableGroupName = uuid(); logger.info( `creating randomTmpDir ${randomTmpDir} and service ${serviceName}` ); // addService call - await createService(randomTmpDir, serviceName, "packages", false, 1337); - - // Check temp test directory exists - expect(fs.existsSync(randomTmpDir)).toBe(true); - - // Check service directory exists - const serviceDirPath = path.join(randomTmpDir, packageDir, serviceName); - expect(fs.existsSync(serviceDirPath)).toBe(true); - - // Verify new azure-pipelines and Dockerfile created - const filepaths = ["azure-pipelines.yaml", "Dockerfile"].map(filename => - path.join(serviceDirPath, filename) - ); - - for (const filepath of filepaths) { - expect(fs.existsSync(filepath)).toBe(true); - } + await createService(randomTmpDir, serviceName, values); + validateDirNFiles(randomTmpDir, serviceName, values); // TODO: Verify root project bedrock.yaml and maintainers.yaml has been changed too. }); @@ -155,32 +234,18 @@ describe("Adding a service to a repo directory", () => { ); await writeSampleBedrockFileToDir(path.join(randomTmpDir, "bedrock.yaml")); - const packageDir = "packages"; + const values = getMockValues(); + values.packagesDir = "packages"; + values.gitPush = true; + values.k8sPort = 1337; const serviceName = uuid(); - const variableGroupName = uuid(); logger.info( `creating randomTmpDir ${randomTmpDir} and service ${serviceName}` ); - // addService call - await createService(randomTmpDir, serviceName, "packages", true, 1337); - - // Check temp test directory exists - expect(fs.existsSync(randomTmpDir)).toBe(true); - - // Check service directory exists - const serviceDirPath = path.join(randomTmpDir, packageDir, serviceName); - expect(fs.existsSync(serviceDirPath)).toBe(true); - - // Verify new azure-pipelines and Dockerfile created - const filepaths = ["azure-pipelines.yaml", "Dockerfile"].map(filename => - path.join(serviceDirPath, filename) - ); - - for (const filepath of filepaths) { - expect(fs.existsSync(filepath)).toBe(true); - } + await createService(randomTmpDir, serviceName, values); + validateDirNFiles(randomTmpDir, serviceName, values); expect(checkoutCommitPushCreatePRLink).toHaveBeenCalled(); }); @@ -191,23 +256,17 @@ describe("Adding a service to a repo directory", () => { ); await writeSampleBedrockFileToDir(path.join(randomTmpDir, "bedrock.yaml")); - const packageDir = ""; + const values = getMockValues(); + values.k8sPort = 1337; const serviceName = uuid(); logger.info( `creating randomTmpDir ${randomTmpDir} and service ${serviceName}` ); // create service with no middleware - await createService(randomTmpDir, serviceName, packageDir, false, 1337); - - // Check temp test directory exists - expect(fs.existsSync(randomTmpDir)).toBe(true); - - // Check service directory exists - const serviceDirPath = path.join(randomTmpDir, packageDir, serviceName); - expect(fs.existsSync(serviceDirPath)).toBe(true); + await createService(randomTmpDir, serviceName, values); + validateDirNFiles(randomTmpDir, serviceName, values); - // get bedrock config const bedrockConfig = Bedrock(randomTmpDir); // check the added service has an empty list for middlewares @@ -228,26 +287,18 @@ describe("Adding a service to a repo directory", () => { ); await writeSampleBedrockFileToDir(path.join(randomTmpDir, "bedrock.yaml")); - const packageDir = ""; + const values = getMockValues(); + values.k8sPort = 1337; + values.middlewaresArray = ["foo", "bar", "baz"]; + const serviceName = uuid(); logger.info( `creating randomTmpDir ${randomTmpDir} and service ${serviceName}` ); - // add some middlewares - const middlewares = ["foo", "bar", "baz"]; - await createService(randomTmpDir, serviceName, packageDir, false, 1337, { - middlewares - }); - - // Check temp test directory exists - expect(fs.existsSync(randomTmpDir)).toBe(true); - - // Check service directory exists - const serviceDirPath = path.join(randomTmpDir, packageDir, serviceName); - expect(fs.existsSync(serviceDirPath)).toBe(true); + await createService(randomTmpDir, serviceName, values); + validateDirNFiles(randomTmpDir, serviceName, values); - // get bedrock config const bedrockConfig = Bedrock(randomTmpDir); // check that the added service has the expected middlewares @@ -257,8 +308,10 @@ describe("Adding a service to a repo directory", () => { if (servicePath.includes(serviceName)) { expect(service.middlewares).toBeDefined(); expect(Array.isArray(service.middlewares)).toBe(true); - expect(service.middlewares?.length).toBe(middlewares.length); - expect(service.middlewares).toStrictEqual(middlewares); + expect(service.middlewares?.length).toBe( + values.middlewaresArray.length + ); + expect(service.middlewares).toStrictEqual(values.middlewaresArray); } } }); diff --git a/src/commands/service/create.ts b/src/commands/service/create.ts index 88466f770..a6b20825c 100644 --- a/src/commands/service/create.ts +++ b/src/commands/service/create.ts @@ -10,6 +10,7 @@ import { addNewService as addNewServiceToBedrockFile, YAML_NAME as BedrockFileName } from "../../lib/bedrockYaml"; +import { build as buildCmd, exit as exitCmd } from "../../lib/commandBuilder"; import { addNewServiceToMaintainersFile, generateDockerfile, @@ -17,8 +18,108 @@ import { generateStarterAzurePipelinesYaml } from "../../lib/fileutils"; import { checkoutCommitPushCreatePRLink } from "../../lib/gitutils"; +import { isPortNumber } from "../../lib/validator"; import { logger } from "../../logger"; import { IBedrockFileInfo, IHelmConfig, IUser } from "../../types"; +import decorator from "./create.decorator.json"; + +export interface ICommandOptions { + displayName: string; + gitPush: boolean; + helmChartChart: string; + helmChartRepository: string; + helmConfigBranch: string; + helmConfigGit: string; + helmConfigPath: string; + maintainerEmail: string; + maintainerName: string; + middlewares: string; + packagesDir: string; + k8sServicePort: string; +} + +export interface ICommandValues extends ICommandOptions { + k8sPort: number; + middlewaresArray: string[]; + variableGroups: string[]; +} + +export const fetchValues = (opts: ICommandOptions) => { + if (!isPortNumber(opts.k8sServicePort)) { + throw new Error("value for --k8s-service-port is not a value port number"); + } + const bedrock = Bedrock(); + + let middlewaresArray: string[] = []; + if (opts.middlewares && opts.middlewares.trim()) { + middlewaresArray = opts.middlewares.split(",").map(str => str.trim()); + } + + const values: ICommandValues = { + displayName: opts.displayName, + gitPush: opts.gitPush, + helmChartChart: opts.helmChartChart, + helmChartRepository: opts.helmChartRepository, + helmConfigBranch: opts.helmConfigBranch, + helmConfigGit: opts.helmConfigGit, + helmConfigPath: opts.helmConfigPath, + k8sPort: parseInt(opts.k8sServicePort, 10), + k8sServicePort: opts.k8sServicePort, + maintainerEmail: opts.maintainerEmail, + maintainerName: opts.maintainerName, + middlewares: opts.middlewares, + middlewaresArray, + packagesDir: opts.packagesDir, + variableGroups: bedrock.variableGroups || [] + }; + + // values need not be validated (that's do not need + // to do is type === "string" check because default values + // are provided in the commander and most of them + // are "" except for gitPush is false and k8sServicePort + // is "80" + return values; +}; + +export const execute = async ( + serviceName: string, + opts: ICommandOptions, + exitFn: (status: number) => Promise +) => { + if (!serviceName) { + logger.error("Service name is missing"); + await exitFn(1); + return; + } + + const projectPath = process.cwd(); + logger.verbose(`project path: ${projectPath}`); + + try { + const fileInfo: IBedrockFileInfo = await bedrockFileInfo(projectPath); + if (fileInfo.exist === false) { + logger.error(projectInitCvgDependencyErrorMessage()); + await exitFn(1); + return; + } + + if (fileInfo.hasVariableGroups === false) { + logger.error(projectCvgDependencyErrorMessage()); + await exitFn(1); + return; + } + + const values = fetchValues(opts); + await createService(projectPath, serviceName, values); + await exitFn(0); + } catch (err) { + logger.error( + `Error occurred adding service ${serviceName} to project ${projectPath}` + ); + logger.error(err); + await exitFn(1); + } +}; /** * Adds the create command to the service command object @@ -26,264 +127,13 @@ import { IBedrockFileInfo, IHelmConfig, IUser } from "../../types"; * @param command Commander command object to decorate */ export const createCommandDecorator = (command: commander.Command): void => { - command - .command("create ") - .alias("c") - .description( - "Add a new service into this initialized spk project repository" - ) - .option( - "-c, --helm-chart-chart ", - "bedrock helm chart name. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", - "" - ) - .option( - "-r, --helm-chart-repository ", - "bedrock helm chart repository. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", - "" - ) - .option( - "-b, --helm-config-branch ", - "bedrock custom helm chart configuration branch. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", - "" - ) - .option( - "-p, --helm-config-path ", - "bedrock custom helm chart configuration path. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", - "" - ) - .option( - "-g, --helm-config-git ", - "bedrock helm chart configuration git repository. --helm-chart-* and --helm-config-* are exclusive; you may only use one.", - "" - ) - .option( - "-d, --packages-dir ", - "The directory containing the mono-repo packages.", - "" - ) - .option( - "-n, --display-name ", - "Display name of the service.", - "" - ) - .option( - "-m, --maintainer-name ", - "The name of the primary maintainer for this service.", - "maintainer name" - ) - .option( - "-e, --maintainer-email ", - "The email of the primary maintainer for this service.", - "maintainer email" - ) - .option( - "--git-push", - "SPK CLI will try to commit and push these changes to a new origin/branch named after the service.", - false - ) - .option( - "--middlewares ", - "Traefik2 middlewares you wish to to be injected into your Traefik2 IngressRoutes", - "" - ) - .option( - "--k8s-service-port ", - "Kubernetes service port which this service is exposed with; will be used to configure Traefik2 IngressRoutes", - "80" - ) - .action(async (serviceName, opts) => { - const projectPath = process.cwd(); - logger.verbose(`project path: ${projectPath}`); - - const fileInfo: IBedrockFileInfo = await bedrockFileInfo(projectPath); - if (fileInfo.exist === false) { - logger.error(projectInitCvgDependencyErrorMessage); - return undefined; - } else if (fileInfo.hasVariableGroups === false) { - logger.error(projectCvgDependencyErrorMessage); - return undefined; - } - - const bedrock = Bedrock(); - - const { - displayName, - gitPush, - helmChartChart, - helmChartRepository, - helmConfigBranch, - helmConfigGit, - helmConfigPath, - maintainerEmail, - maintainerName, - middlewares, - packagesDir - } = opts; - const k8sPort = Number(opts.k8sServicePort); - const variableGroups = bedrock?.variableGroups; - - try { - if ( - !isValidConfig( - helmChartChart, - helmChartRepository, - helmConfigBranch, - helmConfigGit, - helmConfigPath, - serviceName, - packagesDir, - maintainerName, - maintainerEmail, - middlewares, - gitPush, - displayName, - k8sPort - ) - ) { - throw Error(`Invalid configuration provided`); - } - - await createService( - projectPath, - serviceName, - packagesDir, - gitPush, - k8sPort, - { - displayName, - helmChartChart, - helmChartRepository, - helmConfigBranch, - helmConfigGit, - helmConfigPath, - maintainerEmail, - maintainerName, - middlewares: (middlewares as string) - .split(",") - .map(str => str.trim()), - variableGroups - } - ); - } catch (err) { - logger.error( - `Error occurred adding service ${serviceName} to project ${projectPath}` - ); - logger.error(err); - process.exit(1); - } - }); -}; - -/** - * Validates the pipeline configuration - * @param helmChartChart Helm chart chart - * @param helmChartRepository Helm chart repository - * @param helmConfigBranch Helm chart branch - * @param helmConfigGit Helm git - * @param helmConfigPath Helm config path - * @param serviceName Service name - * @param packagesDir Packages directory - * @param maintainerName Name of maintainer - * @param maintainerEmail Email of maintainer - * @param middlewares comma-delimitated list of Traefik2 middlewares - * @param gitPush Push to git - */ -export const isValidConfig = ( - helmChartChart: any, - helmChartRepository: any, - helmConfigBranch: any, - helmConfigGit: any, - helmConfigPath: any, - serviceName: any, - packagesDir: any, - maintainerName: any, - maintainerEmail: any, - middlewares: any, - gitPush: any, - displayName: any, - k8sPort: any -): boolean => { - const missingConfig = []; - - // Type check all parsed command line args here. - if (typeof helmChartChart !== "string") { - missingConfig.push( - `helmChartChart must be of type 'string', ${typeof helmChartChart} given.` - ); - } - if (typeof helmChartRepository !== "string") { - missingConfig.push( - `helmChartRepository must be of type 'string', ${typeof helmChartRepository} given.` - ); - } - if (typeof helmConfigBranch !== "string") { - missingConfig.push( - `helmConfigBranch must be of type 'string', ${typeof helmConfigBranch} given.` - ); - } - if (typeof helmConfigGit !== "string") { - missingConfig.push( - `helmConfigGit must be of type 'string', ${typeof helmConfigGit} given.` - ); - } - if (typeof helmConfigPath !== "string") { - missingConfig.push( - `helmConfigPath must be of type 'string', ${typeof helmConfigPath} given.` - ); - } - if (typeof serviceName !== "string") { - missingConfig.push( - `serviceName must be of type 'string', ${typeof serviceName} given.` - ); - } - if (typeof displayName !== "string") { - missingConfig.push( - `displayName must be of type 'string', ${typeof displayName} given.` - ); - } - if (typeof packagesDir !== "string") { - missingConfig.push( - `packagesDir must be of type 'string', ${typeof packagesDir} given.` - ); - } - if (typeof maintainerName !== "string") { - missingConfig.push( - `maintainerName must be of type 'string', ${typeof maintainerName} given.` - ); - } - if (typeof maintainerEmail !== "string") { - missingConfig.push( - `maintainerEmail must be of type 'string', ${typeof maintainerEmail} given.` - ); - } - if (typeof middlewares !== "string") { - missingConfig.push( - `middlewares must be of type of 'string', ${typeof middlewares} given.` - ); - } - if (typeof gitPush !== "boolean") { - missingConfig.push( - `gitPush must be of type 'boolean', ${typeof gitPush} given.` - ); - } - // k8sPort has to be a positive integer - if ( - typeof k8sPort !== "number" || - !Number.isInteger(k8sPort) || - k8sPort < 0 - ) { - missingConfig.push( - `k8s-port must be a positive integer, parsed ${k8sPort} from input.` - ); - } - - if (missingConfig.length > 0) { - logger.error("Error in configuration: " + missingConfig.join(" ")); - return false; - } - - return true; + buildCmd(command, decorator).action( + async (serviceName: string, opts: ICommandOptions) => { + await execute(serviceName, opts, async (status: number) => { + await exitCmd(logger, process.exit, status); + }); + } + ); }; /** @@ -296,51 +146,27 @@ export const isValidConfig = ( export const createService = async ( rootProjectPath: string, serviceName: string, - packagesDir: string, - gitPush: boolean, - k8sServicePort: number, - opts?: { - displayName?: string; - helmChartChart?: string; - helmChartRepository?: string; - helmConfigBranch?: string; - helmConfigGit?: string; - helmConfigPath?: string; - maintainerEmail?: string; - maintainerName?: string; - variableGroups?: string[]; - middlewares?: string[]; - } + values: ICommandValues ) => { - const { - displayName = "", - helmChartChart = "", - helmChartRepository = "", - helmConfigBranch = "", - helmConfigPath = "", - helmConfigGit = "", - maintainerName = "", - maintainerEmail = "", - middlewares = [], - variableGroups = [] - } = opts ?? {}; - logger.info( - `Adding Service: ${serviceName}, to Project: ${rootProjectPath} under directory: ${packagesDir}` + `Adding Service: ${serviceName}, to Project: ${rootProjectPath} under directory: ${values.packagesDir}` ); logger.info( - `DisplayName: ${displayName}, MaintainerName: ${maintainerName}, MaintainerEmail: ${maintainerEmail}` + `DisplayName: ${values.displayName}, MaintainerName: ${values.maintainerName}, MaintainerEmail: ${values.maintainerEmail}` ); - const newServiceDir = path.join(rootProjectPath, packagesDir, serviceName); + const newServiceDir = path.join( + rootProjectPath, + values.packagesDir, + serviceName + ); logger.info(`servicePath: ${newServiceDir}`); - // Mkdir shelljs.mkdir("-p", newServiceDir); // Create azure pipelines yaml in directory await generateStarterAzurePipelinesYaml(rootProjectPath, newServiceDir, { - variableGroups + variableGroups: values.variableGroups }); // Create empty .gitignore file in directory @@ -351,8 +177,8 @@ export const createService = async ( // add maintainers to file in parent repo file const newUser = { - email: maintainerEmail, - name: maintainerName + email: values.maintainerEmail, + name: values.maintainerName } as IUser; const newServiceRelativeDir = path.relative(rootProjectPath, newServiceDir); @@ -366,35 +192,33 @@ export const createService = async ( // Add relevant bedrock info to parent bedrock.yaml - let helmConfig: IHelmConfig; - if (helmChartChart && helmChartRepository) { - helmConfig = { - chart: { - chart: helmChartChart, - repository: helmChartRepository - } - }; - } else { - helmConfig = { - chart: { - branch: helmConfigBranch, - git: helmConfigGit, - path: helmConfigPath - } - }; - } + const helmConfig: IHelmConfig = + values.helmChartChart && values.helmChartRepository + ? { + chart: { + chart: values.helmChartChart, + repository: values.helmChartRepository + } + } + : { + chart: { + branch: values.helmConfigBranch, + git: values.helmConfigGit, + path: values.helmConfigPath + } + }; addNewServiceToBedrockFile( rootProjectPath, newServiceRelativeDir, - displayName, + values.displayName, helmConfig, - middlewares, - k8sServicePort + values.middlewaresArray, + values.k8sPort ); // If requested, create new git branch, commit, and push - if (gitPush) { + if (values.gitPush) { await checkoutCommitPushCreatePRLink( serviceName, newServiceDir, diff --git a/src/lib/ioUtil.test.ts b/src/lib/ioUtil.test.ts index bf3ec7867..a1ac165fd 100644 --- a/src/lib/ioUtil.test.ts +++ b/src/lib/ioUtil.test.ts @@ -1,5 +1,6 @@ import fs from "fs"; import path from "path"; +import uuid from "uuid/v4"; import { createTempDir, getMissingFilenames, removeDir } from "./ioUtil"; describe("test createTempDir function", () => { @@ -21,6 +22,9 @@ describe("test createTempDir function", () => { }); describe("test removeDir", () => { + it("non exist directory", () => { + removeDir(uuid()); // no exception thrown + }); it("empty directory", () => { const name = createTempDir(); removeDir(name); diff --git a/src/lib/ioUtil.ts b/src/lib/ioUtil.ts index 2a0d22bd7..6d38e2bfe 100644 --- a/src/lib/ioUtil.ts +++ b/src/lib/ioUtil.ts @@ -23,15 +23,17 @@ export const createTempDir = (parent?: string): string => { * @param dir Directory name */ export const removeDir = (dir: string) => { - fs.readdirSync(dir).forEach(item => { - const curPath = path.join(dir, item); - if (fs.statSync(curPath).isDirectory()) { - removeDir(curPath); - } else { - fs.unlinkSync(curPath); - } - }); - fs.rmdirSync(dir); + if (fs.existsSync(dir)) { + fs.readdirSync(dir).forEach(item => { + const curPath = path.join(dir, item); + if (fs.statSync(curPath).isDirectory()) { + removeDir(curPath); + } else { + fs.unlinkSync(curPath); + } + }); + fs.rmdirSync(dir); + } }; /** diff --git a/src/lib/validator.test.ts b/src/lib/validator.test.ts index afa2d3a7d..ff5b52ed3 100644 --- a/src/lib/validator.test.ts +++ b/src/lib/validator.test.ts @@ -1,4 +1,9 @@ -import { hasValue, validateForNonEmptyValue } from "./validator"; +import { + hasValue, + isInteger, + isPortNumber, + validateForNonEmptyValue +} from "./validator"; describe("Tests on validator helper functions", () => { it("Test hasValue function", () => { @@ -10,6 +15,31 @@ describe("Tests on validator helper functions", () => { expect(hasValue(" b ")).toBe(true); expect(hasValue(" a ")).toBe(true); }); + it("Test isInteger function", () => { + expect(isInteger("")).toBe(false); + expect(isInteger(undefined)).toBe(false); + expect(isInteger(null)).toBe(false); + + expect(isInteger("-10")).toBe(false); + expect(isInteger("+10")).toBe(false); + expect(isInteger("010")).toBe(false); + expect(isInteger("10.0")).toBe(false); + expect(isInteger("80")).toBe(true); + }); + it("Test isPortNumber function", () => { + expect(isPortNumber("")).toBe(false); + expect(isPortNumber(undefined)).toBe(false); + expect(isPortNumber(null)).toBe(false); + + expect(isPortNumber("-10")).toBe(false); + expect(isPortNumber("+10")).toBe(false); + expect(isPortNumber("010")).toBe(false); + expect(isPortNumber("10.0")).toBe(false); + expect(isPortNumber("80")).toBe(true); + expect(isPortNumber("8080")).toBe(true); + expect(isPortNumber("0")).toBe(false); + expect(isPortNumber("65536")).toBe(false); + }); it("Test validateForNonEmptyValue function", () => { // expect "error" to be returned ["", undefined, null].forEach(val => { diff --git a/src/lib/validator.ts b/src/lib/validator.ts index d91df3a8b..f8429a772 100644 --- a/src/lib/validator.ts +++ b/src/lib/validator.ts @@ -15,6 +15,32 @@ export const hasValue = (val: undefined | null | string): val is string => { return val !== undefined && val !== null && val !== ""; }; +/** + * Returns true of val is not undefined, not null and not an empty string + * and can be converted to integer. + * + * @param val Value to inspect + */ +export const isInteger = (val: undefined | null | string): val is string => { + if (val === undefined || val === null || val === "") { + return false; + } + return /^[1-9]\d+$/.test(val); +}; + +/** + * Returns true of val is a port number. + * + * @param val Value to inspect + */ +export const isPortNumber = (val: undefined | null | string): val is string => { + if (!isInteger(val)) { + return false; + } + const port = parseInt(val, 10); + return port > 0 && port <= 65535; +}; + /** * Returns err if val is undefined, null or empty string. Returns * otherwise empty string. From 891fbcf6d0e572c00f78e09c43a265a4b2498364 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Thu, 30 Jan 2020 10:24:32 -0800 Subject: [PATCH 2/6] remove failing test --- src/commands/service/create.test.ts | 18 ------------------ src/commands/service/create.ts | 12 ++++++++++-- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/commands/service/create.test.ts b/src/commands/service/create.test.ts index b5eb6576b..b9d58f85a 100644 --- a/src/commands/service/create.test.ts +++ b/src/commands/service/create.test.ts @@ -27,7 +27,6 @@ afterAll(() => { }); beforeEach(() => { - jest.clearAllMocks(); jest.resetAllMocks(); }); @@ -155,23 +154,6 @@ describe("Test execute function", () => { removeDir(testServiceName); // housekeeping } }); - it("Positive test", async () => { - const testServiceName = uuid(); - const exitFn = jest.fn(); - jest.spyOn(config, "bedrockFileInfo").mockImplementation(() => - Promise.resolve({ - exist: true, - hasVariableGroups: true - }) - ); - try { - await execute(testServiceName, getMockValues(), exitFn); - expect(exitFn).toBeCalledTimes(1); - expect(exitFn.mock.calls).toEqual([[0]]); - } finally { - removeDir(testServiceName); // housekeeping - } - }); }); describe("Adding a service to a repo directory", () => { diff --git a/src/commands/service/create.ts b/src/commands/service/create.ts index a6b20825c..a6f18649f 100644 --- a/src/commands/service/create.ts +++ b/src/commands/service/create.ts @@ -48,7 +48,15 @@ export const fetchValues = (opts: ICommandOptions) => { if (!isPortNumber(opts.k8sServicePort)) { throw new Error("value for --k8s-service-port is not a value port number"); } - const bedrock = Bedrock(); + + let variableGroups: string[] = []; + + try { + const bedrock = Bedrock(); + variableGroups = bedrock.variableGroups || []; + } catch (_) { + // NO-OP + } let middlewaresArray: string[] = []; if (opts.middlewares && opts.middlewares.trim()) { @@ -70,7 +78,7 @@ export const fetchValues = (opts: ICommandOptions) => { middlewares: opts.middlewares, middlewaresArray, packagesDir: opts.packagesDir, - variableGroups: bedrock.variableGroups || [] + variableGroups }; // values need not be validated (that's do not need From 592b342aa965e17919b8f003f6dfafa901ef0a17 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Thu, 30 Jan 2020 13:40:48 -0800 Subject: [PATCH 3/6] remove unwanted files --- src/commands/hld/pipeline.decorator.json | 47 ------------------------ 1 file changed, 47 deletions(-) delete mode 100644 src/commands/hld/pipeline.decorator.json diff --git a/src/commands/hld/pipeline.decorator.json b/src/commands/hld/pipeline.decorator.json deleted file mode 100644 index 095e5d4ca..000000000 --- a/src/commands/hld/pipeline.decorator.json +++ /dev/null @@ -1,47 +0,0 @@ -{ - "command": "install-manifest-pipeline", - "alias": "p", - "description": "Install the manifest generation pipeline to your Azure DevOps instance. Default values are set in spk-config.yaml and can be loaded via spk init or overriden via option flags.", - "options": [ - { - "arg": "-n, --pipeline-name ", - "description": "Name of the pipeline to be created", - "defaultValue": "" - }, - { - "arg": "-p, --personal-access-token ", - "description": "Personal Access Token", - "defaultValue": "" - }, - { - "arg": "-o, --org-name ", - "description": "Organization Name for Azure DevOps", - "defaultValue": "" - }, - { - "arg": "-r, --hld-name ", - "description": "HLD Repository Name in Azure DevOps", - "defaultValue": "" - }, - { - "arg": "-u, --hld-url ", - "description": "HLD Repository URL", - "defaultValue": "" - }, - { - "arg": "-m, --manifest-url ", - "description": "Manifest Repository URL", - "defaultValue": "" - }, - { - "arg": "-d, --devops-project ", - "description": "Azure DevOps Project", - "defaultValue": "" - }, - { - "arg": "-b, --build-script-url ", - "description": "Build Script URL. By default it is 'https://raw.githubusercontent.com/Microsoft/bedrock/master/gitops/azure-devops/build.sh'.", - "defaultValue": "https://raw.githubusercontent.com/Microsoft/bedrock/master/gitops/azure-devops/build.sh" - } - ] -} From 06349887517a908eb535d6c9bd5741f57df11928 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Thu, 30 Jan 2020 14:53:18 -0800 Subject: [PATCH 4/6] incorporated comments from review --- src/commands/service/create.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/commands/service/create.ts b/src/commands/service/create.ts index e88ce0457..62bd30bf1 100644 --- a/src/commands/service/create.ts +++ b/src/commands/service/create.ts @@ -87,11 +87,8 @@ export const fetchValues = (opts: ICommandOptions) => { variableGroups }; - // values need not be validated (that's do not need - // to do is type === "string" check because default values - // are provided in the commander and most of them - // are "" except for gitPush is false and k8sServicePort - // is "80" + // Values do not need to be validated + // as they are mostly provided by Commander. return values; }; From d1e0ac09ecb9e74808c02d57536facef640b20c3 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Thu, 30 Jan 2020 18:58:40 -0800 Subject: [PATCH 5/6] update code based on review feedback --- src/commands/service/create.ts | 6 ++--- src/lib/validator.test.ts | 46 +++++++++++++++++----------------- src/lib/validator.ts | 9 ++++--- 3 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/commands/service/create.ts b/src/commands/service/create.ts index 62bd30bf1..2abcbd1ff 100644 --- a/src/commands/service/create.ts +++ b/src/commands/service/create.ts @@ -18,7 +18,7 @@ import { generateStarterAzurePipelinesYaml } from "../../lib/fileutils"; import { checkoutCommitPushCreatePRLink } from "../../lib/gitutils"; -import { isPortNumber } from "../../lib/validator"; +import { isPortNumberString } from "../../lib/validator"; import { logger } from "../../logger"; import { IBedrockFileInfo, IHelmConfig, IUser } from "../../types"; import decorator from "./create.decorator.json"; @@ -48,8 +48,8 @@ export interface ICommandValues extends ICommandOptions { } export const fetchValues = (opts: ICommandOptions) => { - if (!isPortNumber(opts.k8sBackendPort)) { - throw new Error("value for --k8s-service-port is not a value port number"); + if (!isPortNumberString(opts.k8sBackendPort)) { + throw new Error("value for --k8s-service-port is not a valid port number"); } let variableGroups: string[] = []; diff --git a/src/lib/validator.test.ts b/src/lib/validator.test.ts index ff5b52ed3..68bc45ae6 100644 --- a/src/lib/validator.test.ts +++ b/src/lib/validator.test.ts @@ -1,7 +1,7 @@ import { hasValue, - isInteger, - isPortNumber, + isIntegerString, + isPortNumberString, validateForNonEmptyValue } from "./validator"; @@ -15,30 +15,30 @@ describe("Tests on validator helper functions", () => { expect(hasValue(" b ")).toBe(true); expect(hasValue(" a ")).toBe(true); }); - it("Test isInteger function", () => { - expect(isInteger("")).toBe(false); - expect(isInteger(undefined)).toBe(false); - expect(isInteger(null)).toBe(false); + it("Test isIntegerString function", () => { + expect(isIntegerString("")).toBe(false); + expect(isIntegerString(undefined)).toBe(false); + expect(isIntegerString(null)).toBe(false); - expect(isInteger("-10")).toBe(false); - expect(isInteger("+10")).toBe(false); - expect(isInteger("010")).toBe(false); - expect(isInteger("10.0")).toBe(false); - expect(isInteger("80")).toBe(true); + expect(isIntegerString("-10")).toBe(false); + expect(isIntegerString("+10")).toBe(false); + expect(isIntegerString("010")).toBe(false); + expect(isIntegerString("10.0")).toBe(false); + expect(isIntegerString("80")).toBe(true); }); - it("Test isPortNumber function", () => { - expect(isPortNumber("")).toBe(false); - expect(isPortNumber(undefined)).toBe(false); - expect(isPortNumber(null)).toBe(false); + it("Test isPortNumberString function", () => { + expect(isPortNumberString("")).toBe(false); + expect(isPortNumberString(undefined)).toBe(false); + expect(isPortNumberString(null)).toBe(false); - expect(isPortNumber("-10")).toBe(false); - expect(isPortNumber("+10")).toBe(false); - expect(isPortNumber("010")).toBe(false); - expect(isPortNumber("10.0")).toBe(false); - expect(isPortNumber("80")).toBe(true); - expect(isPortNumber("8080")).toBe(true); - expect(isPortNumber("0")).toBe(false); - expect(isPortNumber("65536")).toBe(false); + expect(isPortNumberString("-10")).toBe(false); + expect(isPortNumberString("+10")).toBe(false); + expect(isPortNumberString("010")).toBe(false); + expect(isPortNumberString("10.0")).toBe(false); + expect(isPortNumberString("80")).toBe(true); + expect(isPortNumberString("8080")).toBe(true); + expect(isPortNumberString("0")).toBe(false); + expect(isPortNumberString("65536")).toBe(false); }); it("Test validateForNonEmptyValue function", () => { // expect "error" to be returned diff --git a/src/lib/validator.ts b/src/lib/validator.ts index f8429a772..5cfe8c072 100644 --- a/src/lib/validator.ts +++ b/src/lib/validator.ts @@ -21,7 +21,10 @@ export const hasValue = (val: undefined | null | string): val is string => { * * @param val Value to inspect */ -export const isInteger = (val: undefined | null | string): val is string => { +export const isIntegerString = (val: unknown): val is string => { + if (typeof val !== "string") { + return false; + } if (val === undefined || val === null || val === "") { return false; } @@ -33,8 +36,8 @@ export const isInteger = (val: undefined | null | string): val is string => { * * @param val Value to inspect */ -export const isPortNumber = (val: undefined | null | string): val is string => { - if (!isInteger(val)) { +export const isPortNumberString = (val: unknown): val is string => { + if (!isIntegerString(val)) { return false; } const port = parseInt(val, 10); From 47d32dcd01ae27eb65d44250efa1b6ec61076511 Mon Sep 17 00:00:00 2001 From: Dennis Seah Date: Fri, 31 Jan 2020 10:33:35 -0800 Subject: [PATCH 6/6] remove try catch block around Bedrock function call --- src/commands/service/create.test.ts | 5 ++++- src/commands/service/create.ts | 8 ++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/commands/service/create.test.ts b/src/commands/service/create.test.ts index ea19afc43..e76a69c74 100644 --- a/src/commands/service/create.test.ts +++ b/src/commands/service/create.test.ts @@ -4,6 +4,7 @@ import { promisify } from "util"; import uuid from "uuid/v4"; import { Bedrock } from "../../config"; import * as config from "../../config"; +import { DEFAULT_CONTENT as BedrockMockedContent } from "../../lib/bedrockYaml"; import { checkoutCommitPushCreatePRLink } from "../../lib/gitutils"; import { createTempDir, removeDir } from "../../lib/ioUtil"; import { @@ -27,7 +28,7 @@ afterAll(() => { }); beforeEach(() => { - jest.resetAllMocks(); + jest.clearAllMocks(); }); const mockValues: ICommandValues = { @@ -90,6 +91,7 @@ describe("Test fetchValues function", () => { } }); it("Postive test: with middlewares value", () => { + jest.spyOn(config, "Bedrock").mockReturnValueOnce(BedrockMockedContent); const mocked = getMockValues(); mocked.middlewares = "mid1, mid2"; // space after comma is intentional, expecting trimming to happen const result = fetchValues(mocked); @@ -97,6 +99,7 @@ describe("Test fetchValues function", () => { }); it("Postive test", () => { const mocked = getMockValues(); + jest.spyOn(config, "Bedrock").mockReturnValueOnce(BedrockMockedContent); const result = fetchValues(mocked); expect(result).toEqual(mocked); }); diff --git a/src/commands/service/create.ts b/src/commands/service/create.ts index 2abcbd1ff..c7cf9f7ae 100644 --- a/src/commands/service/create.ts +++ b/src/commands/service/create.ts @@ -54,12 +54,8 @@ export const fetchValues = (opts: ICommandOptions) => { let variableGroups: string[] = []; - try { - const bedrock = Bedrock(); - variableGroups = bedrock.variableGroups || []; - } catch (_) { - // NO-OP - } + const bedrock = Bedrock(); + variableGroups = bedrock.variableGroups || []; let middlewaresArray: string[] = []; if (opts.middlewares && opts.middlewares.trim()) {