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.

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(cdk): add AppSync GraphQLSchema and pipeline resolvers as hot swappable #27197

Merged
merged 5 commits into from
Sep 28, 2023

Conversation

Amplifiyer
Copy link
Contributor

  1. Add GraphQLSchema as another AppSync resource that can be hotswapped
  2. For all AppSync resources, accept the change in S3 assets/files instead of just inline code as a candidate for hotswap
  3. Make pipeline resolvers hotswappable by resolving the functionId of AppSync functions.

Closes #2659, #24112, #24113.


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

@Amplifiyer Amplifiyer changed the title feat(aws-cdk): add AppSync GraphQLSchema and pipeline resolvers as hot swappable feat(cdk): add AppSync GraphQLSchema and pipeline resolvers as hot swappable Sep 19, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team September 19, 2023 15:17
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Sep 19, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Sep 19, 2023
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

looks really good, overall!

};
const evaluatedResourceProperties = await evaluateCfnTemplate.evaluateCfnExpression(sdkProperties);
const sdkRequestObject = transformObjectKeys(evaluatedResourceProperties, lowerCaseFirstCharacter);

// resolve s3 location files as SDK doesn't take in s3 location but inline code
if (sdkRequestObject.requestMappingTemplateS3Location) {
sdkRequestObject.requestMappingTemplate = (await fetchFileFromS3(sdkRequestObject.requestMappingTemplateS3Location, sdk))?.toString('utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the user flow look like with any of the *S3Location properties? The MappingTemplate doesn't support an S3 location, and it also doesn't use it under the hood.

Even if it was supported, the user would have to change the s3 location for hotswap to be able to see it, not the object itself; diff will only detect a change in the object's location, not its contents. Users won't want to rename the object every time they make a change to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tagging @alharris-at for more details. I believe we are using L1 CFN resource here and manually managing the requestMappingTemplateS3Location properties from our L3 data construct. (Don't know if it's the right reference https://github.com/aws-amplify/amplify-category-api/blob/c6772cfb9c0c68d6de4c04aa8190489fba0557a9/packages/graphql-transformer-core/src/util/SchemaResourceUtil.ts#L37)

In my testing, when updating high level amplify schema that results in a resolver change, I found only *S3Location properties to have changed. @alharris-at can you provide more details here?

Copy link

@alharris-at alharris-at Sep 27, 2023

Choose a reason for hiding this comment

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

Because Synth produces assets with a hash in the filename, I think the S3Location will change if a resolver or function's code is updated, and yeah we use L1 constructs here.

@comcalvi not urgent, but we might actually want to open the discussion to using S3Location instead of the inline approach for the L2 (when a file is provided), since that means customers will far more quickly hit Cfn template size limits (we see this happen for Amplify customers w/o inlining much of these assets, since with AppSync it's easy to produce a large number of resources quickly, and if we're inlining all code then it's going to expand very quickly).

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh okay, so the s3 locations are managed through the CDK here. That makes sense, thanks for clarifying.

Comment on lines +83 to +86
await simpleRetry(
() => sdk.appsync().updateFunction({ ...sdkRequestObject, functionId: functionId! }).promise(),
3,
'ConcurrentModificationException');
Copy link
Contributor

@comcalvi comcalvi Sep 20, 2023

Choose a reason for hiding this comment

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

This is a good workaround for the orchestration / resource dependency problem, nice work

} else {
let schemaCreationResponse: GetSchemaCreationStatusResponse = await sdk.appsync().startSchemaCreation(sdkRequestObject).promise();
while (schemaCreationResponse.status && ['PROCESSING', 'DELETING'].some(status => status === schemaCreationResponse.status)) {
await new Promise(resolve => setTimeout(resolve, 1000)); // poll every second
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, updated.

@comcalvi comcalvi self-assigned this Sep 20, 2023
@Amplifiyer Amplifiyer marked this pull request as ready for review September 21, 2023 18:17
@alharris-at
Copy link

@Amplifiyer - looks like this doesn't support code props yet. Is that tracked in a separate PR, or is there a good reason to separate out? Seems like a the change will have significant overlap w/ the updates to S3Locatio, etc.

@Amplifiyer
Copy link
Contributor Author

Amplifiyer commented Sep 27, 2023

@Amplifiyer - looks like this doesn't support code props yet. Is that tracked in a separate PR, or is there a good reason to separate out? Seems like a the change will have significant overlap w/ the updates to S3Locatio, etc.

@alharris-at, Yes I intend to add the Code in a follow up PR. Can you expand on what the overlap you mentioned here? Nvm, it was pretty easy to just add it to this one.

@Amplifiyer Amplifiyer temporarily deployed to test-pipeline September 28, 2023 11:49 — with GitHub Actions Inactive
@comcalvi comcalvi added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Sep 28, 2023
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Sep 28, 2023
@comcalvi comcalvi added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 28, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 28, 2023 20:48

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

@mergify
Copy link
Contributor

mergify bot commented Sep 28, 2023

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: 7dabd1c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 0ea6313 into aws:main Sep 28, 2023
7 of 8 checks passed
@mergify
Copy link
Contributor

mergify bot commented Sep 28, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants