Skip to content

feat(amplify): add compute role support for Amplify branches #34708

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
Merged
merged 9 commits into from
Jun 18, 2025
Merged

feat(amplify): add compute role support for Amplify branches #34708

merged 9 commits into from
Jun 18, 2025

Conversation

@mazyu36
Copy link
Contributor

@mazyu36 mazyu36 commented Jun 13, 2025

Issue # (if applicable)

N/A

Reason for this change

Amplify supports branch-level compute role setting.
But current L2 Construct doesn't support it.

Description of changes

Add computeRole property for Branch construct.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Add a unit test and an integ test.

Checklist

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

@github-actions github-actions bot added the p2 label Jun 13, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team June 13, 2025 01:32
@github-actions github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Jun 13, 2025
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 13, 2025
@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jun 13, 2025
@leonmk-aws leonmk-aws self-assigned this Jun 13, 2025
leonmk-aws
leonmk-aws previously requested changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@leonmk-aws leonmk-aws left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, I have added some comments

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 16, 2025
@mergify mergify bot dismissed leonmk-aws’s stale review June 17, 2025 09:51

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 17, 2025
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 17, 2025

@leonmk-aws
Thank you for the review.
Addressed the comments.

@mazyu36 mazyu36 requested a review from leonmk-aws June 17, 2025 10:53
leonmk-aws
leonmk-aws previously requested changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@leonmk-aws leonmk-aws left a comment

Choose a reason for hiding this comment

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

Thank you for these changes, I added a few comments on the platform field.

packages/@aws-cdk/aws-amplify-alpha/lib/app.ts Outdated
/**
* The platform of the app
*/
readonly platform?: Platform;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not add optional field in the interface that extends iResource, see below one approach on how to do the isSSR check without exposing the field in this interface.

packages/@aws-cdk/aws-amplify-alpha/lib/app.ts Outdated
let computedRole: iam.IRole | undefined;
const isSSR = props.platform === Platform.WEB_COMPUTE || props.platform === Platform.WEB_DYNAMIC;
const appPlatform = props.platform || Platform.WEB;
const isSSR = appPlatform === Platform.WEB_COMPUTE || appPlatform === Platform.WEB_DYNAMIC;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider adding a function in the utils.ts file, to share it between app.ts and branch.ts

packages/@aws-cdk/aws-amplify-alpha/lib/branch.ts Outdated
// Enhanced CDK Analytics Telemetry
addConstructMetadata(this, props);

const platform = props.app.platform;
Copy link
Contributor

@leonmk-aws leonmk-aws Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of relying on the IApp interface to have an optional platform field, another approach would be to check if if (props.app instanceof App) is true and use the platform attribute of this class to perform the isSSR check.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 17, 2025
@mergify mergify bot dismissed leonmk-aws’s stale review June 18, 2025 08:18

Pull request has been modified.

@mazyu36 mazyu36 force-pushed the feature/branch-compute-role branch from c5d8a9f to 052bb10 Compare June 18, 2025 08:34
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 18, 2025
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 18, 2025

@leonmk-aws
Thanks.

I've fixed a validation based on the comments.
Please let me know if i misunderstood the comment intentions

@mazyu36 mazyu36 requested a review from leonmk-aws June 18, 2025 09:39
leonmk-aws
leonmk-aws approved these changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@leonmk-aws leonmk-aws left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, approved

@mergify
Copy link
Contributor

mergify bot commented Jun 18, 2025

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Jun 18, 2025

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

@mergify mergify bot merged commit 817a21a into aws:main Jun 18, 2025
17 checks passed
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@leonmk-aws leonmk-aws leonmk-aws approved these changes

+1 more reviewer

@go-to-k go-to-k go-to-k approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@leonmk-aws leonmk-aws

Labels

distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mazyu36 @aws-cdk-automation @go-to-k @leonmk-aws