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

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

Closed
wants to merge 15 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3fd6411
add feature property to Branch with new unit test
sumupitchayan Jan 19, 2023
7e2511f
added new property to integ app and updated snapshots
sumupitchayan Jan 24, 2023
8f4f4c3
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Jan 24, 2023
a358d8f
Added example usage to README
sumupitchayan Jan 24, 2023
e285ac0
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Jan 25, 2023
31a34df
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Jan 25, 2023
f56a87a
Updated snapshots test passing
sumupitchayan Jan 25, 2023
7d12a5c
Fix README example for codebuild
sumupitchayan Jan 25, 2023
d641f28
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Feb 1, 2023
fdfd551
Add platform variable to app construct with unit test
sumupitchayan Feb 1, 2023
d36ced6
Add platform to integ test and update snapshots
sumupitchayan Feb 1, 2023
02d703e
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Feb 1, 2023
d196c85
Add Platform property to README
sumupitchayan Feb 3, 2023
82cccc2
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Feb 3, 2023
307d514
Merge branch 'main' into sumughan/amplify-missing-cfn-property-#23325
sumupitchayan Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/@aws-cdk/aws-amplify/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,19 @@ const amplifyApp = new amplify.App(this, 'App', {
});
```

## Adding Platform and Framework properties to your Amplify App

Use the `framework` property on `BranchOptions` to pass in an optional value for the framework:

```ts
const amplifyApp = new amplify.App(this, 'App', {
platform: amplify.Platform.WEB,
});
amplifyApp.addBranch('feature/next', {
framework: 'ExampleFramework',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sumupitchayan sumupitchayan Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, the testing comments stand.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

});
```

## Deploying Assets

`sourceCodeProvider` is optional; when this is not specified the Amplify app can be deployed to using `.zip` packages. The `asset` property can be used to deploy S3 assets to Amplify as part of the CDK:
Expand Down
29 changes: 29 additions & 0 deletions packages/@aws-cdk/aws-amplify/lib/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,34 @@ export interface AppProps {
* @default - a new role is created
*/
readonly role?: iam.IRole;

/**
* The Platform type of the Amplify application
*
* @default - no platform
*/
readonly platform?: Platform
}

/**
* The platform type for an Amplify App.
*/
export enum Platform {
/**
* Static app (WEB)
*/
WEB = 'WEB',

/**
* Dynamic SSR app (WEB_COMPUTE)
*/
WEB_COMPUTE = 'WEB_COMPUTE',

/**
* App requiring Amplify hosting's original SSR support only
* (WEB_DYNAMIC)
*/
WEB_DYNAMIC = 'WEB_DYNAMIC',
}

/**
Expand Down Expand Up @@ -249,6 +277,7 @@ export class App extends Resource implements IApp, iam.IGrantable {
oauthToken: sourceCodeProviderOptions?.oauthToken?.unsafeUnwrap(), // Safe usage
repository: sourceCodeProviderOptions?.repository,
customHeaders: props.customResponseHeaders ? renderCustomResponseHeaders(props.customResponseHeaders) : undefined,
platform: props.platform,
});

this.appId = app.attrAppId;
Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/aws-amplify/lib/branch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ export interface BranchOptions {
* @default false
*/
readonly performanceMode?: boolean;

/**
* The Framework for the branch.
*
* @default - no framework
*/
readonly framework?: string;
}

/**
Expand Down Expand Up @@ -180,6 +187,7 @@ export class Branch extends Resource implements IBranch {
pullRequestEnvironmentName: props.pullRequestEnvironmentName,
stage: props.stage,
enablePerformanceMode: props.performanceMode,
framework: props.framework,
});

this.arn = branch.attrArn;
Expand Down
16 changes: 16 additions & 0 deletions packages/@aws-cdk/aws-amplify/test/app.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,3 +442,19 @@ test('with custom headers', () => {
},
});
});

test('create an amplify app with platform ', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

@kevinold kevinold Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

// WHEN
new amplify.App(stack, 'App', {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.unsafePlainText('secret'),
}),
platform: amplify.Platform.WEB,
});
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::App', {
Platform: 'WEB',
});
});
242 changes: 128 additions & 114 deletions packages/@aws-cdk/aws-amplify/test/branch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,141 +4,155 @@ import { Asset } from '@aws-cdk/aws-s3-assets';
import { SecretValue, Stack } from '@aws-cdk/core';
import * as amplify from '../lib';

let stack: Stack;
let app: amplify.App;
beforeEach(() => {
stack = new Stack();
app = new amplify.App(stack, 'App', {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.unsafePlainText('secret'),
}),
describe('amplify app', () => {
let stack: Stack;
let app: amplify.App;
beforeEach(() => {
stack = new Stack();
app = new amplify.App(stack, 'App', {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});
});
});

test('create a branch', () => {
// WHEN
app.addBranch('dev');
test('create a branch', () => {
// WHEN
app.addBranch('dev');

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
AppId: {
'Fn::GetAtt': [
'AppF1B96344',
'AppId',
],
},
BranchName: 'dev',
EnableAutoBuild: true,
EnablePullRequestPreview: true,
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
AppId: {
'Fn::GetAtt': [
'AppF1B96344',
'AppId',
],
},
BranchName: 'dev',
EnableAutoBuild: true,
EnablePullRequestPreview: true,
});
});
});

test('with basic auth from credentials', () => {
// WHEN
app.addBranch('dev', {
basicAuth: amplify.BasicAuth.fromCredentials('username', SecretValue.unsafePlainText('password')),
});
test('with basic auth from credentials', () => {
// WHEN
app.addBranch('dev', {
basicAuth: amplify.BasicAuth.fromCredentials('username', SecretValue.unsafePlainText('password')),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
BasicAuthConfig: {
EnableBasicAuth: true,
Password: 'password',
Username: 'username',
},
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
BasicAuthConfig: {
EnableBasicAuth: true,
Password: 'password',
Username: 'username',
},
});
});
});

test('with basic auth from generated password', () => {
// WHEN
app.addBranch('dev', {
basicAuth: amplify.BasicAuth.fromGeneratedPassword('username'),
});
test('with basic auth from generated password', () => {
// WHEN
app.addBranch('dev', {
basicAuth: amplify.BasicAuth.fromGeneratedPassword('username'),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
BasicAuthConfig: {
EnableBasicAuth: true,
Password: {
'Fn::Join': [
'',
[
'{{resolve:secretsmanager:',
{
Ref: 'AppdevdevBasicAuthB25D2314',
},
':SecretString:password::}}',
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
BasicAuthConfig: {
EnableBasicAuth: true,
Password: {
'Fn::Join': [
'',
[
'{{resolve:secretsmanager:',
{
Ref: 'AppdevdevBasicAuthB25D2314',
},
':SecretString:password::}}',
],
],
],
},
Username: 'username',
},
Username: 'username',
},
});
});
});

test('with env vars', () => {
// WHEN
const branch = app.addBranch('dev', {
environmentVariables: {
key1: 'value1',
},
test('with env vars', () => {
// WHEN
const branch = app.addBranch('dev', {
environmentVariables: {
key1: 'value1',
},
});
branch.addEnvironment('key2', 'value2');

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
EnvironmentVariables: [
{
Name: 'key1',
Value: 'value1',
},
{
Name: 'key2',
Value: 'value2',
},
],
});
});
branch.addEnvironment('key2', 'value2');

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
EnvironmentVariables: [
{
Name: 'key1',
Value: 'value1',
test('with asset deployment', () => {
// WHEN
const asset = new Asset(app, 'SampleAsset', {
path: path.join(__dirname, './test-asset'),
});
app.addBranch('dev', { asset });

// THEN
Template.fromStack(stack).hasResourceProperties('Custom::AmplifyAssetDeployment', {
ServiceToken: {
'Fn::GetAtt': [
'comamazonawscdkcustomresourcesamplifyassetdeploymentproviderNestedStackcomamazonawscdkcustomresourcesamplifyassetdeploymentproviderNestedStackResource89BDFEB2',
'Outputs.comamazonawscdkcustomresourcesamplifyassetdeploymentprovideramplifyassetdeploymenthandlerproviderframeworkonEventA449D9A9Arn',
],
},
AppId: {
'Fn::GetAtt': [
'AppF1B96344',
'AppId',
],
},
{
Name: 'key2',
Value: 'value2',
BranchName: 'dev',
S3ObjectKey: '8c89eadc6be22019c81ed6b9c7d9929ae10de55679fd8e0e9fd4c00f8edc1cda.zip',
S3BucketName: {
'Fn::Sub': 'cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}',
},
],
});
});
});

test('with asset deployment', () => {
// WHEN
const asset = new Asset(app, 'SampleAsset', {
path: path.join(__dirname, './test-asset'),
});
app.addBranch('dev', { asset });
test('with performance mode', () => {
// WHEN
app.addBranch('dev', {
performanceMode: true,
});

// THEN
Template.fromStack(stack).hasResourceProperties('Custom::AmplifyAssetDeployment', {
ServiceToken: {
'Fn::GetAtt': [
'comamazonawscdkcustomresourcesamplifyassetdeploymentproviderNestedStackcomamazonawscdkcustomresourcesamplifyassetdeploymentproviderNestedStackResource89BDFEB2',
'Outputs.comamazonawscdkcustomresourcesamplifyassetdeploymentprovideramplifyassetdeploymenthandlerproviderframeworkonEventA449D9A9Arn',
],
},
AppId: {
'Fn::GetAtt': [
'AppF1B96344',
'AppId',
],
},
BranchName: 'dev',
S3ObjectKey: '8c89eadc6be22019c81ed6b9c7d9929ae10de55679fd8e0e9fd4c00f8edc1cda.zip',
S3BucketName: {
'Fn::Sub': 'cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}',
},
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
EnablePerformanceMode: true,
});
});
});

test('with performance mode', () => {
// WHEN
app.addBranch('dev', {
performanceMode: true,
});
test('with framework', () => {
// WHEN
app.addBranch('dev', {
framework: 'testFramework',
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
EnablePerformanceMode: true,
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Amplify::Branch', {
Framework: 'testFramework',
});
});
});
});
2 changes: 1 addition & 1 deletion .../test/integ.app-asset-deployment.js.snapshot/cdk-amplify-app-asset-deployment.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "21.0.0",
"version": "29.0.0",
"files": {
"673eedce19cf9e5cb7018f3029adb9937d4c7c7b167af1f80c69613cac83b7da": {
"source": {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-amplify/test/integ.app-asset-deployment.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"21.0.0"}
{"version":"29.0.0"}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-amplify/test/integ.app-asset-deployment.js.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "21.0.0",
"version": "29.0.0",
"testCases": {
"integ.app-asset-deployment": {
"stacks": [
Expand Down
Loading