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): add RDS datasource #9258

Merged
merged 28 commits into from Nov 2, 2020
Merged

feat(appsync): add RDS datasource #9258

merged 28 commits into from Nov 2, 2020

Conversation

kochie
Copy link
Contributor

@kochie kochie commented Jul 25, 2020

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

@mergify
Copy link
Contributor

mergify bot commented Jul 25, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@kochie kochie changed the title feat(appsync) added rds datasource for appsync feat(appsync): Added rds datasource for appsync Jul 25, 2020
Copy link
Contributor

@BryanPan342 BryanPan342 left a 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.

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
@BryanPan342 BryanPan342 linked an issue Jul 31, 2020 that may be closed by this pull request
2 tasks
@skinny85 skinny85 changed the title feat(appsync): Added rds datasource for appsync feat(appsync): add RDS datasource Jul 31, 2020
kochie and others added 3 commits August 4, 2020 11:45
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
Co-authored-by: Bryan Pan <bryanpan342@gmail.com>
@mergify mergify bot dismissed BryanPan342’s stale review August 4, 2020 01:46

Pull request has been modified.

@BryanPan342
Copy link
Contributor

@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

Copy link
Contributor

@BryanPan342 BryanPan342 left a 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 🎉

@MrArnoldPalmer
Copy link
Contributor

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.

@kochie
Copy link
Contributor Author

kochie commented Oct 15, 2020

Excellent @MrArnoldPalmer, I'll give it a tidy up and get it done.

@gitpod-io
Copy link

gitpod-io bot commented Oct 17, 2020

@kochie
Copy link
Contributor Author

kochie commented Oct 19, 2020

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.

Copy link
Contributor

@BryanPan342 BryanPan342 left a 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

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@BryanPan342 BryanPan342 left a 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

@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2020

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ee00839
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2020

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

@mergify mergify bot merged commit 23d0943 into aws:master Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appsync Related to AWS AppSync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-appsync] Add Relational Database (rds) datasource
4 participants