Code Improvement: sam deploy workflow #573

awschristou · 2019-05-23T01:35:55Z

Types of changes

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

Description

improved user messaging and logging for failed deploys
split up the deploy sam workflow code, and added more stub points
added test coverage to the sam workflow code

Testing

Created new tests
Ran through deploy scenarios in the toolkit

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.

bryceitoc9 · 2019-05-23T17:35:34Z

src/lambda/commands/deploySamApplication.ts

-        // TODO: Is this the right way to provide this feedback?
-        vscode.window.showInformationMessage(msg)
+
+        params.window.showInformationMessage(localize(


Do we need two alerts?

The output channel can be thought of as a more verbose accounting of what's going on.
Because deploys take some amount of time, users have likely moved on to do other things, and the showInfo is a toast to inform that deploy has completed. (The toast is standard across JB/VS toolkits)

Here's a sample of the two (remember that users may not have the Output pane open if they've moved on to do other things while deploying)

I agree that we should avoid spamming the user with alerts. But as Chris mentioned, in this case the operation can take quite some time, so I think that a toast makes sense in this case.

Yeah, now that I see it in context, it should be fine.

bryceitoc9 · 2019-05-23T17:41:42Z

src/lambda/commands/deploySamApplication.ts

@@ -186,35 +204,104 @@ async function buildPackageDeploy(params: {
            params.invoker,
            params.channelLogger.logger,
        )
+    } catch (err) {


How come only deployOperation has a catch? This will also generate two logs, one at this level and one from deploy() via outputDeployError...is this what we want?

We don't really want this catch here. If we can move enhanceAwsCloudFormationInstructions logic elsewhere (or remote it outright), we could remove this catch.

I don't see why we can't call enhanceAwsCloudFormationInstructions in the other catch statement, that also has access to deployParams.

Unfortunately, we need it if we want to allow the users to do a fast-retry.

bryceitoc9 · 2019-05-23T17:43:28Z

src/test/shared/fakeBasicLogger.ts

@@ -0,0 +1,36 @@
+/*!
+ * Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


bryceitoc9 · 2019-05-23T17:44:02Z

src/test/lambda/commands/deploySamApplication.test.ts

@@ -0,0 +1,346 @@
+/*!
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.


2018 -> 2019 ; only do this for new files for now (dashes, commas, and spaces aren't supported until 557 merges)

codecov-io · 2019-05-23T18:16:39Z

Codecov Report

Merging #573 into develop will increase coverage by 1.71%.
The diff coverage is 84.9%.

@@             Coverage Diff             @@
##           develop     #573      +/-   ##
===========================================
+ Coverage    53.82%   55.54%   +1.71%     
===========================================
  Files          112      112              
  Lines         4362     4382      +20     
  Branches       657      658       +1     
===========================================
+ Hits          2348     2434      +86     
+ Misses        2014     1948      -66

Impacted Files	Coverage Δ
src/lambda/commands/deploySamApplication.ts	`90.47% <84.9%> (+62.35%)`	⬆️
src/shared/sam/cli/samCliBuild.ts	`93.02% <0%> (+20.93%)`	⬆️
src/shared/awsContext.ts	`100% <0%> (+25%)`	⬆️
src/shared/sam/cli/samCliDeploy.ts	`100% <0%> (+45%)`	⬆️
src/shared/sam/cli/samCliPackage.ts	`100% <0%> (+64.28%)`	⬆️

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 b796d7b...58f93b2. Read the comment docs.

src/lambda/commands/deploySamApplication.ts

mpiroc · 2019-05-23T19:46:01Z

src/lambda/commands/deploySamApplication.ts

-        // TODO: Is this the right way to provide this feedback?
-        vscode.window.showInformationMessage(msg)
+
+        params.window.showInformationMessage(localize(


I agree that we should avoid spamming the user with alerts. But as Chris mentioned, in this case the operation can take quite some time, so I think that a toast makes sense in this case.

src/lambda/commands/deploySamApplication.ts

…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 3 commits May 22, 2019 15:22

Split up deploy routine

2594b93

Common error output

fb07e9c

Added test coverage to sam app deployment

fb36cb2

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

awschristou requested review from mpiroc and bryceitoc9 May 23, 2019 17:33

bryceitoc9 reviewed May 23, 2019

View reviewed changes

Feedback - updated year in comment

749ab6b

mpiroc reviewed May 23, 2019

View reviewed changes

Feedback - syntax

58f93b2

mpiroc approved these changes May 23, 2019

View reviewed changes

Merge branch 'develop' into awschristou/tidy-deploy-workflow

332e19e

bryceitoc9 approved these changes May 24, 2019

View reviewed changes

awschristou merged commit 413c89e into develop May 24, 2019

awschristou deleted the awschristou/tidy-deploy-workflow branch May 24, 2019 00:26

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

Port 'Code Improvement: sam deploy workflow (#573)' from develop.

a561502

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Improvement: sam deploy workflow #573

Code Improvement: sam deploy workflow #573

awschristou commented May 23, 2019

bryceitoc9 May 23, 2019

awschristou May 23, 2019

mpiroc May 23, 2019

bryceitoc9 May 23, 2019

bryceitoc9 May 23, 2019

awschristou May 23, 2019

bryceitoc9 May 23, 2019

bryceitoc9 May 23, 2019

bryceitoc9 May 23, 2019

codecov-io commented May 23, 2019 •

edited

Loading

mpiroc May 23, 2019

		@@ -0,0 +1,36 @@
		/*!
		* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -0,0 +1,346 @@
		/*!
		* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Code Improvement: sam deploy workflow #573

Code Improvement: sam deploy workflow #573

Conversation

awschristou commented May 23, 2019

Types of changes

Description

Testing

Checklist

License

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

Choose a reason for hiding this comment

codecov-io commented May 23, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

codecov-io commented May 23, 2019 •

edited

Loading