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.

Sign up for GitHub

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

Jump to bottom

feat: Auto detect the use of Podman from DOCKER_HOST #982

Merged
merged 6 commits into from Mar 30, 2023
Merged

feat: Auto detect the use of Podman from DOCKER_HOST #982

merged 6 commits into from Mar 30, 2023

Conversation

jtwatson
Copy link
Contributor

@jtwatson jtwatson commented Mar 24, 2023 •
edited

What does this PR do?

When running testcontainers-go with Podman, it seems that ProviderType: ProviderPodman, must always be specified.

The documentation here seems to contradict my experience.

Since Podman 4 was released with a new network stack, it appears that the default network for Podman uses the bridge driver and is named podman. As such, the library must be configured diffrently when using Podman.

This PR attempts to auto-detect when ProviderType has not been explicitly set and Podman is in use, using the following logic:

If ProviderType has not been set, and DOCKER_HOST contains podman.sock, then use ProviderPodman

Why is it important?

When attempting to use testcontainers-go with Podman, it appears that it is always the case that

	ProviderType: testcontainers.ProviderPodman,

must be set. When it is not set, the resulting error is quite difficult to troubleshoot.

testcontainers.GenericContainer(): Error response from daemon: container create: unable to find network with name or ID bridge: network not found: creating reaper failed: failed to create container

If the library can reliably detect when Podman is in use, the developer experience would be much better.

Related issues

@netlify
Copy link

netlify bot commented Mar 24, 2023 •
edited

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit cb7788f
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/64258b30a6e4b600082e1f58
😎 Deploy Preview https://deploy-preview-982--testcontainers-go.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 settings.

@mdelapenya
Copy link
Collaborator

@jtwatson at first sight, LGTM! Thanks for adding this automatic detection.

Could you update the docs for Podman describing what this PR does in a high level language? 🙏

@jtwatson jtwatson marked this pull request as ready for review March 24, 2023 06:39
@jtwatson jtwatson requested a review from a team as a code owner March 24, 2023 06:39
@jtwatson
Copy link
Contributor Author

@mdelapenya I question if the docs are accurate around when it is required to manually specify the ProviderType. I can't think of a situation where you would be using Docker and not need to define DOCKER_HOST.

It is probably good to leave it in so the usage is documented, but I wonder if the advice around when to use it should be corrected or not.

@mdelapenya
Copy link
Collaborator

@mdelapenya I question if the docs are accurate around when it is required to manually specify the ProviderType. I can't think of a situation where you would be using Docker and not need to define DOCKER_HOST.

It is probably good to leave it in so the usage is documented, but I wonder if the advice around when to use it should be corrected or not.

I'm planning to move the logic of the provider, and many other things, to the testcontainer.properties, so it's defined when bootstrapping the library, not leaving the decision to manually add it to each container request.

@jtwatson
Copy link
Contributor Author

If you would like I can add a test to validate the auto-detection based on the DOCKER_HOST value.

@jtwatson @mdelapenya
feat: Autodetect the use of Podman if no provider is set, and DOCKER_…
9e616a2 
…HOST contains podman.sock

Updated documentation
Unit tests for auto-detection of ProviderPodman

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@jtwatson jtwatson force-pushed the feature/transparent-docker-support branch from 7583569 to 9e616a2 Compare March 25, 2023 22:26
@jtwatson jtwatson changed the title Auto detect the use of Podman from DOCKER_HOST feat: Auto detect the use of Podman from DOCKER_HOST Mar 25, 2023
@jtwatson
Copy link
Contributor Author

@mdelapenya I added unit tests, squashed my PR, and updated the title as a feature. Should be ready to go.

@jtwatson jtwatson force-pushed the feature/transparent-docker-support branch from c68a105 to a42a35d Compare March 25, 2023 22:40
mdelapenya
mdelapenya reviewed Mar 25, 2023
View reviewed changes
provider_test.go Outdated Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Mar 27, 2023
@mdelapenya mdelapenya added podman Issues regarding podman. enhancement New feature or request labels Mar 27, 2023
@mdelapenya
Copy link
Collaborator

It's extremely weird that the neo4j module fails in a consistent manner on CI. I've fetched this branch locally, run the tests, and they pass 🤔

@mdelapenya
Copy link
Collaborator

It's extremely weird that the neo4j module fails in a consistent manner on CI. I've fetched this branch locally, run the tests, and they pass 🤔

For some reason that inconsistency is no longer present 🤷

@sonarcloud
Copy link

sonarcloud bot commented Mar 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
8.6% 8.6% Duplication

@mdelapenya mdelapenya merged commit b1edcd4 into testcontainers:main Mar 30, 2023
53 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Mar 30, 2023
@mdelapenya
Merge branch 'main' into redis-module
5013d50 
* main:
  feat: Auto detect the use of Podman from DOCKER_HOST (testcontainers#982)
  chore(deps): bump github.com/opencontainers/runc from 1.1.3 to 1.1.5 (testcontainers#1017)
  Revert "chore: render logos from the base location (testcontainers#995)" (testcontainers#998)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Apr 3, 2023
@jtwatson @mdelapenya
feat: Auto detect the use of Podman from DOCKER_HOST (testcontainers#982
c0f7111 
)

* feat: Autodetect the use of Podman if no provider is set, and DOCKER_HOST contains podman.sock

Updated documentation
Unit tests for auto-detection of ProviderPodman

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

* Cleanup linter issues

* Use `t.Setenv()` in tests

* Fix the spelling of reap

---------

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Co-authored-by: Manuel de la Peña <mdelapenya@gmail.com>
@austince austince mentioned this pull request Dec 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristianrgreco cristianrgreco cristianrgreco left review comments

@mdelapenya mdelapenya mdelapenya approved these changes

Assignees

@mdelapenya mdelapenya

Labels
enhancement New feature or request podman Issues regarding podman.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: GenericContainer fails when used with podman, unless SkipReaper is true
3 participants
@jtwatson @mdelapenya @cristianrgreco