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: Add Redpanda module #1058

Merged
merged 2 commits into from
May 6, 2023
Merged

Conversation

weeco
Copy link
Contributor

@weeco weeco commented Apr 15, 2023

What does this PR do?

This PR adds a module for Redpanda.

Why is it important?

Redpanda is a Kafka API compatible streaming solution. Compared to Apache Kafka this starts very quickly, is easier to operate, doesn't require ZooKeeper, it's very lightweight and also comes with schema registry on board which makes it the ideal testcontainer for users that want to test their Kafka services.

Related issues

Run the provided go test.

@weeco weeco requested a review from a team as a code owner April 15, 2023 21:25
@netlify
Copy link

netlify bot commented Apr 15, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 9b770ba
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6455f59646082900089bda96
😎 Deploy Preview https://deploy-preview-1058--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@weeco weeco force-pushed the redpanda-module branch from 266e857 to ff4d9c6 Compare April 16, 2023 14:19
Copy link
Member

@eddumelendez eddumelendez 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, @weeco! In Testcontainers for Java, Redpanda module was introduced and the image used to simplify the configuration was docker.redpanda.com/vectorized/redpanda:v22.2.1. We already use docker.redpanda.com/redpandadata/redpanda. I was wondering if, in order to keep consistency cross-language implementation we can do the same. Recently, there have been some changes in Testcontainers for Go in order to introduces container lifecycle, see #1036 #1037. I also wonder how the redpanda.yaml.tpl plays with the config we already have in Testcontainers for Java in order to enable more features.

@weeco
Copy link
Contributor Author

weeco commented Apr 17, 2023

Hey @eddumelendez ,
thanks for taking a look at this PR. This module uses the right docker registry and the Java module should be updated to use this registry and version by default, too. In general I expect this Go module to be the source of truth, because the majority of our engineers use Go. Once this module will be used internally, it will be maintained by more people but it will take some extra time until we get there. I can ping our Java contributors to update the image in the Java module if needed.

Regarding container lifecycles - I totally forgot about that; I'll look into them later today. I think the PostStart hook may work here.

I also wonder how the redpanda.yaml.tpl plays with the config we already have in Testcontainers for Java in order to enable more features.

In general I'd say that the Go module supports way more features. The Java module seems to rely on our rpk cli to take care of the configuration. Because this module is more complex and therefore requires more configuration this approach wouldn't work here anymore.

mdelapenya
mdelapenya previously approved these changes Apr 24, 2023
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

@weeco thanks for this great module! Looks pretty neat!

I only have one small nit, that we could address in a follow up PR (or here, whatever you prefer): after merging with upstream, there is a need to run go mod tidy, which is trivial.

I'm also adding a suggestion for the docs, as we added a tag icon with the version in which a module was introduced.

Other than that, great job!! Thanks!

@mdelapenya mdelapenya changed the title Add Redpanda module feat: Add Redpanda module Apr 24, 2023
@mdelapenya mdelapenya self-assigned this Apr 24, 2023
@mdelapenya mdelapenya added the enhancement New feature or request label Apr 24, 2023
@weeco weeco force-pushed the redpanda-module branch 2 times, most recently from 8e481e4 to b32bcdf Compare April 24, 2023 17:25
@weeco weeco requested a review from mdelapenya April 24, 2023 17:26
@weeco
Copy link
Contributor Author

weeco commented Apr 24, 2023

Thanks for reviewing @mdelapenya and @eddumelendez .

I committed your suggested change, also tidied the Go modules and squashed my commits. I'm unsure how to address @eddumelendez's comment as this client is just a HTTP wrapper to create the Redpanda users once the cluster is started. This is only needed if the user uses authentication. If we remove this feature the user would be in charge of creating the users on their own via the Redpanda admin client that is being used.

@mdelapenya
Copy link
Member

I'm unsure how to address @eddumelendez's comment as this client is just a HTTP wrapper to create the Redpanda users once the cluster is started

As commented online, I good step to avoid a dependency would be a little of copying :) I wonder if you could copy the code you need from the rpk/admin package into the module 🤔

@weeco
Copy link
Contributor Author

weeco commented May 5, 2023

@mdelapenya I replaced the rpk dependency by adding a little helper client which currently only supports this specific endpoint for adding users. Appreciate another pass on this.

@weeco weeco force-pushed the redpanda-module branch from b32bcdf to 442d239 Compare May 5, 2023 16:42
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

@weeco thanks for this contribution. I liked a lot reviewing the templating part, which I think will help with an upcoming support for different versions.

LGTM!

Comment on lines +19 to +26
//go:embed mounts/redpanda.yaml.tpl
nodeConfigTpl string

//go:embed mounts/bootstrap.yaml.tpl
bootstrapConfigTpl string

//go:embed mounts/entrypoint-tc.sh
entrypoint []byte
Copy link
Member

Choose a reason for hiding this comment

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

I liked this to embed the static resources! 👏

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mdelapenya mdelapenya merged commit 965f85b into testcontainers:main May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Kafka module
3 participants