Skip to content

feat: Add Apache Kafka (KRaft) builder (#1347)#1348

Closed
IbrayevRamil wants to merge 4 commits into
testcontainers:developfrom
IbrayevRamil:feature/add-apache-kafka-kraft-support
Closed

feat: Add Apache Kafka (KRaft) builder (#1347)#1348
IbrayevRamil wants to merge 4 commits into
testcontainers:developfrom
IbrayevRamil:feature/add-apache-kafka-kraft-support

Conversation

@IbrayevRamil

@IbrayevRamil IbrayevRamil commented Jan 24, 2025

Copy link
Copy Markdown

What does this PR do?

Adds Apache Kafka (KRaft) container builder

Why is it important?

Current Kafka builder doesn't support it

Related issues

@netlify

netlify Bot commented Jan 24, 2025 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit e0c0dad
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/67988e8d56c1ad00083c8c28
😎 Deploy Preview https://deploy-preview-1348--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.

rafek1241
rafek1241 reviewed Jan 25, 2025
View reviewed changes
Comment thread src/Testcontainers.Kafka/ApacheKafkaBuilder.cs
@SukharevAndrey SukharevAndrey mentioned this pull request Jan 27, 2025
Merged
@SukharevAndrey

SukharevAndrey commented Jan 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What a coincedence. The same thought came to me last week and I took different approach in #1353.
The difference:

  1. Supports for KRaft in Confluent image too
  2. Throws an error if KRaft is not supported in provided image
  3. Does not require another class to use. All happens in the same class without breaking backward compatibility.
  4. Makes the project nullable.
  5. Tests different setups.

HofmeisterAn
HofmeisterAn reviewed Feb 18, 2025
View reviewed changes

@HofmeisterAn HofmeisterAn left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@IbrayevRamil, do you have any feedback on @SukharevAndrey's comment? Can we please consolidate both PRs?

@IbrayevRamil

IbrayevRamil commented Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

@IbrayevRamil, do you have any feedback on @SukharevAndrey's comment? Can we please consolidate both PRs?

His approach is much flexible and better all in all, so I would stick with it. Mine is just a hot fix :)
I will review his PR soon in more details and leave some comments if any

@HofmeisterAn

HofmeisterAn commented Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@IbrayevRamil, do you have any feedback on @SukharevAndrey's comment? Can we please consolidate both PRs?

His approach is much flexible and better all in all, so I would stick with it. Mine is just a hot fix :) I will review his PR soon in more details and leave some comments if any

Thanks for the quick response. I will close this PR, and we continue working on: #1353. Still thanks for your efforts and contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HofmeisterAn HofmeisterAn HofmeisterAn left review comments
@rafek1241 rafek1241 Awaiting requested review from rafek1241

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Enhancement]: Add Apache Kafka Test Container to Kafka module

4 participants

@IbrayevRamil @SukharevAndrey @HofmeisterAn @rafek1241