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(appsync): environmentVariables property for GraphqlApi #29064

Merged
merged 12 commits into from
Feb 23, 2024

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Feb 10, 2024

Reason for this change

AppSync now supports environment variables in GraphQL resolvers and functions. It would be good for GraphqlApi construct to have the property.

Description of changes

The environmentVariables property is added to GraphqlApi construct. To add environment variables after the initiation, we can use addEnvironmentVariables method.

Description of how you validated changes

Both unit and integ tests.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team February 10, 2024 18:14
@github-actions github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Feb 10, 2024
@go-to-k go-to-k marked this pull request as ready for review February 10, 2024 19:03
@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 Feb 10, 2024
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Hi @go-to-k! I just have one question really. the rest looks good

}

private validateEnvironmentVariables() {
this.node.addValidation( {
Copy link
Contributor

Choose a reason for hiding this comment

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

why add validation to the node rather than just throwing an error here? I'm not too familiar with this method but usually we want to default to fail as early as possible and i wonder if this violates that. did you see this done elsewhere?

Copy link
Contributor Author

@go-to-k go-to-k Feb 14, 2024

Choose a reason for hiding this comment

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

The addValidation (see DESIGN_GUIDELINES) method is a lazy way to delay and make decisions about processing that cannot be determined during initialization by the constructor.

For example, in this case, we get an array-type property from props and initialize an instance variable in the constructor. However, there are cases where users want to pass nothing in props and add it later with the addEnvironmentVariable method. In that case, if we check the number of elements in the array in the constructor, it will be assumed to be zero.
Because of this case, we should use this method for lazy checking instead of early checking at the constructor.

Also, if we were to check the number of elements in the array in the addEnvironmentVariable method, it would throw an error the moment the maximum number is reached, so even if users specify 60 elements for the array, they would get a message Only 50 environment variables can be set, got 51. The addValidation method is also necessary to check exactly how many elements the user has specified for the array.

Elsewhere, I saw this in CodePipeline.

https://github.com/go-to-k/aws-cdk/blob/a21731c9b1f34525433fbee668ebb2321debf5b8/packages/aws-cdk-lib/aws-codepipeline/lib/pipeline.ts#L542

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it a little clearer.

1a100b5

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the clarification. i agree that this is the right thing to do here now

@go-to-k
Copy link
Contributor Author

go-to-k commented Feb 14, 2024

@kaizencc

Thanks for your review. I have returned your comment and would like you to confirm it.

@onlybakam
Copy link
Contributor

This looks great. can you add a check to make sure that environment variables are not set on a MERGED api?

@go-to-k
Copy link
Contributor Author

go-to-k commented Feb 15, 2024

@onlybakam

Thanks, nice catch. I will add the change not to set the environment variables on the Merged API.

Comment on lines +877 to +879
if (this.definition.sourceApiOptions) {
throw new Error('Environment variables are not supported for merged APIs');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Environment variables are not supported for merged API. The Cfn errors:

MergedAPI (MergedAPI08D3EAD1) Operation not allowed, Merged API resources are read-only (Service: AmazonDeepdish; Status Code: 400; Error Code: BadRequestException; Request ID: ...

@go-to-k
Copy link
Contributor Author

go-to-k commented Feb 15, 2024

@kaizencc

I have also added new commits, could you please check them?

Copy link
Contributor

mergify bot commented Feb 20, 2024

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 aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 20, 2024
Copy link
Contributor

mergify bot commented Feb 20, 2024

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

@go-to-k
Copy link
Contributor Author

go-to-k commented Feb 21, 2024

@kaizencc

Thanks for your approval, but the merge has failed because of Queue: Embarked in merge queue.

Could you please trigger again?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

mergify bot commented Feb 23, 2024

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 f0af5b1 into aws:main Feb 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2
Projects
None yet
4 participants