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 network support to the Kafka container #1316

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

SebastienDegodez
Copy link
Contributor

@SebastienDegodez SebastienDegodez commented Dec 15, 2024

What does this PR do?

This PR adds the functionality to support network within a Docker container.

Why is it important?

Network isolation is crucial for testing in an environment that closely mimics production. It ensures that the containers can communicate within a controlled network, similar to how they would in a real-world scenario.

Related issues

How to test this PR

  1. Build and start the Kafka and kcat containers.
  2. Execute the test cases in KafkaContainerNetworkTest to verify message production and consumption.
  3. Check the logs to ensure the message "Message produced by kcat" is correctly produced and consumed.

Follow-ups

  • Consider adding the same functionality for the redpanda module.

Copy link

netlify bot commented Dec 15, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit fa508a6
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/6785dec485c9e90008f07fe8
😎 Deploy Preview https://deploy-preview-1316--testcontainers-dotnet.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.

@SebastienDegodez
Copy link
Contributor Author

I have some problems to run cake on my computer. If you can do some checks before continue (or if you have documentation to contribute on macos).

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Ignore this. I was in the wrong tab - sorry.

@HofmeisterAn
Copy link
Collaborator

QQ: Does this address #736 too?

@SebastienDegodez
Copy link
Contributor Author

QQ: Does this address #736 too?

I think yes, do you want me to take the test?

@HofmeisterAn
Copy link
Collaborator

QQ: Does this address #736 too?

I think yes, do you want me to take the test?

That would be really kind. Then we can link the PR to the issue. I'll try to review the PR sometime this week.

@eddumelendez
Copy link
Member

Yes, this fixes #736 given the new advertised listener is added. I have an example in java working with schema registry https://github.com/eddumelendez/testcontainers-samples/blob/main/spring-boot-kafka/src/test/java/com/example/consumer/SpringBootKafkaAvroTest.java#L39-L57

@SebastienDegodez
Copy link
Contributor Author

SebastienDegodez commented Jan 9, 2025

QQ: Does this address #736 too?

I think yes, do you want me to take the test?

That would be really kind. Then we can link the PR to the issue. I'll try to review the PR sometime this week.

You can find a new commit with registry.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn 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 taking the time and contributing. I just had the chance to review the PR. The PR looks good; I made a few minor changes and aligned them with the repository standards. Let me know if you're okay with it. I haven't had the time to review the tests yet, but I will either this evening or sometime tomorrow. Thanks again for the PR!

@SebastienDegodez
Copy link
Contributor Author

Thanks for taking the time and contributing. I just had the chance to review the PR. The PR looks good; I made a few minor changes and aligned them with the repository standards. Let me know if you're okay with it. I haven't had the time to review the tests yet, but I will either this evening or sometime tomorrow. Thanks again for the PR!

It's OK for me. The code is community no just for me.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

I have completed the review and noticed that the KafkaContainerRegistryTest threw the following error:

Schema being registered is incompatible with an earlier schema for subject "user-topic-value"; error code: 409.

Was this intentional? I made changes to the schema to match the actual topic. Could you please review these changes? After that, I will be happy to merge the PR. Thanks again!

@HofmeisterAn HofmeisterAn added bug Something isn't working enhancement New feature or request labels Jan 13, 2025
@HofmeisterAn HofmeisterAn changed the title feat(kafka): Add network support for Kafka container feat: Add network support to the Kafka container Jan 13, 2025
@SebastienDegodez
Copy link
Contributor Author

SebastienDegodez commented Jan 13, 2025

I have completed the review and noticed that the KafkaContainerRegistryTest threw the following error:

Schema being registered is incompatible with an earlier schema for subject "user-topic-value"; error code: 409.

Was this intentional? I made changes to the schema to match the actual topic. Could you please review these changes? After that, I will be happy to merge the PR. Thanks again!

My intention was to show that the registry schema was wired correctly by rejecting a message that did not match the contracts.
I was afraid that on a message matching the contract, The success does not validate really the correctly linked registry schema.

Copy link
Contributor Author

@SebastienDegodez SebastienDegodez left a comment

Choose a reason for hiding this comment

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

Review done, it's ok for me.

@HofmeisterAn
Copy link
Collaborator

The success does not validate really the correctly linked registry schema.

Thanks for clarification, but I do not understand this. Using a different type that does not match the topic's schema will throw an exception, right? Why's that not enough? Should we extend the test and add this as a test assertion too?

@SebastienDegodez
Copy link
Contributor Author

SebastienDegodez commented Jan 13, 2025

The success does not validate really the correctly linked registry schema.

Thanks for clarification, but I do not understand this. Using a different type that does not match the topic's schema will throw an exception, right? Why's that not enough? Should we extend the test and add this as a test assertion too?

I think your test still shows that it works, the contract is stored in kafka by the SR (in topic). As you find the schema when you consume messages, I think we can say that everything is good.

It's more my preference to show the interest of the SR to refuse messages with a non-compliant schema. Your approach works too.

@HofmeisterAn HofmeisterAn merged commit f1b2e0b into testcontainers:develop Jan 14, 2025
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants