-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
Allow use reuse containers in parrallel packages tests #1572
Allow use reuse containers in parrallel packages tests #1572
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prongq thanks for working on this PR, I understand the use case and I see the potential value of it. If you mind, I'm postponing a final review until #1513 is merged (hopefully this week), as it comes with some synchronisation regarding the reaper container when running in the same test session (go test ./...), to be reused. I think it's very related to your work here.
In the mean time, it apparently looks good to me. Need to double check locally
f099a52
to
81e5b11
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments before approving it.
FYI we are going to start an internal discussion around reusable containers, so in a short future, we could be visiting this code again for a refactor depending on the new design for reusable containers.
Other than that, LGTM, thanks!
81e5b11
to
9016c56
Compare
Hello @mdelapenya. Could you take a look at the pr please. |
Sure, let me do it tomorrow my morning. Sorry for the delay 🙏 conference stuff past weeks... |
9016c56
to
e423a11
Compare
@prongq for some reason the helper test is not skipped. Could you please take a look at the errors in https://github.com/testcontainers/testcontainers-go/actions/runs/6466407444/job/17556657245?pr=1572? |
f93cd0b
to
b7b44c6
Compare
@mdelapenya Hello. The test shouldn't have been skipped. I don't understand why this error is happening. I have not been able to reproduce it locally. I think it has relation to keep-alive and I turned it off in the last commit. Could you please allow the tests to run. Sorry for the long response, not enough time because of work |
b7b44c6
to
c941f6e
Compare
I think the keep-alive problem is solved. I also removed the check that the same container is used with reuse, because tests can run for a long time and reaper can kill the container in the process. |
@mdelapenya can you run the tests, please? |
* main: (22 commits) feat: Allow the container working directory to be specified (testcontainers#1899) chore: make rabbitmq examples more readable (testcontainers#1905) chore(deps): bump github.com/twmb/franz-go and github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1896) Fix - respect ContainerCustomizer in neo4j module (testcontainers#1903) chore(deps): bump github.com/nats-io/nkeys and github.com/nats-io/nats.go in /modules/nats (testcontainers#1897) chore: add tests for withNetwork option (testcontainers#1894) chore(deps): bump google.golang.org/grpc and cloud.google.com/go/firestore in /modules/gcloud (testcontainers#1891) chore(deps): bump github.com/aws/aws-sdk-go and github.com/aws/aws-sdk-go-v2/config in /modules/localstack (testcontainers#1892) chore(deps): bump Github actions (testcontainers#1890) chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.9 to 3.23.10 (testcontainers#1858) chore(deps): bump github.com/hashicorp/consul/api in /examples/consul (testcontainers#1863) chore(deps): bump github.com/IBM/sarama in /modules/kafka (testcontainers#1874) chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1861) chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#1867) chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1869) chore(deps): bump github.com/Shopify/toxiproxy/v2 in /examples/toxiproxy (testcontainers#1882) chore(deps): bump github.com/elastic/go-elasticsearch/v8 (testcontainers#1864) chore(deps): bump mkdocs-include-markdown-plugin from 6.0.1 to 6.0.3 (testcontainers#1860) chore(deps): bump k8s.io/client-go from 0.28.2 to 0.28.3 in /modules/k3s (testcontainers#1889) feat: expose a WithNetwork functional option (testcontainers#1887) ...
What does this PR do?
When creating reusable container from different test binaries in parallel, you can get "409 - name conflict" from docker daemon, because different binaries don't have a shared mutex from #593.
Instead of exiting with error when receiving "409 - name conflict", we try to wait for the container creation to complete using the
ContainerList
methodWhy is it important?
This PR allows a reusable container to be used in parallel test binaries run from the "go test ./..." command.
Related issues