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: S2 Input and Output plugins #222

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

vrongmeal
Copy link
Contributor

No description provided.

@vrongmeal
Copy link
Contributor Author

The lint failures don't seem to be related to this change.

@vrongmeal
Copy link
Contributor Author

There seems to be some issue with the specific golangci-lint version on why the lint timeouts.

@gregfurman gregfurman assigned gregfurman and vrongmeal and unassigned gregfurman Jan 30, 2025
@gregfurman gregfurman self-requested a review January 30, 2025 10:12
Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome contribution! 🚀 Have left some comments. Mostly cosmetic suggestions (feel free to ignore nits) but some (like the use of panic) require some feedback.

Also, is there any chance you can add an integration test? Doesn't need to be complicated but would just provide some sanity checks 🙂 See this NATS test for an example.

Also, I'll look into this weird golangci-lint issue.

@vrongmeal
Copy link
Contributor Author

Hey @gregfurman. Thank you for your review. I'll address most of your concerns with the PR.

Also, is there any chance you can add an integration test?

I don't think I can do this right-now. We do want to create a mock-server that we can use for testing. Will add an integration test as soon as we have that.

@vrongmeal
Copy link
Contributor Author

@gregfurman I have updated the PR.

Copy link
Collaborator

@gregfurman gregfurman 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 addressing comments!

Golang lint job seemed to be OOM killed. Looks like an issue with Go v1.23. (from golangci/golangci-lint#4909) and fixed in 1.63.x. Will update the CI to a later version.

Also, can you double check on that s2 dependency requiring 1.23.4?

Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fast response time on my feedback @vrongmeal!

I'll look into creating a mock server for this plugin similar to what we do for GCP with http handlers created on-the-fly.

Lemme know if all works fine! Else, reach out on Slack or Discord if you need any help.

BTW, love the product! I recall seeing S2 on HackerNews a couple of months ago and am excited to see where the project goes 🚀 🙂

@gregfurman gregfurman merged commit eef335c into warpstreamlabs:main Feb 3, 2025
3 checks passed
@vrongmeal
Copy link
Contributor Author

Thanks for your kind words!

Loved working on Bento plugins. The architecture's super extendable and everything just works so smoothly! Really glad to have S2 plugins merged.

I'll look into creating a mock server for this plugin similar to what we do for GCP with http handlers created on-the-fly.

This is so great! For testing Bento, I think implementing just the StreamService should suffice. There might be some changes required on the SDK side (for eg, HTTPS is enabled by default and cannot be configured). Would love to help out!

@vrongmeal vrongmeal deleted the s2-input-output branch February 3, 2025 16:45
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