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

Merge pirocchi/dotnet/merge-develop into develop #594

Merged
merged 44 commits into from
Jun 7, 2019

Conversation

mpiroc
Copy link
Contributor

@mpiroc mpiroc commented Jun 5, 2019

Description

Merge the feature branch pirocchi/dotnet/merge-develop back into develop.

Motivation and Context

The feature branch is complete.

Testing

  1. Created a temporary branch off of develop, and merge merge-develop into that branch.
  2. Run all tests from temporary branch.
  3. Performed sanity-level manual testing on Windows from temporary branch.

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

Note: I will add a CHANGELOG entry and update this PR.

License

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

awschristou and others added 30 commits April 10, 2019 09:46
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
mpiroc and others added 13 commits May 30, 2019 15:34
…itialized before initial credential validation (#577)' from develop.
* 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
@mpiroc mpiroc self-assigned this Jun 5, 2019
@codecov-io
Copy link

codecov-io commented Jun 5, 2019

Codecov Report

Merging #594 into develop will decrease coverage by 0.91%.
The diff coverage is 42.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #594      +/-   ##
==========================================
- Coverage    55.82%   54.9%   -0.92%     
==========================================
  Files          115     117       +2     
  Lines         4498    4655     +157     
  Branches       681     692      +11     
==========================================
+ Hits          2511    2556      +45     
- Misses        1987    2099     +112
Impacted Files Coverage Δ
src/shared/templates/sam/samTemplateGenerator.ts 96.77% <ø> (ø) ⬆️
src/shared/telemetry/telemetryEvent.ts 100% <ø> (ø) ⬆️
src/shared/sam/cli/samCliInfo.ts 100% <ø> (ø) ⬆️
src/lambda/wizards/samInitWizard.ts 35.22% <ø> (ø) ⬆️
src/shared/credentials/userCredentialsUtils.ts 82.43% <ø> (ø) ⬆️
src/shared/defaultAwsContextCommands.ts 0% <0%> (ø) ⬆️
src/shared/codelens/typescriptCodeLensProvider.ts 0% <0%> (ø) ⬆️
src/shared/telemetry/defaultTelemetryClient.ts 50% <0%> (+4.16%) ⬆️
src/extension.ts 0% <0%> (ø) ⬆️
src/shared/sam/cli/samCliBuild.ts 89.47% <100%> (+0.58%) ⬆️
... and 23 more

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 85286ee...c51fb0f. Read the comment docs.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
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.

Only two things seem out of place to me

@@ -271,7 +259,11 @@ function enhanceAwsCloudFormationInstructions(
// and append region to assist in troubleshooting the error
// (command uses CLI configured value--users that don't know this and omit region won't see error)
// tslint:disable-next-line:max-line-length
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary?

'java' |
'ruby' |
'ruby2.5'
'dotnetcore2.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed? You can't deploy new dotnetcore2.0 apps.

'java',
'ruby',
'ruby2.5'
'dotnetcore2.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto above

@@ -20,7 +20,7 @@ import { defaultMetricDatum } from '../telemetry/telemetryUtils'
import { toArrayAsync } from '../utilities/collectionUtils'
import { localize } from '../utilities/vsCodeUtils'

export type Language = 'python' | 'javascript'
export type Language = 'python' | 'javascript' | 'csharp'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should we rename typescriptCodeLensProvider to javascriptCodeLensProvider?

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for the feature branch merge

@mpiroc mpiroc force-pushed the pirocchi/dotnet/merge-develop branch from c51fb0f to ec8b88b Compare June 7, 2019 19:27
@mpiroc
Copy link
Contributor Author

mpiroc commented 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, squash the ports before merging develop.

@mpiroc mpiroc merged commit 4e43030 into develop Jun 7, 2019
@mpiroc mpiroc deleted the pirocchi/dotnet/merge-develop branch June 7, 2019 20:07
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.

5 participants