Code Improvement: Error handling on sam cli calls #574

awschristou · 2019-05-23T20:11:06Z

Types of changes

Technical debt (change which improves the maintainability of the codebase, but does not change functionality)

Description

Consolidated all of the SAM CLI error handling
Fixed problem with TestLogger that didn't properly clean up or read from the log file

Testing

Ran tests
Ran toolkit
- Created new sam app
- local run/debug of sam app
- deploy sam app

Checklist

I have read the README document
I have read the CONTRIBUTING document
My code follows the code style of this project
I have added tests to cover my changes
All new and existing tests passed
A short description of the change has been added to the CHANGELOG

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2019-05-23T20:15:51Z

Codecov Report

Merging #574 into develop will decrease coverage by 0.05%.
The diff coverage is 95.91%.

@@             Coverage Diff             @@
##           develop     #574      +/-   ##
===========================================
- Coverage    55.61%   55.55%   -0.06%     
===========================================
  Files          114      115       +1     
  Lines         4463     4435      -28     
  Branches       673      668       -5     
===========================================
- Hits          2482     2464      -18     
+ Misses        1981     1971      -10

Impacted Files	Coverage Δ
src/shared/sam/cli/samCliPackage.ts	`100% <100%> (ø)`	⬆️
src/shared/sam/cli/samCliInit.ts	`100% <100%> (+64.28%)`	⬆️
src/shared/loggerUtils.ts	`100% <100%> (+15.78%)`	⬆️
src/shared/sam/cli/samCliInfo.ts	`100% <100%> (ø)`	⬆️
src/shared/sam/cli/samCliDeploy.ts	`100% <100%> (ø)`	⬆️
src/shared/sam/cli/samCliBuild.ts	`88.88% <87.5%> (-4.14%)`	⬇️
src/shared/sam/cli/samCliInvokerUtils.ts	`92.85% <92.85%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 413c89e...db227f0. Read the comment docs.

bryceitoc9 · 2019-05-23T20:40:37Z

src/shared/loggerUtils.ts


-        return(logText.includes(str))
+        return (logText.includes(str))


Was about to ask why there were parenthesis around the return statement and then I realized that I was the one that put those in :/

Mind fixing that for me?

bryceitoc9 · 2019-05-23T20:46:45Z

src/shared/sam/cli/samCliInvokerUtils.ts

+): void {
+    if (processResult.exitCode === expectedExitCode) { return }
+
+    logger.error(`Unexpected exitcode (${processResult.exitCode})`)


Unexpected exitcode (${processResult.exitCode}), expecting (${expectedExitCode}) ?

bryceitoc9 · 2019-05-23T20:47:44Z

src/shared/sam/cli/samCliInit.ts

-    const err = new Error(`sam init encountered an error: ${error && error.message || stderr || stdout}`)
-    logger.error(err)
-    throw err
+    logAndThrowIfUnexpectedExitCode(childProcessResult, 0)


I feel like you should leave out the , 0. It feels like that should be understood.

At what points would we expect something other than 0?

Best to be explicit and make no assumptions about what default logAndThrowIfUnexpectedExitCode contains.

Thoughts on something like:

export function logAndThrowIfError(childProcessResult) { logAndThrowIfUnexpectedExitCode(childProcessResult, 0) }

It just doesn't feel like there's much reason to watch for anything aside from 0.

mpiroc · 2019-05-23T21:20:14Z

src/shared/loggerUtils.ts

-        this._logfile = logfile
-    }
+    private constructor(
+        private readonly logFolder: string,


Is there any reason that logFolder needs to be a parameter rather than just doing path.dirname(logFilePath)?

This class is not well written to handle cases other than the one hardcoded into createTestLogger. I'll see if I can clean it up a little.

mpiroc · 2019-05-23T22:00:42Z

src/shared/sam/cli/samCliBuild.ts

@@ -73,6 +76,7 @@ export class SamCliBuildInvocation {
            skipPullImage = false,
            ...params
        }: SamCliBuildInvocationArguments,
+        private readonly file: FileFunctions = getDefaultFileFunctions()


Nit: For consistency, please wrap this in a context, i.e. private readonly context: { file: FileFunctions } = ...

…develop.

…y. (#599) * Initial codelens implementation for C# (#477) * Open sam template upon sam init (#481) * C# run command (#488) Added C# run command with some tests and some refactoring to the localLambdaRunner and pythonCodeRunner files. Additional work deemed out-of-scope for this feature can be found here: #507 * Prepare for .NET Core Debug implemention. (#513) * Removing unused runtimes (#535) * Implement "Debug Locally" code lens for .NET Core. (#552) * Port changes from develop for eb07933..9c3d7c3. * Fix npm audit issues. * Port 'Switch away from Terminal to using Debug Console and Output tabs for local debug and run (#493)' from develop. * Port 'Added AWS logo with min retina size (#461) (#498)' from develop. * Port 'Handle port-waiting timeout; Catch Python debugging Errors (#494)' from develop. * Python Path Mappings now provide both drive letter casings for Windows (#495) * Port 'Explicitly show the Debug Console after each successful debugger attach (#496)' from develop. * Port 'Prevent AWS Explorer context menu items from getting added to other extension/view menus (#506)' from develop * Port 'Design for operations against resources attached to code pipelines (#503)' from develop. * Port 'childProcess times out and dies if the debugger does not attach in time (#512)' from develop. * Port 'Remove lambda views for Policy and Configuration (#505)' from develop. * Port 'Normalizing telemetry (#534)' from develop. * Port 'SAM calls that use AWS resources now correctly use an AWS profile (#554)' from develop. * Port 'SAM CLI Validation Refactor (#555)' from develop. * Port 'Fixes issue introduced in f9816c1 that caused the AWS Explorer context menu to disappear (#560)' from develop * Port 'Added a help button to the sam init wizard's runtime selection step (#538)' from develop. * Port 'Metrics overrides for result and duration (#562)Metrics overrides for result and duration (#562)' from develop. * Port 'Added vs code task to run lint fixer (#561)' from develop. * Port 'SAM CLI Validation at the start of Create New SAM App flow (#563)' from develop. * Port 'Updated the message shown when SAM CLI is newer than valid range (#569)' from develop. * Port 'SAM CLI Validation at the start of Deploy SAM App flow (#570)' from develop. * Port 'Code Improvement: sam deploy calls (#571)' from develop. * Port 'Code Improvement: sam package calls (#572)' from develop. * Port 'Telemetry data includes active AWS account ID (if configured) (#557)' from develop. * Port 'Code Improvement: sam deploy workflow (#573)' from develop. * Port 'Code Improvement: Error handling on sam cli calls (#574)' from develop. * Port 'Extension throws an error if the toolkitClientBuilder is not initialized before initial credential validation (#577)' from develop. * Port 'addUserDataToContext throws an error if a profile's credentials are nonexistent (#579)' from develop. * Port 'Ensure log folder exists prior to initialization (#583)' from develop. * Port 'Adding help buttons to deploy workflow (#584)' from develop. * Port 'Credentials are now passed to STS client correctly on validation. (#585)' from develop * Port 'Add prettier formatter and git hook (#578)' from develop. * Fix WAIT_FOR_DEBUGGER_MESSAGE for .NET Core. * Disable prettier pre-commit hook, and fix linter issues introduced by prettier. * Fix dotnet debug install issues on Mac, Linux (#592) * Instead of installing VSDBG by manually invoking crossSpawn, use existing ChildProcess utility class. * Simplify code by unconditionally installing debugger each time - the script is smart enough to determine if an install is needed. * DockerContext outputs stdout to output channel * Fix installing debugger on non-windows platforms * Fix issue that caused templates.json to be ignored when invoking .NET Core functions. (#593) * Address feedback from PR.

Due to a series of misteps, we are squashing this entire feature branch into develop. 1. We allowed `feature/dotnet` to drift too far from `develop`, which required significant manual conflict resolution to resolve. 2. We should have squashed the conflict-resolution history *before* merging develop, but we did not. Therefore in order to squash the pre-merge commits, we would need to recreate the merge and re-resolve conflicts. This would reduce our confidence in the quality of the branch. In the future: 1. We will regularly merge into feature branches to prevent drift. 2. If we do encounter a situation that involves manually porting, we will squash the ports *before* merging develop. * Initial codelens implementation for C# (#477) * Open sam template upon sam init (#481) * C# run command (#488) Added C# run command with some tests and some refactoring to the localLambdaRunner and pythonCodeRunner files. Additional work deemed out-of-scope for this feature can be found here: #507 * Prepare for .NET Core Debug implemention. (#513) * Removing unused runtimes (#535) * Implement "Debug Locally" code lens for .NET Core. (#552) * Port changes from develop for eb07933..9c3d7c3. * Fix npm audit issues. * Port 'Switch away from Terminal to using Debug Console and Output tabs for local debug and run (#493)' from develop. * Port 'Added AWS logo with min retina size (#461) (#498)' from develop. * Port 'Handle port-waiting timeout; Catch Python debugging Errors (#494)' from develop. * Python Path Mappings now provide both drive letter casings for Windows (#495) * Port 'Explicitly show the Debug Console after each successful debugger attach (#496)' from develop. * Port 'Prevent AWS Explorer context menu items from getting added to other extension/view menus (#506)' from develop * Port 'Design for operations against resources attached to code pipelines (#503)' from develop. * Port 'childProcess times out and dies if the debugger does not attach in time (#512)' from develop. * Port 'Remove lambda views for Policy and Configuration (#505)' from develop. * Port 'Normalizing telemetry (#534)' from develop. * Port 'SAM calls that use AWS resources now correctly use an AWS profile (#554)' from develop. * Port 'SAM CLI Validation Refactor (#555)' from develop. * Port 'Fixes issue introduced in f9816c1 that caused the AWS Explorer context menu to disappear (#560)' from develop * Port 'Added a help button to the sam init wizard's runtime selection step (#538)' from develop. * Port 'Metrics overrides for result and duration (#562)Metrics overrides for result and duration (#562)' from develop. * Port 'Added vs code task to run lint fixer (#561)' from develop. * Port 'SAM CLI Validation at the start of Create New SAM App flow (#563)' from develop. * Port 'Updated the message shown when SAM CLI is newer than valid range (#569)' from develop. * Port 'SAM CLI Validation at the start of Deploy SAM App flow (#570)' from develop. * Port 'Code Improvement: sam deploy calls (#571)' from develop. * Port 'Code Improvement: sam package calls (#572)' from develop. * Port 'Telemetry data includes active AWS account ID (if configured) (#557)' from develop. * Port 'Code Improvement: sam deploy workflow (#573)' from develop. * Port 'Code Improvement: Error handling on sam cli calls (#574)' from develop. * Port 'Extension throws an error if the toolkitClientBuilder is not initialized before initial credential validation (#577)' from develop. * Port 'addUserDataToContext throws an error if a profile's credentials are nonexistent (#579)' from develop. * Port 'Ensure log folder exists prior to initialization (#583)' from develop. * Port 'Adding help buttons to deploy workflow (#584)' from develop. * Port 'Credentials are now passed to STS client correctly on validation. (#585)' from develop * Port 'Add prettier formatter and git hook (#578)' from develop. * Fix WAIT_FOR_DEBUGGER_MESSAGE for .NET Core. * Disable prettier pre-commit hook, and fix linter issues introduced by prettier. * Fix dotnet debug install issues on Mac, Linux (#592) * Instead of installing VSDBG by manually invoking crossSpawn, use existing ChildProcess utility class. * Simplify code by unconditionally installing debugger each time - the script is smart enough to determine if an install is needed. * DockerContext outputs stdout to output channel * Fix installing debugger on non-windows platforms * Fix issue that caused templates.json to be ignored when invoking .NET Core functions. (#593) * Address feedback from PR.

awschristou added 2 commits May 23, 2019 11:43

Fixed TestLogger to properly check for text and clean up.

0b3ffa3

Consolidated error handling for sam cli commands

ff2e34a

awschristou added this to the Sprint 21 (2019) milestone May 23, 2019

awschristou requested review from mpiroc and bryceitoc9 May 23, 2019 20:11

awschristou added 2 commits May 23, 2019 13:23

WIP - gratuitous commit to force codebuild to re-run

7dd5a47

WIP - gratuitous commit to force codebuild to re-run (revert)

e116a0d

bryceitoc9 reviewed May 23, 2019

View reviewed changes

mpiroc approved these changes May 23, 2019

View reviewed changes

awschristou added 7 commits May 23, 2019 16:09

Feedback

a70b733

TestLogger now handles log path automatically

cb56d5f

Feedback

e41d8af

Updated tests based on feedback

792c3c2

Merge branch 'develop' into awschristou/tidy-childprocess-exitcode

cd16a31

Merge branch 'develop' into awschristou/tidy-childprocess-exitcode

5417693

Feedback

db227f0

bryceitoc9 approved these changes May 24, 2019

View reviewed changes

awschristou merged commit 9f6da6a into develop May 24, 2019

awschristou deleted the awschristou/tidy-childprocess-exitcode branch May 24, 2019 16:00

mpiroc added a commit that referenced this pull request May 30, 2019

Port 'Code Improvement: Error handling on sam cli calls (#574)' from …

abbd852

…develop.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Improvement: Error handling on sam cli calls #574

Code Improvement: Error handling on sam cli calls #574

awschristou commented May 23, 2019

codecov-io commented May 23, 2019 •

edited

Loading

bryceitoc9 May 23, 2019

bryceitoc9 May 23, 2019

bryceitoc9 May 23, 2019

awschristou May 23, 2019

bryceitoc9 May 23, 2019

mpiroc May 23, 2019

awschristou May 23, 2019

mpiroc May 23, 2019

Code Improvement: Error handling on sam cli calls #574

Code Improvement: Error handling on sam cli calls #574

Conversation

awschristou commented May 23, 2019

Types of changes

Description

Testing

Checklist

License

codecov-io commented May 23, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 23, 2019 •

edited

Loading