Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4561 +/- ##
=======================================
Coverage 77.24% 77.24%
=======================================
Files 88 88
Lines 3002 3002
=======================================
Hits 2319 2319
Misses 599 599
Partials 84 84
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Improves Kafka integration test isolation from the host environment by removing hard-coded Docker container names, hostnames, port bindings, and the custom test network.
Changes:
- Removed fixed
dockertest.WithName/dockertest.WithHostnameusage for Zookeeper/Kafka containers. - Removed fixed host port bindings and switched to deriving connection addresses at runtime.
- Dropped the custom Docker network and added helper logic to read container IPs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| instrumentation/github.com/confluentinc/confluent-kafka-go/v2/kafka/splunkkafka/test/kafka_test.go | Makes Kafka v2 integration tests use dynamically-derived container endpoints instead of fixed names/ports/networks. |
| instrumentation/github.com/confluentinc/confluent-kafka-go/kafka/splunkkafka/test/kafka_test.go | Applies the same isolation changes to the deprecated (non-v2) Kafka instrumentation tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MrAlias
left a comment
There was a problem hiding this comment.
This change looks cleaner from an isolation standpoint, but I want to check whether you intended to narrow the environments these tests can run in.
Both Kafka test variants now advertise and connect to the broker via the container IP (containerIP(...):9092) instead of a host-reachable published port. That works when the test process can route directly to Docker’s bridge subnet, but it stops working with remote DOCKER_HOST setups, Docker Desktop-style VM networking, and other engines where container IPs are not reachable from the host test process.
Do you understand and accept that consequence? If these tests are only meant to run against a local Linux daemon, that seems consistent with the current CI signal. If not, I think we should keep using a host-reachable endpoint for the client/bootstrap path rather than relying on container IP reachability.
|
Regarding Docker Desktop-style VM networking, I thought that container IPs are not reachable, but I may be completely wrong. I wonder if the current setup on |
Fixes #4537
The following should make the test better isolated from the host env:
dockertest.WithNamevalues from Kafka integration test containersdockertest.WithHostnamesvalues from Kafka integration test containersdockertest.WithPortBindingsvalues from Kafka integration test containersThis helped in making the changes: https://www.confluent.io/blog/kafka-listeners-explained/
I think creating a test network is an overkill: