-
Notifications
You must be signed in to change notification settings - Fork 4k
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): add RDS datasource #9258
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
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.
Thanks for the contribution @kochie 🎉
It's a great first stab! Some parts that need addressing.. specifically the granting to allow appsync to perform operations on the data source.
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
Pull request has been modified.
@kochie there have been some big changes to the code base and I had the bandwidth to implement my suggestions! Please make changes if anything looks out of place |
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.
@MrArnoldPalmer @shivlaks this PR LGTM but I made changes to it so I'm definitely biased 🙃
Can I get a second pair of eyes to review this that way we can get @kochie's changes into production 🎉
Sorry for the delay here. With conflicts resolved I think I'm good to approved this. @kochie if you're able to resolve those that would be great. If not I'll try to get to it in the next few days. |
Excellent @MrArnoldPalmer, I'll give it a tidy up and get it done. |
Hey @MrArnoldPalmer, do you mind giving it a once over again, I had to change some of the objects in tests to get a clean build. |
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.
@MrArnoldPalmer @shivlaks LGTM after these changes that I think are a product of merging
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
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 🥳
@MrArnoldPalmer passes my check
Thank you for contributing! Your pull request will be updated from master 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Very, very preliminary attempt at adding RDS data source to AppSync.Still need to fix tests and lint.This PR adds support for RDS as a datasource for AppSync.
There are several examples included in the README, integration tests, and documentation.
Fixes #9152
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license