-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): addRdsDataSource support for DatabaseCluster #29544
feat(AppSync): addRdsDataSource support for DatabaseCluster #29544
Conversation
…ort rds serverless v2
There was a problem hiding this 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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this 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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
@@ -187,7 +187,7 @@ export interface IGraphqlApi extends IResource { | |||
*/ | |||
addRdsDataSource( | |||
id: string, | |||
serverlessCluster: IServerlessCluster, | |||
serverlessCluster: IServerlessCluster | IDatabaseCluster, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not be acceptable change. This function is a public API that customers can use. Although it's perfectly fine in typescript, CDK uses JSII to translate typescript to other languages and this specific code change will break JSII compilation unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made update to have 2 different method to avoid jsii conversion issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for making the changes and addressing the feedback.
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). |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Issue # (if applicable)
(aws-appsync): addRdsDataSource doesnt support taking a DatabaseCluster
Closes #29302
Reason for this change
AppSync CDK construct currently accept only IServerlessCluster for RDS source as cluster type. However, with Aurora V2, serverless aurora clusters such as postgres aurora v14 and above are construct using the DatabaseCluster construct and as such AppSync.addRdsDataSource() method need ability to support both type of cluster interfaces.
Description of changes
To support the change I created a second props to support IDatabaseCluster in addition to IServerlessCluster already supported and I overloaded the constructor to support both type of props.
However, to make the change possible, some modification to aws-rds were also required:
1 - Need IDatabaseCluster interface to have grantDataApiAccess() method published as part of the interface (the method was there but not published in the interface, when IServerlessCLuster interface have it published.
2 - need DatabaseCluster.grantDataApiAccess() to follow the same IAM permission pattern than ServerlessCluster.grantDataApiAccess() to have consistency.
ServerlessCluster.grantDataApiAccess() method is adding automatically the required permission to secret manager if Data API is enabled.
However, DatabaseCluster.grantDataApiAccess() do not. As such without the change it will have been required for end users to add that IAM permission to secret manager as an extra line when they will have assigned a serverless V2 cluster to AppSync datasource.
To keep the experience unified across the 2 type of RDS clusters, i updated the method to have that IAM permission embedded.
Description of how you validated changes
I run all unit test and the integration test successfully.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license