Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a help button to the sam init wizard's runtime selection step #538

Merged
merged 10 commits into from
May 16, 2019

Conversation

bryceitoc9
Copy link
Contributor

@bryceitoc9 bryceitoc9 commented May 8, 2019

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Infrastructure (change which improves the lifecycle management (CI/CD, build, package, deploy, lint, etc) of the application, but does not change functionality.)
  • Technical debt (change which improves the maintainability of the codebase, but does not change functionality)
  • Testing (change which modifies or adds test coverage, but does not change functionality)
  • Documentation (change which modifies or adds documentation, but does not change functionality)

Description

Adding help buttons to our wizards with direct links to documentation. Looking to add more as part of this PR.

DONE:

Motivation and Context

Users might be confused by some of our prompts (especially regarding AWS-specific features). This provides direct links to give instant context.

Related Issue(s)

#530

Testing

Tested light and dark themes, icon appears in wizard.

Screenshots (if appropriate)

helpbutton_superior
helpbutton_inferior

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.

* @param path Extension's absolute path
*/
export function initializeButtons(context: ExtensionContext): void {
resourcesAbsolutePath = context.asAbsolutePath('resources')
Copy link
Contributor

@mpiroc mpiroc May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to avoid global init functions like this where possible. The init wizard is only used in createNewSamApp, which already has the extension context available. Suggest instead to:

  1. Replace context: Pick<vscode.ExtensionContext, 'globalState'> with context: Pick<vscode.ExtensionContext, 'globalState' | 'asAbsolutePath'> in the signature of createNewSamApp.
  2. Add the following to samInitWizard.ts:
type CreateNewSamAppWizardArguments = Pick<vscode.ExtensionContext, 'asAbsolutePath'>
...
public constructor(
    private readonly args: CreateNewSamAppWizardArguments,
    ...
) {
  1. Pass args.asAbsolutePath to promptUserForRuntime.

Copy link
Contributor Author

@bryceitoc9 bryceitoc9 May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we think that this or other custom buttons might be used outside the SAM app? I positioned the buttons.ts file in a way that it can be used in a more generic fashion, and so that way we can add additional buttons with different icons.

Are there any ways we could make this happen with relative paths instead of the extension absolute path?

If the answers are no and no, then a big 👍 to the suggestion.

@bryceitoc9
Copy link
Contributor Author

Not adding additional tests to the samInitWizard as I don't want to add any tests that open external URLs.

@codecov-io
Copy link

codecov-io commented May 15, 2019

Codecov Report

Merging #538 into develop will decrease coverage by 0.01%.
The diff coverage is 48%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #538      +/-   ##
===========================================
- Coverage    54.23%   54.21%   -0.02%     
===========================================
  Files          109      110       +1     
  Lines         4276     4296      +20     
  Branches       647      651       +4     
===========================================
+ Hits          2319     2329      +10     
- Misses        1957     1967      +10
Impacted Files Coverage Δ
src/lambda/lambdaTreeDataProvider.ts 47.36% <0%> (ø) ⬆️
src/shared/ui/buttons.ts 100% <100%> (ø)
src/shared/constants.ts 100% <100%> (ø) ⬆️
src/lambda/wizards/samInitWizard.ts 35.22% <28.57%> (-1.62%) ⬇️
src/lambda/commands/createNewSamApp.ts 18.46% <33.33%> (-0.29%) ⬇️

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 61d1102...4278776. Read the comment docs.

…d is applied to the whole wizard with a more generic link
@awschristou awschristou self-requested a review May 16, 2019 15:39
Copy link
Contributor

@awschristou awschristou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking PR out of approved state to review latest commit
(Edit: Oops, this comment didn't prevent merge.)

Copy link
Contributor

@awschristou awschristou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking PR out of approved state to prevent merge until the latest commits are reviewed

src/lambda/wizards/samInitWizard.ts Outdated Show resolved Hide resolved
src/lambda/wizards/samInitWizard.ts Outdated Show resolved Hide resolved

public constructor(
private readonly context: CreateNewSamAppWizardContext = new DefaultCreateNewSamAppWizardContext()
private readonly vscodeContext: Pick<vscode.ExtensionContext, 'asAbsolutePath' | 'globalState'>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still part of the context for CreateNewSamAppWizard, so it shouldn't be a separate argument.

Option 1: Make the vscode context a property of the wizard context

export interface CreateNewSamAppWizardContext {
    ...
    vscodeContext: Pick<vscode.ExtensionContext, 'asAbsolutePath' | 'globalState'>
}

Option 2: Merge the vscode context into the wizard context

interface CreateNewSamAppWizardContext { /* unchanged */ }
type MergedContext = CreateNewSamAppWizardContext & Pick<vscode.ExtensionContext, 'asAbsolutePath' | 'globalState'>
export { MergedContext as CreateNewSamAppWizardContext }

Option 3: Make CreateNewSamAppWizardContext derive from the extension context.

export interface CreateNewSamAppWizardContext implements Pick<vscode.ExtensionContext, 'asAbsolutePath' | 'globalState'> { /* unchanged */ }

@bryceitoc9 bryceitoc9 merged commit 3477680 into develop May 16, 2019
@bryceitoc9 bryceitoc9 deleted the bryceito/add-help-buttons branch May 16, 2019 22:20
mpiroc added a commit that referenced this pull request May 29, 2019
mpiroc added a commit that referenced this pull request Jun 7, 2019
…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.
mpiroc added a commit that referenced this pull request Jun 7, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants