feat(amplify): Add Amplify asset deployment resource by samkio · Pull Request #16922 · aws/aws-cdk

samkio · 2021-10-12T14:50:51Z

This change adds a custom resource that allows users
to publish S3 assets to AWS Amplify.

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

gitpod-io · 2021-10-12T14:50:53Z

samkio · 2021-10-12T17:52:31Z

Build currently fails with:

@aws-cdk/aws-amplify-alpha: �[96mlib/asset-deployment-handler/common.ts
�[0m:�[93m4�[0m:�[93m8�[0m - �[91merror�[0m�[90m TS2307: �[0mCannot find module 'aws-cdk-lib/custom-resources/lib/provider-framework/types' or its corresponding type declarations.
@aws-cdk/aws-amplify-alpha: �[7m4�[0m } from 'aws-cdk-lib/custom-resources/lib/provider-framework/types';
@aws-cdk/aws-amplify-alpha: �[7m �[0m �[91m       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~�[0m
@aws-cdk/aws-amplify-alpha: �[96mlib/asset-deployment-handler/handler.ts�
[0m:�[93m4�[0m:�[93m8�[0m - �[91merror�[0m�[90m TS2307: �[0mCannot find module 'aws-cdk-lib/custom-resources/lib/provider-framework/types' or its corresponding type declarations.
@aws-cdk/aws-amplify-alpha: �[7m4�[0m } from 'aws-cdk-lib/custom-resources/lib/provider-framework/types';
@aws-cdk/aws-amplify-alpha: �[7m �[0m �[91m       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~�[0m

Which I added as a dependency. It worked locally too... 😕
I followed the same structure as aws-eks (https://github.com/samkio/aws-cdk/blob/42d43db12bc2ee7cf254424f41571348f8cd6222/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/common.ts#L2) which has the same import.

skinny85 · 2021-10-22T19:18:49Z

@samkio would you mind updating your PR with the latest master? I want to see if the build succeeds or fails.

Thanks,
Adam

skinny85

Putting in "Request changes" to clear this one from my To-Do list. @samkio please re-request my review when this is ready!

Pull request has been modified.

samkio · 2021-10-24T16:34:15Z

@skinny85 can you share the docs on what AWS CodeBuild us-east-1 (AutoBuildProject89A8053A-LhjRyN9kxr8o) — Build failed for project AutoBuildProject89A8053A-LhjRyN9kxr8o does? I am starting to lose hope in this change. Everything works fine locally but fails in this CodeBuild step. Now it complains about the hashes being different.

Running locally however:

Verifying integ.app-asset-deployment.js against integ.app-asset-deployment.expected.json ... OK.
Verifying integ.app-codecommit.js against integ.app-codecommit.expected.json ... OK.
Verifying integ.app.js against integ.app.expected.json ... OK.
Tests successful. Total time (7.3s) | /home/samkio/projects/aws-cdk/node_modules/jest/bin/jest.js (5.0s) | cdk-integ-assert (2.2s)

Integrations are taking longer as I can't try locally; I have to push and see CodeBuild fail. Any help will be appreciated.

skinny85

@skinny85 can you share the docs on what AWS CodeBuild us-east-1 (AutoBuildProject89A8053A-LhjRyN9kxr8o) — Build failed for project AutoBuildProject89A8053A-LhjRyN9kxr8o does? I am starting to lose hope in this change. Everything works fine locally but fails in this CodeBuild step. Now it complains about the hashes being different.

Running locally however:
Verifying integ.app-asset-deployment.js against integ.app-asset-deployment.expected.json ... OK.
Verifying integ.app-codecommit.js against integ.app-codecommit.expected.json ... OK.
Verifying integ.app.js against integ.app.expected.json ... OK.
Tests successful. Total time (7.3s) | /home/samkio/projects/aws-cdk/node_modules/jest/bin/jest.js (5.0s) | cdk-integ-assert (2.2s)
Integrations are taking longer as I can't try locally; I have to push and see CodeBuild fail. Any help will be appreciated.

Hey @samkio! Don't give up, you'r extremely close 🙂. They're a simple change you can make to your integ test that should make the build pass - I've included it in my review.

packages/@aws-cdk/aws-amplify/test/integ.app-asset-deployment.ts

Pull request has been modified.

This change adds a custom resource that allows users to publish S3 assets to AWS Amplify. fixes aws#16208

MrArnoldPalmer · 2021-12-13T18:03:48Z

@samkio I've been playing with this locally and have gotten it to work against a v1 app. Since Amplify is marked as stable and included in aws-cdk-lib, I think we should actually merge this as a new experimental module so users know there may be changes to the API and/or behavior of the custom resource if needed.

To do this we will have to create a new package in packages/@aws-cdk that has stability experimental. This will automatically release as a separate package with -alpha appended to the name. I'm asking around for guidance on the best way to do this, IE what to name this module etc. As far as I can tell, it would be fairly easy to decouple this from the existing App construct and instead do something like new AmplifyAssetsDeploy(this, 'scope', myAmplifyApp) to create the custom resource.

What do you think about that?

samkio · 2021-12-13T18:22:34Z

@samkio I've been playing with this locally and have gotten it to work against a v1 app. Since Amplify is marked as stable and included in aws-cdk-lib, I think we should actually merge this as a new experimental module so users know there may be changes to the API and/or behavior of the custom resource if needed.

To do this we will have to create a new package in packages/@aws-cdk that has stability experimental. This will automatically release as a separate package with -alpha appended to the name. I'm asking around for guidance on the best way to do this, IE what to name this module etc. As far as I can tell, it would be fairly easy to decouple this from the existing App construct and instead do something like new AmplifyAssetsDeploy(this, 'scope', myAmplifyApp) to create the custom resource.

What do you think about that?

Happy for it to be marked experimental. The change was originally decoupled as AmplifyAssetDeployment; from the suggestions above I made it part of the API however.

MrArnoldPalmer · 2021-12-13T18:38:50Z

@samkio ahh yes I see. I think we should revert back to that structure and extract this into a separate module, perhaps @aws-cdk/amplify-assets, as that kinda follows some other naming we have, with stability: experimental. Once stabilized I do think adding as a method on the existing Branch class is ideal. @jogold cool with that?

I'm able to help with this if you run into issues or are just working on other stuff.

jogold · 2021-12-13T19:50:58Z

But @aws-cdk/aws-amplify is still experimental...

aws-cdk/packages/@aws-cdk/aws-amplify/package.json

Lines 110 to 111 in 5497525

    
           "stability": "experimental", 
        
           "maturity": "experimental",

and already published separately https://www.npmjs.com/package/@aws-cdk/aws-amplify-alpha

jogold · 2021-12-13T20:00:10Z

packages/@aws-cdk/aws-amplify/README.md

+const asset = new assets.Asset(this, "SampleAsset", {});
+const amplifyApp = new amplify.App(this, 'MyApp', {});
+const branch = amplifyApp.addBranch("dev");
+branch.addAssetDeployment(asset);


I know I suggested this addAssetDeployment() but thinking more about this, can we add more than one asset deployment to a branch? I don't think so. So maybe it should be moved to a construction prop in Branch/option in addBranch()?

Sounds good; amended.

You need to update the README for the latest change.

MrArnoldPalmer · 2021-12-13T20:54:47Z

oh wow, completely disregard what I said, I was looking at the wrong package apparently? I'm not exactly sure where I got this idea from.

packages/@aws-cdk/aws-amplify/lib/branch.ts

Discussing NodeJSFunction usage

mergify · 2021-12-14T17:41:11Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-12-14T18:23:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: ebb20bd
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-12-14T18:24:09Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This change adds a custom resource that allows users to publish S3 assets to AWS Amplify. fixes aws#16208 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

samkio force-pushed the master branch from 1c46472 to 49115cb Compare October 12, 2021 16:27

samkio marked this pull request as ready for review October 12, 2021 16:30

samkio mentioned this pull request Oct 12, 2021

(aws-cdk/aws-amplify): Add a sourceCodeProvider for S3 or Zipped Asset #16208

Closed

2 tasks

samkio force-pushed the master branch 3 times, most recently from 03a7ebe to 622fa23 Compare October 13, 2021 15:32

MrArnoldPalmer requested review from MrArnoldPalmer and skinny85 October 13, 2021 16:59

MrArnoldPalmer added effort/small Small work item – less than a day of effort p1 labels Oct 13, 2021

peterwoodworth changed the title ~~feat(amplify): Add Amplify asset deployment resource~~ feat(amplify): Add Amplify asset deployment resource Oct 21, 2021

github-actions bot assigned skinny85 Oct 21, 2021

github-actions bot added the @aws-cdk/aws-amplify Related to AWS Amplify label Oct 21, 2021

skinny85 previously requested changes Oct 22, 2021

View reviewed changes

samkio force-pushed the master branch from 622fa23 to f7f3cd8 Compare October 23, 2021 18:51

samkio force-pushed the master branch from f7f3cd8 to 318a191 Compare October 24, 2021 15:10

samkio requested a review from skinny85 October 24, 2021 16:34

skinny85 previously requested changes Oct 25, 2021

View reviewed changes

packages/@aws-cdk/aws-amplify/test/integ.app-asset-deployment.ts Show resolved Hide resolved

samkio force-pushed the master branch from 318a191 to 7e0a319 Compare October 25, 2021 16:37

skinny85 assigned MrArnoldPalmer and unassigned skinny85 Oct 25, 2021

samkio force-pushed the master branch 3 times, most recently from 60c57b3 to 56034fe Compare October 26, 2021 09:51

feat(amplify): Add Amplify asset deployment resource

bd97d74

This change adds a custom resource that allows users to publish S3 assets to AWS Amplify. fixes aws#16208

samkio force-pushed the master branch from f76016e to bd97d74 Compare November 18, 2021 11:07

samkio requested a review from skinny85 November 18, 2021 12:04

MrArnoldPalmer and others added 3 commits November 23, 2021 13:04

Merge branch 'master' into master

a0769bb

Merge branch 'master' into master

9b4d672

Fix package.json typo

92eee7d

jogold reviewed Dec 13, 2021

View reviewed changes

samkio added 5 commits December 14, 2021 09:55

Merge remote-tracking branch 'upstream/master'

d75d551

Make deployment part of branch constructor

333cfbf

Fix integ test

c59b290

Fix unit test

dd76559

Merge branch 'master' into master

ddfd773

MrArnoldPalmer previously approved these changes Dec 14, 2021

View reviewed changes

jogold reviewed Dec 14, 2021

View reviewed changes

packages/@aws-cdk/aws-amplify/lib/branch.ts Show resolved Hide resolved

MrArnoldPalmer self-requested a review December 14, 2021 16:29

samkio added 2 commits December 14, 2021 16:52

Update README

f8cae0a

Merge branch 'master' of github.com:samkio/aws-cdk

11ce555

MrArnoldPalmer approved these changes Dec 14, 2021

View reviewed changes

Merge branch 'master' into master

ebb20bd

mergify bot merged commit 499ba85 into aws:master Dec 14, 2021

Conversation

samkio commented Oct 12, 2021

Uh oh!

gitpod-io bot commented Oct 12, 2021

Uh oh!

samkio commented Oct 12, 2021

Uh oh!

skinny85 commented Oct 22, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

samkio commented Oct 24, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MrArnoldPalmer commented Dec 13, 2021

Uh oh!

samkio commented Dec 13, 2021

Uh oh!

MrArnoldPalmer commented Dec 13, 2021

Uh oh!

jogold commented Dec 13, 2021

Uh oh!

jogold Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

samkio Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jogold Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

samkio Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

MrArnoldPalmer commented Dec 13, 2021

Uh oh!

Uh oh!

mergify bot commented Dec 14, 2021

Uh oh!

aws-cdk-automation commented Dec 14, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

samkio Dec 14, 2021 •

edited

Loading