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

Added support for AppSync DataSource invocations #144

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

bendubuisson
Copy link
Contributor

Proposed changes

This adds support for AppSync DataSources. Tested with the serverless-appsync-plugin plugin.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

I had to change the lifecycle hook we use as the plugin adding DataSources (mentioned earlier) happens later in the cycle, so the DataSources weren't in the CF template yet... Let me know if you think it will be a problem?

@davidgf
Copy link
Owner

davidgf commented Mar 3, 2022

It looks great, thank you very much for your contribution! I'd like to ask for a minor request before merging it though, could you add integration tests? To do that, you only need to add the appropriate input, output and service files to the fixtures/ folder, which you can achieve by copying the latest samples (15.input.with-long-name.json, 15.output.with-long-name.json and
15.service.with-long-name.json) and adding the expected CloudFormation bits for AppSync.

@bendubuisson
Copy link
Contributor Author

Hey man will get on that, thanks. I think I'll need to use Resources and CloudFormation since AppSync is not really supported?

@davidgf
Copy link
Owner

davidgf commented Mar 8, 2022

I'm not familiar with AppSync, but if it's not supported by the Serverless framework yeah, you'll most likely have to use Resources. Now that you mention it, it would be great if you could also modify the example/ to show how this new feature would be used

@bendubuisson
Copy link
Contributor Author

Hey @davidgf, added integration tests as requested, I'm not sure what you mean by updating the example bit, as this should just work with AppSync DataSources?

@bendubuisson
Copy link
Contributor Author

@davidgf hey mate, any updates on this when you get a chance please? Cheers

@davidgf davidgf merged commit 81e6679 into davidgf:master Apr 11, 2022
@davidgf
Copy link
Owner

davidgf commented Apr 11, 2022

Hey @bendubuisson, first of all, I'm very sorry for the delay, but my professional commitments are keeping me quite busy. By examples I mean that there is an example folder in this repository, which includes a serverless application with a function that is triggered by all the event sources supported by this plugin and all the necessary AWS resources. This helps me testing the new functionality and serves as an example for anyone using the plugin. I'd be very grateful if you could extend the example to test the AppSync trigger, but I'm merging the PR anyway

@davidgf davidgf mentioned this pull request Apr 11, 2022
@bendubuisson
Copy link
Contributor Author

Hey @davidgf, don't be sorry, I perfectly understand! Thank you for the hard work and I'll look into your request!

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants