feat(appsync): Standalone L2 construct for SourceApiAssociation by ndejaco2 · Pull Request #27121 · aws/aws-cdk

ndejaco2 · 2023-09-13T00:17:19Z

As part of supporting AppSync Merged APIs, this change introduces a standalone SourceApiAssociation construct for declaring a source api association between a source API and a Merged API.

Why do we need a standalone construct?

There are two potential deployment models when dealing with separate stacks/pipelines between the source API and Merged API: 1. Push model where the source API owners manage the association in their stack 2. Pull model where the associations are managed in the Merged API stack.
Having a standalone construct gives developers more flexibility while still handling all the IAM permission handling in a single place.
Developers can continue to use the GraphQLApi construct and declare the source api configuration all within a single construct as before. But, if they want to have the source api association as a standalone object this change gives them flexibility

I also fixed two issues related to IAM:

The resource for appsync:SourceGraphQL needs both the source api arn and the source api arn + "/*" to get all top level fields.
The merged api execution role also needs appsync:StartSchemaMerge if the association is using AUTO_MERGE. The fix here is preferred over existing PR: fix(appsync): Source APIs are not auto-merged by default #27025

Closes #26986

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

…rt of AppSync Merged APIs

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

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

ndejaco2 · 2023-09-13T16:48:34Z

I went ahead and undid the renaming to not introduce a breaking change. I believe it would benefit from updating the naming, but its not a high priority worth dealing with in this PR.

ndejaco2 · 2023-09-14T16:13:38Z

@rix0rrr any chance you could review? On the service team, we have been cut a few tickets regarding the IAM permissions bugs as well as the construct for Merged APIs so I would like to get this one addressed.

… not introduce breaking change

…es cause removal

MrArnoldPalmer

Just some small stuff for now. Gonna ask for another set of eyes since this is new api surface area that we will need to guarantee backwards compatibility on.

Pull request has been modified.

rix0rrr · 2023-09-18T09:43:51Z

+      // Add permissions to merged api execution role, only if it was not passed in.
+      if (!this.definition.sourceApiOptions?.mergedApiExecutionRole) {
+        const executionRole = this.mergedApiExecutionRole as IRole;


This is not the right way to allow that use case.

If people are passing in a role and they don't want it modified, they can pass role.withoutPolicyUpdates(), or import an existing one with Role.fromRoleName({mutable: false }).

So when implementing a construct, you should feel free to always update the role.

The existing support works like this where it is not updating the role policy if it was passed in. Would it be a breaking change?

it might be... since this is pre-existing we can look at changing this separately.

rix0rrr · 2023-09-18T09:46:33Z

+  /**
+   * Import Appsync Source Api Association from source API, merged api, and merge type.
+   */
+  public static fromSourceApiAssociationAttributes(scope: Construct, id: string, attrs: SourceApiAssociationAttributes): ISourceApiAssociation {


I'm not necessarily against adding this function, but its use is very unlikely, right?

There isn't a single API that accepts an ISourceApiAssociation ?

Yea not currently, but it will be necessary for this: #27170

…assed in or not

Pull request has been modified.

Co-authored-by: Mitchell Valine <mitchellvaline@gmail.com>

Pull request has been modified.

mergify · 2023-09-20T17:41:46Z

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

Pull request has been modified.

aws-cdk-automation · 2023-09-20T18:11:29Z

AWS CodeBuild CI Report

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

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

mergify · 2023-09-20T18:11:45Z

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

feat(appsync): Standalone L2 construct for SourceApiAssociation as pa…

73edebc

…rt of AppSync Merged APIs

github-actions Bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Sep 13, 2023

aws-cdk-automation requested a review from a team September 13, 2023 00:17

github-actions Bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Sep 13, 2023

ndejaco2 mentioned this pull request Sep 13, 2023

fix(appsync): Source APIs are not auto-merged by default #27025

Closed

aws-cdk-automation previously requested changes Sep 13, 2023

View reviewed changes

ndejaco2 added 2 commits September 12, 2023 17:29

Update README for SourceApiAssociation construct

6bd2f05

Change naming in README

2ad634c

Undo renaming of input types

7fe1aca

ndejaco2 mentioned this pull request Sep 13, 2023

(aws_appsync): Unable to reference external SourceApis in Defintions.fromSourceApis() #27098

Closed

Remove unused param

be34bd7

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 13, 2023

ndejaco2 added 9 commits September 14, 2023 15:42

Simplify interface and support imported source apis using IGraphqlApi

e1e445d

Add additional snapshot test for flag enabled

83a806f

README update

681ddae

Remove use of SourceApiAssociation construct in existing construct to…

4abd223

… not introduce breaking change

Updated breaking change to allow

17842e5

Use feature flag in GraphqlApi construct as changing this property do…

8219b13

…es cause removal

Attempt to fix build by setting flag to always be false in integ test

c010e9c

Use postCliContext so flag takes precedence

0a02889

Add ISourceApiAssociation for import and update README

ef4d105

MrArnoldPalmer previously requested changes Sep 15, 2023

View reviewed changes

Comment thread packages/aws-cdk-lib/aws-appsync/lib/graphqlapi.ts

Comment thread packages/aws-cdk-lib/aws-appsync/lib/source-api-association.ts

Comment thread packages/aws-cdk-lib/aws-appsync/lib/source-api-association.ts

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 15, 2023

Add note about resource replacement

05129ed

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 15, 2023

ndejaco2 mentioned this pull request Sep 15, 2023

(aws-appsync): Custom Resource to Perform Manual Merge of a Source API And Signal Cloudformation Based On Merge Operation Result #27170

Closed

2 tasks

rix0rrr previously requested changes Sep 18, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 18, 2023

Add separate example in README and add permissions whether role was p…

4b1f35d

…assed in or not

ndejaco2 added 3 commits September 18, 2023 10:37

Fix unit test

17a8ce2

Remove trailing whitespace

6d17bfd

Update README

47e08e8

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 18, 2023

ndejaco2 requested review from MrArnoldPalmer and rix0rrr September 18, 2023 19:21

MrArnoldPalmer previously requested changes Sep 20, 2023

View reviewed changes

Comment thread packages/aws-cdk-lib/aws-appsync/lib/graphqlapi.ts Outdated

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 20, 2023

PR feeback

2144db3

Co-authored-by: Mitchell Valine <mitchellvaline@gmail.com>

MrArnoldPalmer previously approved these changes Sep 20, 2023

View reviewed changes

Merge branch 'main' into main

4152ad9

MrArnoldPalmer approved these changes Sep 20, 2023

View reviewed changes

mergify Bot merged commit 7b9652e into aws:main Sep 20, 2023

github-actions Bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

Conversation

ndejaco2 commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

ndejaco2 commented Sep 13, 2023

Uh oh!

ndejaco2 commented Sep 14, 2023

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rix0rrr Sep 18, 2023 • 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!

ndejaco2 Sep 18, 2023 • 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!

MrArnoldPalmer Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

rix0rrr Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

ndejaco2 Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mergify Bot commented Sep 20, 2023

Uh oh!

aws-cdk-automation commented Sep 20, 2023

AWS CodeBuild CI Report

Uh oh!

mergify Bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ndejaco2 commented Sep 13, 2023 •

edited

Loading

rix0rrr Sep 18, 2023 •

edited

Loading

ndejaco2 Sep 18, 2023 •

edited

Loading