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

Fix kafka internal docker connection #1

Merged
merged 23 commits into from
Sep 17, 2024

Conversation

mdelapenya
Copy link

Hey @catinapoke, I'm sending a PR to your main branch (the one you used to send the original PR).

What does this PR do?

This PR is merging the current state of main with the branch, adding commits for:

  • return back to PLAINTEXT/BROKER, instead of EXTERNAL/INTERNAL. This is caused because there could be expectations on current users on those names. But I'd be open to a rename in a separate PR if you consider it important. We can then isolate that breaking change from the current bug fix.
  • updates usage of deprecated RunContainer, with Run. Same for network creation APIs.
  • resolve some lint errors, such as error handling, indentation and sarama client closing.
  • refactors a test using subtests instead of different blocks in the same test. I think this improves readability.
  • adds a test using kcat, shadowing what the redpanda module does. I left the test using initKafkaTest untouched on purpose, I hope you can work on that replacing the current logic with the kcat example. 🙏

I think there are other tests needing the kcat

Why is it important?

Unblock testcontainers#2490

Related issues

mdelapenya and others added 23 commits August 27, 2024 10:33
* Fix trailing slash on Image Prefix

* Apply Lint fix
…ainers#2753)

* Upgrade milvus-io/milvus-sdk-go to avoid checksum mismatch.

* Update go.sum

* Update indirect dependencies.
)

* fix: check if the discovered docker socket responds

* fix: update tests

* chore: add test

* Revert "chore: add test"

This reverts commit c6c4832.

* Revert "fix: update tests"

This reverts commit fbadada.

* Revert "fix: check if the discovered docker socket responds"

This reverts commit 19fb55b.

* chore: support passing callback checks to the docker host/socket path resolution

This way the tests are able to verify if the socket/host is reachable by calling a mock client.

The production code will use the default callback check, which calls a vanilla docker client using the discovered host as host

* chore: convert var into function

* chore: mock callback check instead

* chore: simplify

* chore: raise error when extracting the docker host

* docs: document that the extract functions panics

* chore: simplify error handler

* chore: use require.Panics

* fix: remove duplicated case

* fix: negotiate API version in the plain docker client call

* fix: defer closing the client earlier

* chore: better function name

* chore: convert vars into functions

* chore: no need to assert as panic should occur

* chor: rename check function

* chore: pass ctx to new function

* chore: more exhaustive error check in tests

* docs: typo

* fix: update usage
…estcontainers#2760)

Bumps [mkdocs-include-markdown-plugin](https://github.com/mondeja/mkdocs-include-markdown-plugin) from 6.0.4 to 6.2.2.
- [Release notes](https://github.com/mondeja/mkdocs-include-markdown-plugin/releases)
- [Commits](mondeja/mkdocs-include-markdown-plugin@v6.0.4...v6.2.2)

---
updated-dependencies:
- dependency-name: mkdocs-include-markdown-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…s#2762)

Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.1 to 4.1.7.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@b4ffde6...692973e)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…testcontainers#2761)

Bumps [mkdocs-markdownextradata-plugin](https://github.com/rosscdh/mkdocs-markdownextradata-plugin) from 0.2.5 to 0.2.6.
- [Release notes](https://github.com/rosscdh/mkdocs-markdownextradata-plugin/releases)
- [Commits](rosscdh/mkdocs-markdownextradata-plugin@0.2.5...0.2.6)

---
updated-dependencies:
- dependency-name: mkdocs-markdownextradata-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This commit allows users of the compose module to selectively enable services
by using Docker Compose profiles.

More about profiles: https://docs.docker.com/compose/profiles
* feat(wait): for file

Add the ability to wait for a file and optionally its contents.

This leverages generated mocks using mockery and testify/mock to make
testing easier.

* debugging: rate limit config dump

Add output of the docker config if we hit rate limiting to aid in
debugging why this is happening on github actions.

Don't use logger as that's no-op when verbose is not in effect.

* docs: list the file wait strategy in docs

---------

Co-authored-by: Manuel de la Peña <[email protected]>
* docs: refine heading badges in README

* docs: consistency across langs
* main:
  docs: refine heading badges in README (testcontainers#2770)
  feat(wait): for file (testcontainers#2731)
  feat(compose): select services via profiles (testcontainers#2758)
  chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (testcontainers#2761)
  fix: update template too (testcontainers#2763)
  chore(deps): bump actions/checkout from 4.1.1 to 4.1.7 (testcontainers#2762)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (testcontainers#2760)
  fix: check if the discovered docker socket responds (testcontainers#2741)
  Upgrade milvus-io/milvus-sdk-go to avoid checksum mismatch. (testcontainers#2753)
  Fix trailing slash on Image Prefix (testcontainers#2747)
  chore: use new testcontainers/ryuk:0.9.0 image (testcontainers#2750)
@catinapoke
Copy link
Owner

LGTM
So things is left - TestKafka_restProxyService and TestKafka_networkConnectivity (remake to kcat by example)

@mdelapenya
Copy link
Author

@catinapoke how do you want to proceed with this? Merge into your branch and then add kcat to those tests?

@catinapoke catinapoke marked this pull request as ready for review September 17, 2024 15:45
@catinapoke catinapoke merged commit ba701e1 into catinapoke:main Sep 17, 2024
105 of 108 checks passed
@catinapoke
Copy link
Owner

Yes, going to redo test with kcat

@catinapoke
Copy link
Owner

Btw, have you run tests? I see my tests fail now at basic stuff :/

@catinapoke
Copy link
Owner

catinapoke commented Sep 17, 2024

uhm, yea, I see in Github tests details

2024/09/09 18:10:15 🐳 Starting container: c5830d4ef694
2024/09/09 18:10:20 ✅ Container started: c5830d4ef694
2024/09/09 18:10:20 🔔 Container is ready: c5830d4ef694
kafka_test.go:142:
Error Trace: /home/runner/work/testcontainers-go/testcontainers-go/modules/kafka/kafka_test.go:142
Error: Received unexpected error:
failed to create topics: kafka: broker not connected
Test: TestKafka_networkConnectivity

=== FAIL: . TestTrimValidateListeners/testcontainers#3 (re-run 5) (0.00s)
kafka_helpers_test.go:214: expected to fail due to reserved listener name PLAINTEXT duplication
--- FAIL: TestTrimValidateListeners/testcontainers#3 (0.00s)

=== FAIL: . TestTrimValidateListeners (re-run 5) (0.00s)

=== FAIL: . TestKafka (re-run 5) (5.27s)
2024/09/09 18:10:21 github.com/testcontainers/testcontainers-go - Connected to docker:
Server Version: 26.1.3
API Version: 1.45
Operating System: Ubuntu 22.04.4 LTS
Total Memory: 15981 MB
Testcontainers for Go Version: v0.34.0
Resolved Docker Host: unix:///var/run/docker.sock
Resolved Docker Socket Path: /var/run/docker.sock
Test SessionID: 16bd96c67741897b2ff88f00d299db0b40d32d12a20ead0e88649d9c5b46481c
Test ProcessID: 5486db9c-3b38-476f-bb8d-fd77d4e4b4c0
2024/09/09 18:10:21 🐳 Creating container for image testcontainers/ryuk:0.9.0
2024/09/09 18:10:21 ✅ Container created: 94bfd7836393
2024/09/09 18:10:21 🐳 Starting container: 94bfd7836393
2024/09/09 18:10:21 ✅ Container started: 94bfd7836393
2024/09/09 18:10:21 ⏳ Waiting for container id 94bfd7836393 image: testcontainers/ryuk:0.9.0. Waiting for: &{Port:8080/tcp timeout: PollInterval:100ms skipInternalCheck:false}
2024/09/09 18:10:21 🔔 Container is ready: 94bfd7836393

@catinapoke
Copy link
Owner

catinapoke commented Sep 17, 2024

@mdelapenya looks like something has been broken after your changes (or before your and after mine, I dunno)

upd. fixed basic test for kafka, but network one is still broken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants