feat(amplify): Add missing `Framework` and `Platform` Cfn properties to `Amplify` #23818

sumupitchayan · 2023-01-24T21:12:08Z

This PR closes #23325 which requests adding the missing Framework CloudFormation property to BranchOptions in Amplify.
fixes #24076

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2023-01-24T21:12:18Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

TheRealAmazonKendra

Thank you for your contribution! I have one question inline.

TheRealAmazonKendra · 2023-01-25T03:32:46Z

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

+
+```ts
+const amplifyApp = new amplify.App(this, 'MyApp', {
+  framework: 'ExampleFramework',


Is any value here valid or are there specific frameworks that work?

Thanks for your review Kendra! This is a question I've also been struggling to find the answer to -- the CloudFormation documentation has no information on what the framework property actually is and whether or not there are specific values that work (same with the Amplify Docs).

Did the integ test successfully deploy with framework: TestFramework? If so, I think we can say any value is valid.

@kaizencc Yes it did. Since I was able to successfully deploy with test: TestFramework it seems that this property can take in an arbitrary string, so there is not a list of specific frameworks that are valid; if this was the case, we would implement the property as an enum instead of a string @TheRealAmazonKendra

Deploying with an arbitrary value doesn't always mean the service will work correctly once deployed. Give me a day or two to check with the service team on this.

@kaizencc @TheRealAmazonKendra After speaking with Amplify, we learned that the framework property is indeed meant to be an open text field where the user can enter any value (Amplify just ignores it if it does not recognize the framework).

However, we will also have to add a platform property in the amplify.App construct to solve the original problem from this issue - see the updated comments on the Github issue here for full context.

So, I've been doing a deep dive on this module because it's on our list to stabilize and it seems that the frameworks can't actually be any value. The template might deploy, but the branch won't deploy within the app. We should discover what the combinations of platforms and frameworks are valid and update the contract accordingly. This may require a much larger set of changes that you intended to take on here, though.

Feel free to hit me up and chat about this. If it does require sweeping and/or breaking changes, I might just accept this change for the moment so that we unblock customers on this, even if I will end up altering the contract.

Still, the testing comments stand.

Hi, PM here from the Amplify team. I can confirm that the framework property in that it is an open text field for any value. The detection around how to deploy the application happens based on the platform property. WEB is for SPAs or HTML based applications and WEB_COMPUTE is for Next.js SSR only, at the moment.

Personally, I would approve this framework example test.

It may be an open text but if we know that specific values work and only work in certain cases, we can make the contract reflect that so there's less room for error.

aws-cdk-automation · 2023-02-08T17:29:44Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 307d514
Result: SUCCEEDED
Build Logs (available for 30 days)

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

TheRealAmazonKendra · 2023-02-09T08:18:09Z

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

    });

    amplifyApp.addCustomRule({
      source: '/source',
      target: '/target',
    });

-    const mainBranch = amplifyApp.addBranch('main');
+    const mainBranch = amplifyApp.addBranch('main', {


This should be its own test, not just an update to a current test. Also, we should be using the new IntegTest construct for all new or edited test cases. I'd like to see an assertion added to the new test to ensure that it actually successfully deploys as well.

Agreed, this needs to be a separate IntegTest.

TheRealAmazonKendra · 2023-02-09T08:18:45Z

packages/@aws-cdk/aws-amplify/test/app.test.ts

@@ -442,3 +442,19 @@ test('with custom headers', () => {
    },
  });
 });
+
+test('create an amplify app with platform ', () => {


This single test doesn't cover all the updates. Please increase the test coverage on your changes.

@TheRealAmazonKendra Hi, Amplify PM here, I think this test might cover the change made to the platform updates applied at the Amplify App level. The framework property tests are done in the branch.test.ts.

This is the minimum test I had in my PR for the same functionality for this construct.

TheRealAmazonKendra · 2023-02-09T08:22:28Z

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

+
+```ts
+const amplifyApp = new amplify.App(this, 'MyApp', {
+  framework: 'ExampleFramework',


So, I've been doing a deep dive on this module because it's on our list to stabilize and it seems that the frameworks can't actually be any value. The template might deploy, but the branch won't deploy within the app. We should discover what the combinations of platforms and frameworks are valid and update the contract accordingly. This may require a much larger set of changes that you intended to take on here, though.

Feel free to hit me up and chat about this. If it does require sweeping and/or breaking changes, I might just accept this change for the moment so that we unblock customers on this, even if I will end up altering the contract.

TheRealAmazonKendra · 2023-02-09T08:22:39Z

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

+
+```ts
+const amplifyApp = new amplify.App(this, 'MyApp', {
+  framework: 'ExampleFramework',


Still, the testing comments stand.

kevinold · 2023-02-10T22:12:49Z

👋 Hi @TheRealAmazonKendra @sumupitchayan @kaizencc Amplify PM here and author of a similar PR (#24078) until this more thorough PR was brought to my attention.

I have made a few clarifications in PR comments but wanted to centralize here.

I can confirm:

The framework property on the AWS::Amplify::Branch CF resource is a freeform text field for developers to populate as they wish. Amplify Hosting would populate a create-react-app based SPA with "React" in this field, but it is not used to drive deployment.
The platform property on the AWS::Amplify::App CF resource is one of WEB, WEB_COMPUTE or WEB_DYNAMIC and helps drive the correct deployment mechanism in Amplify Hosting.

@TheRealAmazonKendra Per your comment #23818 (comment), I can confirm that there are not any combinations of platforms and frameworks to validate.

As further validation of this PR and anecdotal evidence of the value of this PR from my perspective, and that of @michrome, the Amplify Hosting PM on a project to deploy Amplify apps using multiple platforms via the CDK, this PR accurately creates the CloudFormation necessary to deploy the apps in the correct manner. That said, I do agree there should be an integration test added to further validate, then this PR would be ready to merge.

TheRealAmazonKendra · 2023-02-13T05:33:55Z

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

+
+```ts
+const amplifyApp = new amplify.App(this, 'MyApp', {
+  framework: 'ExampleFramework',


It may be an open text but if we know that specific values work and only work in certain cases, we can make the contract reflect that so there's less room for error.

TheRealAmazonKendra · 2023-02-15T18:26:26Z

👋 Hi @TheRealAmazonKendra @sumupitchayan @kaizencc Amplify PM here and author of a similar PR (#24078) until this more thorough PR was brought to my attention.

I have made a few clarifications in PR comments but wanted to centralize here.

I can confirm:

The framework property on the AWS::Amplify::Branch CF resource is a freeform text field for developers to populate as they wish. Amplify Hosting would populate a create-react-app based SPA with "React" in this field, but it is not used to drive deployment.

The platform property on the AWS::Amplify::App CF resource is one of WEB, WEB_COMPUTE or WEB_DYNAMIC and helps drive the correct deployment mechanism in Amplify Hosting.

@TheRealAmazonKendra Per your comment #23818 (comment), I can confirm that there are not any combinations of platforms and frameworks to validate.

As further validation of this PR and anecdotal evidence of the value of this PR from my perspective, and that of @michrome, the Amplify Hosting PM on a project to deploy Amplify apps using multiple platforms via the CDK, this PR accurately creates the CloudFormation necessary to deploy the apps in the correct manner. That said, I do agree there should be an integration test added to further validate, then this PR would be ready to merge.

We often write a tighter contract than the CloudFormation resources to improve the user experience. In playing around with this construct and with the Cfn resources, I found that, yes, you can put anything in framework and it reports a success, but it actually fails silently. Silent failures are not a thing we want. If we know that certain combinations are valid, we should do our best to provide that experience instead of just freeform.

tqhoughton · 2023-03-12T02:32:38Z

Any action on this? Notice it's been 3 weeks with no commits or comments and this is a feature I'd love to see added to the CDK to be able to use the "web compute" platform more easily

TheRealAmazonKendra · 2023-05-15T21:03:55Z

Closing due to in-progress redesign.

sumupitchayan added 2 commits January 19, 2023 09:48

add feature property to Branch with new unit test

3fd6411

added new property to integ app and updated snapshots

7e2511f

aws-cdk-automation requested a review from a team January 24, 2023 21:12

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jan 24, 2023

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

8f4f4c3

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 24, 2023

aws-cdk-automation previously requested changes Jan 24, 2023

View reviewed changes

sumupitchayan changed the title ~~sumughan/amplify missing cfn property #23325~~ feat(amplify): Add missing cfn property Jan 24, 2023

Added example usage to README

a358d8f

TheRealAmazonKendra requested changes Jan 25, 2023

View reviewed changes

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

e285ac0

sumupitchayan changed the title ~~feat(amplify): Add missing cfn property~~ feat(amplify): Add missing Framework property to BranchOptions Construct Jan 25, 2023

sumupitchayan changed the title ~~feat(amplify): Add missing Framework property to BranchOptions Construct~~ feat(amplify): Add missing Framework Cfn property to BranchOptions Jan 25, 2023

sumupitchayan and others added 9 commits January 25, 2023 11:41

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

31a34df

Updated snapshots test passing

f56a87a

Fix README example for codebuild

7d12a5c

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

d641f28

Add platform variable to app construct with unit test

fdfd551

Add platform to integ test and update snapshots

d36ced6

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

02d703e

Add Platform property to README

d196c85

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

82cccc2

sumupitchayan changed the title ~~feat(amplify): Add missing Framework Cfn property to BranchOptions~~ feat(amplify): Add missing Framework and Platform Cfn properties to Amplify Feb 6, 2023

Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325

307d514

TheRealAmazonKendra mentioned this pull request Feb 9, 2023

feat(amplify): Add platform property to Amplify App #24078

Closed

TheRealAmazonKendra requested changes Feb 9, 2023

View reviewed changes

kevinold mentioned this pull request Feb 9, 2023

(aws-cdk/aws-amplify): (Add Platform property to amplify.App) #24076

Closed

2 tasks

TheRealAmazonKendra requested changes Feb 15, 2023

View reviewed changes

TheRealAmazonKendra self-assigned this Apr 12, 2023

TheRealAmazonKendra closed this May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(amplify): Add missing `Framework` and `Platform` Cfn properties to `Amplify` #23818

feat(amplify): Add missing `Framework` and `Platform` Cfn properties to `Amplify` #23818

sumupitchayan commented Jan 24, 2023 •

edited by peterwoodworth

Loading

gitpod-io bot commented Jan 24, 2023

aws-cdk-automation left a comment

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jan 25, 2023

sumupitchayan Jan 25, 2023

kaizencc Jan 25, 2023

sumupitchayan Jan 25, 2023

TheRealAmazonKendra Jan 26, 2023

sumupitchayan Jan 30, 2023 •

edited

Loading

TheRealAmazonKendra Feb 9, 2023

TheRealAmazonKendra Feb 9, 2023

kevinold Feb 10, 2023

TheRealAmazonKendra Feb 13, 2023

aws-cdk-automation commented Feb 8, 2023

TheRealAmazonKendra Feb 9, 2023

kevinold Feb 10, 2023

TheRealAmazonKendra Feb 9, 2023

kevinold Feb 10, 2023 •

edited

Loading

TheRealAmazonKendra Feb 9, 2023

TheRealAmazonKendra Feb 9, 2023

kevinold commented Feb 10, 2023 •

edited

Loading

TheRealAmazonKendra Feb 13, 2023

TheRealAmazonKendra commented Feb 15, 2023

tqhoughton commented Mar 12, 2023 •

edited

Loading

TheRealAmazonKendra commented May 15, 2023

feat(amplify): Add missing Framework and Platform Cfn properties to Amplify #23818

feat(amplify): Add missing Framework and Platform Cfn properties to Amplify #23818

Conversation

sumupitchayan commented Jan 24, 2023 • edited by peterwoodworth Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 24, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a 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

sumupitchayan Jan 30, 2023 • edited Loading

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

aws-cdk-automation commented Feb 8, 2023

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinold Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinold commented Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra commented Feb 15, 2023

tqhoughton commented Mar 12, 2023 • edited Loading

TheRealAmazonKendra commented May 15, 2023

feat(amplify): Add missing `Framework` and `Platform` Cfn properties to `Amplify` #23818

feat(amplify): Add missing `Framework` and `Platform` Cfn properties to `Amplify` #23818

sumupitchayan commented Jan 24, 2023 •

edited by peterwoodworth

Loading

sumupitchayan Jan 30, 2023 •

edited

Loading

kevinold Feb 10, 2023 •

edited

Loading

kevinold commented Feb 10, 2023 •

edited

Loading

tqhoughton commented Mar 12, 2023 •

edited

Loading