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.

Sign up for GitHub

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

Jump to bottom

feat: migrate CLI to CDK v2 #10988

Merged
merged 16 commits into from
Oct 27, 2022
Merged

feat: migrate CLI to CDK v2 #10988

merged 16 commits into from
Oct 27, 2022

Conversation

akshbhu
Copy link
Contributor

@akshbhu akshbhu commented Sep 8, 2022 •
edited
Loading

Description of changes

  • Categories converted : auth , storage, geo , awscloudformation , cli-internal , cli-ext helper

Issue #, if available

Description of how you validated changes

https://app.circleci.com/pipelines/github/aws-amplify/amplify-cli/10838/workflows/c59422ac-0e8e-43f4-b695-056b55efd422

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

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

@akshbhu akshbhu requested a review from a team as a code owner September 8, 2022 17:30
@akshbhu akshbhu marked this pull request as draft September 8, 2022 17:31
@akshbhu akshbhu changed the title feat: converts auth and storage categories to cdk V2 feat: aws-cdk v2 Conversion Sep 8, 2022
sobolk
sobolk reviewed Sep 9, 2022
View reviewed changes
Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

Could we please check:

  • how are e2e tests are with the change.
  • check what happens with existing project when customer rolls forward to cli with v2 cdk.
  • spike v2 v1 side by side support.

...y-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2022 •
edited
Loading

Codecov Report

Merging #10988 (92dc6b8) into cdkv2 (592fd90) will decrease coverage by 0.00%.
The diff coverage is 94.39%.

@@            Coverage Diff             @@
##            cdkv2   #10988      +/-   ##
==========================================
- Coverage   49.57%   49.57%   -0.01%     
==========================================
  Files         682      682              
  Lines       32848    32837      -11     
  Branches     6705     6705              
==========================================
- Hits        16285    16278       -7     
+ Misses      15083    15079       -4     
  Partials     1480     1480
Impacted Files Coverage Δ
...category-custom/src/utils/generate-cfn-from-cdk.ts 44.44% <ø> (ø)
...amplify-cli-core/src/feature-flags/featureFlags.ts 82.77% <ø> (ø)
...ation/src/root-stack-builder/root-stack-builder.ts 61.70% <55.55%> (-1.57%) ⬇️
...fy-provider-awscloudformation/src/network/stack.ts 14.08% <87.50%> (-0.99%) ⬇️
...dformation/src/utils/consolidate-apigw-policies.ts 69.29% <94.44%> (+0.92%) ⬆️
...n/auth-stack-builder/auth-cognito-stack-builder.ts 71.26% <100.00%> (ø)
...rmation/auth-stack-builder/auth-stack-transform.ts 87.35% <100.00%> (+0.07%) ⬆️
...tack-builder/auth-user-pool-group-stack-builder.ts 56.70% <100.00%> (ø)
...dformation/auth-stack-builder/stack-synthesizer.ts 82.14% <100.00%> (+0.66%) ⬆️
...h-stack-builder/user-pool-group-stack-transform.ts 85.88% <100.00%> (ø)
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@akshbhu akshbhu changed the title feat: aws-cdk v2 Conversion feat: aws-cdk v2 Conversion [Auth and storage Category] Sep 16, 2022
@akshbhu akshbhu added the do-not-merge PRs that are approved and green, but should not be merged yet label Sep 16, 2022
@akshbhu akshbhu marked this pull request as ready for review September 16, 2022 16:43
sobolk
sobolk reviewed Oct 3, 2022
View reviewed changes
Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

We should do a major version bump of affected categories.

...sts__/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.test.ts Show resolved Hide resolved
...sts__/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.test.ts Outdated Show resolved Hide resolved
...y-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.ts Show resolved Hide resolved
akshbhu
akshbhu commented Oct 25, 2022
View reviewed changes
packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts
@@ -11,6 +12,7 @@ import {
pathManager,
stateManager,
} from 'amplify-cli-core';
import { Construct } from 'constructs';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

relevant changes for cdk conversion

@akshbhu akshbhu changed the base branch from dev to cdkv2 October 25, 2022 21:06
@akshbhu akshbhu changed the title feat: aws-cdk v2 Conversion [Auth and storage Category] feat: aws-cdk v2 Conversion Oct 25, 2022
sobolk
sobolk reviewed Oct 25, 2022
View reviewed changes
Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

Looking good overall. Please see comments.

...rc/provider-utils/awscloudformation/auth-stack-builder/auth-user-pool-group-stack-builder.ts Outdated Show resolved Hide resolved
packages/amplify-category-custom/src/utils/dependency-management-utils.ts Outdated Show resolved Hide resolved
packages/amplify-category-geo/src/__tests__/service-stacks/__snapshots__/mapStack.test.ts.snap Show resolved Hide resolved
...amplify-category-geo/src/__tests__/service-stacks/__snapshots__/placeIndexStack.test.ts.snap Show resolved Hide resolved
packages/amplify-category-geo/src/service-stacks/geofenceCollectionStack.ts Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Oct 25, 2022

This pull request introduces 1 alert when merging 529b39e into 24a811d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@akshbhu akshbhu changed the title feat: aws-cdk v2 Conversion feat: migrate CLI to CDK v2 Oct 26, 2022
sobolk
sobolk previously approved these changes Oct 26, 2022
View reviewed changes
sobolk
sobolk previously approved these changes Oct 26, 2022
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Oct 26, 2022

This pull request introduces 1 alert when merging 46ecbd0 into 592fd90 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

aws-eddy
aws-eddy reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@aws-eddy aws-eddy left a comment

Choose a reason for hiding this comment

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

small question, otherwise lgtm

jhockett
jhockett reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@jhockett jhockett left a comment

Choose a reason for hiding this comment

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

A few nits and a question, but overall LGTM!

nx.json Outdated Show resolved Hide resolved
packages/amplify-category-auth/package.json Outdated Show resolved Hide resolved
packages/amplify-category-geo/package.json Outdated Show resolved Hide resolved
packages/amplify-category-storage/package.json Outdated Show resolved Hide resolved
packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2022

This pull request introduces 1 alert when merging 7fdddcb into 592fd90 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2022

This pull request introduces 1 alert when merging 92dc6b8 into 592fd90 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

jhockett
jhockett previously approved these changes Oct 27, 2022
View reviewed changes
sobolk
sobolk previously approved these changes Oct 27, 2022
View reviewed changes
packages/amplify-cli-core/src/feature-flags/featureFlags.ts Outdated Show resolved Hide resolved
@akshbhu akshbhu dismissed stale reviews from sobolk and jhockett via f2a1ca6 October 27, 2022 19:12
@akshbhu akshbhu merged commit 7fd3bc1 into aws-amplify:cdkv2 Oct 27, 2022
This was referenced Nov 1, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aws-eddy aws-eddy aws-eddy approved these changes

@sobolk sobolk sobolk approved these changes

@jhockett jhockett jhockett approved these changes

Assignees
No one assigned
Labels
do-not-merge PRs that are approved and green, but should not be merged yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@akshbhu @codecov-commenter @jhockett @sobolk @aws-eddy @phani-srikar