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

DBZ-7214 AWS SQS as sink type #53

Closed
wants to merge 1 commit into from
Closed

DBZ-7214 AWS SQS as sink type #53

wants to merge 1 commit into from

Conversation

koneru9999
Copy link
Contributor

@koneru9999 koneru9999 commented Dec 1, 2023

https://issues.redhat.com/browse/DBZ-7214

The implementation mostly is a clone of existing kinesis sink.

@jpechane
Copy link
Contributor

jpechane commented Dec 5, 2023

@koneru9999 Thanks for the PR! Would you be willing to develop it a bit more if possible? Since Kinesis came TestContainers (https://java.testcontainers.org/modules/localstack/) with localstack support were made available. We use test containers in other tests like RocketMqIT. Would you be willing to try and rewrite the test to not run against AWS but against the local container so it ould be executed as standard part of CI? If it is something that is technically viable then we might retorfit Kinesis test to use it too.
WDYT?

@koneru9999
Copy link
Contributor Author

koneru9999 commented Dec 5, 2023

@koneru9999 Thanks for the PR! Would you be willing to develop it a bit more if possible? Since Kinesis came TestContainers (https://java.testcontainers.org/modules/localstack/) with localstack support were made available. We use test containers in other tests like RocketMqIT. Would you be willing to try and rewrite the test to not run against AWS but against the local container so it ould be executed as standard part of CI? If it is something that is technically viable then we might retorfit Kinesis test to use it too. WDYT?

That makes sense! I will have a re-look at the tests. 👍

@koneru9999
Copy link
Contributor Author

@jpechane This PR has now been adjusted to use localstack module from Testcontainers. I have not modified the existing kinesis tests as they are beyond the scope of this PR IMHO.

@jpechane
Copy link
Contributor

@koneru9999 LGTM. I triggerd the CI and if it passes could you please suqsh the commits into a single one and remove the merge commit? The final commit message should start with DBZ-7214 . Thanks

@koneru9999
Copy link
Contributor Author

@koneru9999 LGTM. I triggerd the CI and if it passes could you please suqsh the commits into a single one and remove the merge commit? The final commit message should start with DBZ-7214 . Thanks

@jpechane I have now squashed the commits into a single commit even though the build failed on CI but on the other modules. Please have a final look.

@jpechane
Copy link
Contributor

@koneru9999 Merged via #59, thanks a lot!
Could you please sheare your name so I can add you to the list of contributors?
Also could you please prepare a PR with documentation?

@jpechane jpechane closed this Dec 14, 2023
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.

2 participants