Code Improvement: Error handling on sam cli calls (#574)

* Fixed TestLogger to properly check for text and clean up.
* Consolidated error handling for sam cli commands

src/shared/loggerUtils.ts

@@ -11,39 +11,45 @@ import * as filesystemUtilities from './filesystemUtilities'
 import * as l from './logger'
 
 export class TestLogger {
-    private readonly _logger: l.Logger
-    private readonly _logfile: string
-
-    // initializes a default logger. This persists through all tests.
-    // initializing a default logger means that any tested files with logger statements will work.
-    // as a best practice, please initialize a TestLogger before running tests on a file with logger statements.
-    private constructor(logfile: string, logger: l.Logger) {
-        this._logger = logger
-        this._logfile = logfile
-    }
+    /**
+     * initializes a default logger. This persists through all tests.
+     * initializing a default logger means that any tested files with logger statements will work.
+     * as a best practice, please initialize a TestLogger before running tests on a file with logger statements.
+     *
+     * @param logFolder - Folder to be managed by this object. Will be deleted on cleanup
+     * @param logger - Logger to work with
+     */
+    private constructor(
+        private readonly logFolder: string,
+        private readonly logger: l.Logger
+    ) { }
 
     // cleanupLogger clears out the logger's transports, but the logger will still exist as a default
     // this means that the default logger will still work for other files but will output an error
     public async cleanupLogger(): Promise<void> {
-        this._logger.releaseLogger()
-        if (await filesystemUtilities.fileExists(this._logfile)) {
-            await del(this._logfile, { force: true })
+        this.logger.releaseLogger()
+        if (await filesystemUtilities.fileExists(this.logFolder)) {
+            await del(this.logFolder, { force: true })
         }
     }
 
     public async logContainsText(str: string): Promise<boolean> {
-        const logText = await filesystemUtilities.readFileAsString(this._logfile as string)
+        const logText = await filesystemUtilities.readFileAsString(TestLogger.getLogPath(this.logFolder))
 
-        return(logText.includes(str))
+        return logText.includes(str)
     }
 
     public static async createTestLogger(): Promise<TestLogger> {
-        const logfile = await filesystemUtilities.makeTemporaryToolkitFolder()
+        const logFolder = await filesystemUtilities.makeTemporaryToolkitFolder()
         const logger = await l.initialize({
-            logPath: path.join(logfile, 'temp.log'),
+            logPath: TestLogger.getLogPath(logFolder),
             logLevel: 'debug'
         })
 
-        return new TestLogger(logfile, logger)
+        return new TestLogger(logFolder, logger)
+    }
+
+    private static getLogPath(logFolder: string): string {
+        return path.join(logFolder, 'temp.log')
     }
 }

src/shared/sam/cli/samCliBuild.ts

@@ -7,9 +7,8 @@
 
 import { fileExists } from '../../filesystemUtilities'
 import { getLogger, Logger } from '../../logger'
-import { ChildProcessResult } from '../../utilities/childProcess'
 import { DefaultSamCliProcessInvoker } from './samCliInvoker'
-import { SamCliProcessInvoker } from './samCliInvokerUtils'
+import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils'
 
 export interface SamCliBuildInvocationArguments {
     /**
@@ -50,6 +49,10 @@ export interface SamCliBuildInvocationArguments {
     manifestPath?: string
 }
 
+export interface FileFunctions {
+    fileExists: typeof fileExists
+}
+
 export class SamCliBuildInvocation {
     private readonly buildDir: string
     private readonly baseDir?: string
@@ -73,6 +76,7 @@ export class SamCliBuildInvocation {
             skipPullImage = false,
             ...params
         }: SamCliBuildInvocationArguments,
+        private readonly context: { file: FileFunctions } = { file: getDefaultFileFunctions() },
     ) {
         this.buildDir = params.buildDir
         this.baseDir = params.baseDir
@@ -85,7 +89,6 @@ export class SamCliBuildInvocation {
     }
 
     public async execute(): Promise<void> {
-        const logger: Logger = getLogger()
         await this.validate()
 
         const invokeArgs: string[] = [
@@ -100,24 +103,11 @@ export class SamCliBuildInvocation {
         this.addArgumentIf(invokeArgs, !!this.skipPullImage, '--skip-pull-image')
         this.addArgumentIf(invokeArgs, !!this.manifestPath, '--manifest', this.manifestPath!)
 
-        const { exitCode, error, stderr, stdout }: ChildProcessResult = await this.invoker.invoke(
+        const childProcessResult = await this.invoker.invoke(
             ...invokeArgs
         )
 
-        if (exitCode === 0) {
-            return
-        }
-
-        console.error('SAM CLI error')
-        console.error(`Exit code: ${exitCode}`)
-        console.error(`Error: ${error}`)
-        console.error(`stderr: ${stderr}`)
-        console.error(`stdout: ${stdout}`)
-
-        const err =
-            new Error(`sam build encountered an error: ${error && error.message ? error.message : stderr || stdout}`)
-        logger.error(err)
-        throw err
+        logAndThrowIfUnexpectedExitCode(childProcessResult, 0)
     }
 
     private addArgumentIf(args: string[], addIfConditional: boolean, ...argsToAdd: string[]) {
@@ -127,11 +117,18 @@ export class SamCliBuildInvocation {
     }
 
     private async validate(): Promise<void> {
-        const logger: Logger = getLogger()
-        if (!await fileExists(this.templatePath)) {
+        if (!await this.context.file.fileExists(this.templatePath)) {
+            const logger: Logger = getLogger()
+
             const err = new Error(`template path does not exist: ${this.templatePath}`)
             logger.error(err)
             throw err
         }
     }
 }
+
+function getDefaultFileFunctions(): FileFunctions {
+    return {
+        fileExists
+    }
+}

src/shared/sam/cli/samCliDeploy.ts

@@ -6,9 +6,8 @@
 'use strict'
 
 import { BasicLogger, getLogger } from '../../logger'
-import { ChildProcessResult } from '../../utilities/childProcess'
 import { map } from '../../utilities/collectionUtils'
-import { SamCliProcessInvoker } from './samCliInvokerUtils'
+import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils'
 
 export interface SamCliDeployParameters {
     templateFile: string
@@ -41,20 +40,7 @@ export async function runSamCliDeploy(
         args.push('--parameter-overrides', ...overrides)
     }
 
-    const { exitCode, error, stderr, stdout }: ChildProcessResult = await invoker.invoke(...args)
+    const childProcessResult = await invoker.invoke(...args)
 
-    if (exitCode === 0) {
-        return
-    }
-
-    console.error('SAM deploy error')
-    console.error(`Exit code: ${exitCode}`)
-    console.error(`Error: ${error}`)
-    console.error(`stderr: ${stderr}`)
-    console.error(`stdout: ${stdout}`)
-
-    const message = error && error.message ? error.message : stderr || stdout
-    const err = new Error(`sam deploy encountered an error: ${message}`)
-    logger.error(err)
-    throw err
+    logAndThrowIfUnexpectedExitCode(childProcessResult, 0, logger)
 }

src/shared/sam/cli/samCliInfo.ts

@@ -6,9 +6,8 @@
 'use strict'
 
 import { getLogger, Logger } from '../../logger'
-import { ChildProcessResult } from '../../utilities/childProcess'
 import { DefaultSamCliProcessInvoker } from './samCliInvoker'
-import { SamCliProcessInvoker } from './samCliInvokerUtils'
+import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils'
 
 /**
  * Maps out the response text from the sam cli command `sam --info`
@@ -24,33 +23,17 @@ export class SamCliInfoInvocation {
     }
 
     public async execute(): Promise<SamCliInfoResponse> {
-        const logger: Logger = getLogger()
-        const { error, exitCode, stderr, stdout }: ChildProcessResult = await this.invoker.invoke('--info')
+        const childProcessResult = await this.invoker.invoke('--info')
 
-        if (exitCode === 0) {
-            const response = this.convertOutput(stdout)
+        logAndThrowIfUnexpectedExitCode(childProcessResult, 0)
 
-            if (!!response) {
-                return response
-            }
+        const response = this.convertOutput(childProcessResult.stdout)
 
+        if (!response) {
             throw new Error('SAM CLI did not return expected data')
         }
 
-        console.error('SAM CLI error')
-        console.error(`Exit code: ${exitCode}`)
-        console.error(`Error: ${error}`)
-        console.error(`stderr: ${stderr}`)
-        console.error(`stdout: ${stdout}`)
-
-        const err = new Error(
-            `sam --info encountered an error: ${error}
-    ${error && error.message ? 'message: ' + error.message : ''}
-    stderr : ${stderr}
-    stdout : ${stdout}`
-        )
-        logger.error(err)
-        throw err
+        return response
     }
 
     /**

src/shared/sam/cli/samCliInit.ts

@@ -6,9 +6,7 @@
 'use strict'
 
 import { SamLambdaRuntime } from '../../../lambda/models/samLambdaRuntime'
-import { getLogger, Logger } from '../../logger'
-import { ChildProcessResult } from '../../utilities/childProcess'
-import { SamCliProcessInvoker } from './samCliInvokerUtils'
+import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils'
 
 export interface SamCliInitArgs {
     runtime: SamLambdaRuntime
@@ -20,25 +18,12 @@ export async function runSamCliInit(
     initArguments: SamCliInitArgs,
     invoker: SamCliProcessInvoker
 ): Promise<void> {
-    const { exitCode, error, stderr, stdout }: ChildProcessResult = await invoker.invoke(
+    const childProcessResult = await invoker.invoke(
         { cwd: initArguments.location },
         'init',
         '--name', initArguments.name,
         '--runtime', initArguments.runtime
     )
 
-    if (exitCode === 0) {
-        return
-    }
-
-    console.error('SAM CLI error')
-    console.error(`Exit code: ${exitCode}`)
-    console.error(`Error: ${error}`)
-    console.error(`stderr: ${stderr}`)
-    console.error(`stdout: ${stdout}`)
-
-    const logger: Logger = getLogger()
-    const err = new Error(`sam init encountered an error: ${error && error.message || stderr || stdout}`)
-    logger.error(err)
-    throw err
+    logAndThrowIfUnexpectedExitCode(childProcessResult, 0)
 }

src/shared/sam/cli/samCliInvokerUtils.ts

@@ -6,9 +6,37 @@
 'use strict'
 
 import { SpawnOptions } from 'child_process'
+import { BasicLogger, getLogger } from '../../logger'
 import { ChildProcessResult } from '../../utilities/childProcess'
 
 export interface SamCliProcessInvoker {
     invoke(options: SpawnOptions, ...args: string[]): Promise<ChildProcessResult>
     invoke(...args: string[]): Promise<ChildProcessResult>
 }
+
+export function logAndThrowIfUnexpectedExitCode(
+    processResult: ChildProcessResult,
+    expectedExitCode: number,
+    logger: BasicLogger = getLogger(),
+): void {
+    if (processResult.exitCode === expectedExitCode) { return }
+
+    logger.error(`Unexpected exitcode (${processResult.exitCode}), expecting (${expectedExitCode})`)
+    logger.error(`Error: ${processResult.error}`)
+    logger.error(`stderr: ${processResult.stderr}`)
+    logger.error(`stdout: ${processResult.stdout}`)
+
+    let message: string | undefined
+
+    if (processResult.error instanceof Error) {
+        if (processResult.error.message) {
+            message = processResult.error.message
+        }
+    }
+
+    if (!message) {
+        message = processResult.stderr || processResult.stdout || 'No message available'
+    }
+
+    throw new Error(`Error with child process: ${message}`)
+}

src/shared/sam/cli/samCliPackage.ts

@@ -6,8 +6,7 @@
 'use strict'
 
 import { BasicLogger, getLogger } from '../../logger'
-import { ChildProcessResult } from '../../utilities/childProcess'
-import { SamCliProcessInvoker } from './samCliInvokerUtils'
+import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils'
 
 export interface SamCliPackageParameters {
     /**
@@ -28,7 +27,7 @@ export async function runSamCliPackage(
     invoker: SamCliProcessInvoker,
     logger: BasicLogger = getLogger(),
 ): Promise<void> {
-    const { exitCode, error, stderr, stdout }: ChildProcessResult = await invoker.invoke(
+    const childProcessResult = await invoker.invoke(
         'package',
         '--template-file', packageArguments.sourceTemplateFile,
         '--s3-bucket', packageArguments.s3Bucket,
@@ -37,18 +36,5 @@ export async function runSamCliPackage(
         '--profile', packageArguments.profile
     )
 
-    if (exitCode === 0) {
-        return
-    }
-
-    console.error('SAM package error')
-    console.error(`Exit code: ${exitCode}`)
-    console.error(`Error: ${error}`)
-    console.error(`stderr: ${stderr}`)
-    console.error(`stdout: ${stdout}`)
-
-    const message = error && error.message ? error.message : stderr || stdout
-    const err = new Error(`sam package encountered an error: ${message}`)
-    logger.error(err)
-    throw err
+    logAndThrowIfUnexpectedExitCode(childProcessResult, 0, logger)
 }