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

Support Kraft mode for Kafka container #723

Merged
merged 6 commits into from
Mar 19, 2024

Conversation

silh
Copy link
Contributor

@silh silh commented Mar 7, 2024

Kraft is out of beta for some time now and and soon zookeeper support will be completely removed so it's useful to prepare for kraft support. It was also possible to use testcontainers-java without Zookeeper since 1.18.0.

This PR ports the solution to enable usage of Kafka without Zookeeper from testcontainers-java.

Unrelated changes:

  • fixed copyContentToContainer not setting the correct mode for the file inside the container.

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 9c35f94
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/65f74276c4ebc000083d342a
😎 Deploy Preview https://deploy-preview-723--testcontainers-node.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 configuration.

@silh silh changed the title Added support for Kraft mode for Kafka container Support Kraft mode for Kafka container Mar 7, 2024
@cristianrgreco cristianrgreco added enhancement New feature or request minor Backward compatible functionality labels Mar 12, 2024
@silh silh marked this pull request as ready for review March 12, 2024 15:04
@silh
Copy link
Contributor Author

silh commented Mar 12, 2024

Hey @cristianrgreco , fixed all the problems. Now the PR is ready for review.

@cristianrgreco
Copy link
Collaborator

Thanks @silh, could you please also add an example to the Kafka docs showing how to use Kraft with the KafkaContainer?

@silh
Copy link
Contributor Author

silh commented Mar 15, 2024

@cristianrgreco done 👍

const boundPorts = BoundPorts.fromInspectResult(client.info.containerRuntime.hostIps, inspectResult).filter(
this.exposedPorts
);
await waitForContainer(client, dockerContainer, this.originalWaitinStrategy, boundPorts);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is different, to manually invoke a wait strategy after a container is said to have started. Ideally we should get this working just by setting this.waitStrategy and nothing else. E.g would it work if on line 139 we do

this.waitStrategy = Wait.forAll([
  this.waitStrategy, 
  Wait.forLogMessage(WAIT_FOR_SCRIPT_MESSAGE)
]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work unfortunately.

The main problem is we need to gather some info which is only available in when container is started and copy it to the container. Unlike the java implementation, some of this methods are not available on not-started container. It can be achieved, but the code will be messier and will have some duplicated functionality. I'll try to prepare a commit with that implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, we can look at this some other time. For now I think it's more important to add support for Kraft

@cristianrgreco cristianrgreco merged commit f7fc568 into testcontainers:main Mar 19, 2024
106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants