feat: add the ability to configure the provider type in the properties file or as an env var #3494
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds Config.Provider (bound to Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Resolver as ProviderType.UnderlyingProviderType()
participant ENV as Environment (TESTCONTAINERS_PROVIDER)
participant CFG as Config (.testcontainers.properties / provider)
participant AUTO as Auto-detect (host/docker)
Client->>Resolver: Request effective provider
rect rgba(200,225,255,0.5)
Resolver->>Resolver: Check explicit ProviderType argument
alt explicit provided
Resolver-->>Client: Return explicit provider
end
end
rect rgba(220,255,200,0.5)
Resolver->>ENV: Check TESTCONTAINERS_PROVIDER
alt env set
Resolver-->>Client: Return provider from ENV
else
Resolver->>CFG: Check Config.Provider
alt cfg set
Resolver-->>Client: Return provider from CFG
end
end
end
rect rgba(255,240,200,0.5)
Resolver->>AUTO: Auto-detect (e.g., inspect Docker host for Podman)
alt auto-detect Podman
Resolver-->>Client: Return Podman
else
Resolver-->>Client: Return Docker (default)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mkeeler
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
provider.go (1)
104-109: Consider case-insensitive provider matching.The provider string matching is case-sensitive, which may lead to configuration errors if users specify "Docker" or "Podman" with capital letters.
Consider making the comparison case-insensitive:
conf := config.Read() - switch conf.Provider { - case "docker": + switch strings.ToLower(conf.Provider) { + case "docker": return ProviderDocker - case "podman": + case "podman": return ProviderPodman }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/config/config.go(2 hunks)internal/config/config_test.go(5 hunks)provider.go(2 hunks)provider_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
provider.go (2)
internal/config/config.go (1)
Read(99-105)internal/core/docker_host.go (1)
MustExtractDockerHost(87-98)
provider_test.go (2)
provider.go (4)
ProviderType(23-23)ProviderDocker(17-17)ProviderPodman(18-18)ProviderDefault(16-16)internal/config/config.go (1)
Reset(111-113)
internal/config/config_test.go (1)
internal/config/config.go (2)
Read(99-105)Config(32-93)
🔇 Additional comments (6)
provider.go (1)
96-118: LGTM! Well-structured precedence logic.The
UnderlyingProviderType()method correctly implements the precedence rules: explicit code setting → environment variable → properties file → auto-detection → default. The implementation cleanly centralizes provider resolution logic.internal/config/config.go (2)
89-92: LGTM! Clear documentation and proper field definition.The
Providerfield is well-documented with the environment variable name and example values, following the same pattern as other configuration fields in the struct.
149-152: LGTM! Consistent with other environment variable handling.The environment variable reading for
TESTCONTAINERS_PROVIDERfollows the same pattern as other configuration fields and correctly applies the precedence (environment variables override properties file values).internal/config/config_test.go (2)
29-29: LGTM! Proper test environment cleanup.Adding
TESTCONTAINERS_PROVIDERto the environment reset ensures clean test state between test runs.
524-557: LGTM! Comprehensive test coverage for Provider configuration.The three new test cases properly validate:
- Provider set via properties file alone
- Provider set via environment variable alone
- Environment variable precedence over properties file
This aligns with the expected configuration precedence behavior.
provider_test.go (1)
15-120: LGTM! Thorough test coverage for UnderlyingProviderType.The test function comprehensively validates the provider resolution logic across all precedence levels:
- Explicit provider type always wins (lines 23-31, 71-87)
- Environment variable overrides properties file (lines 62-68)
- Properties file is respected when no env var is set (lines 34-43)
The test setup properly isolates each case with
config.Reset()and temporary directories, ensuring clean state between runs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/system_requirements/using_podman.md(1 hunks)provider.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider.go (2)
internal/config/config.go (1)
Read(99-105)internal/core/docker_host.go (1)
MustExtractDockerHost(87-98)
🔇 Additional comments (3)
docs/system_requirements/using_podman.md (1)
85-86: LGTM! Clear documentation of the new configuration option.The documentation accurately describes the new feature, providing both the environment variable and properties file approaches as alternatives to per-test provider configuration.
provider.go (2)
96-118: Well-structured provider resolution with clear precedence.The implementation correctly follows the documented precedence: explicit code provider → configuration → auto-detection → default. The logic is clean and the comments clearly explain each step.
130-130: LGTM! Proper delegation to the new resolution logic.The change correctly delegates to
UnderlyingProviderType()to apply the full precedence chain before selecting the appropriate provider implementation.
|
Hi @mkeeler thanks for opening this PR. We are working on moving the core of this library to use https://github.com/docker/go-sdk so that the selection of the container runtime (the provider) will be managed by the Docker config file and its current docker context. So selecting Podman, Orbstack or Colima would be just a matter of changing the current context. We have examples here: https://github.com/mdelapenya/go-sdk-examples In any case, for an initial shallow review, I think this PR is harmless, although I need to start a more in-depth one, just wanted to give you feedback the soonest. |
|
@mdelapenya Thanks for the update. Using docker contexts would definitely be better. If moving to the docker/go-sdk is something that would land relatively soon then I see no reason to review and merge this PR. If it will take a while longer, then this does solve the test portability issue for now. |
mkeeler
force-pushed
the
provider-config
branch
from
1c2bf2a to
9804605
Compare
|
@mdelapenya Has there been any progress (and is there an ETA) on the docker context work? If not, then I would consider reviewing/merging this PR. In addition to the original issue of test portability, I discovered that its impossible to use the |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
@mkeeler could you resolve the lint issue? Just by running make lint from the root would do it. Also, I think one of the robot's comment is valid, se please address it for continuing with the merge.
Other than that, I think this code is harmless until we have a final implementation based on docker/go-sdk
mkeeler
force-pushed
the
provider-config
branch
from
9804605 to
3237483
Compare
… or as an env var Using WithProvider may not be portable when some developers running tests may be using Podman and others using Docker. Allowing setting the Provider type in the global configuration or environment ensures that the provider is a per-developer configurable setting.
mkeeler
force-pushed
the
provider-config
branch
from
3237483 to
c0e1edf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@provider.go`:
- Around line 96-118: The auto-detect path in
ProviderType.UnderlyingProviderType calls core.MustExtractDockerHost which
panics when no host is configured; replace that call with a non-panicking
variant (or call core.ExtractDockerHost(context.Background()) and check its
error) and only run strings.Contains on the host if no error was returned; if
host resolution fails, skip the Podman auto-detect and fall back to returning
ProviderDocker so provider resolution is safe in config-only scenarios.
| func (t ProviderType) UnderlyingProviderType() ProviderType { | ||
| // Provider set within code has precedence over all others | ||
| if t != ProviderDefault { | ||
| return t | ||
| } | ||
|
|
||
| // Configuration of an explicit provider has the next priority | ||
| conf := config.Read() | ||
| switch strings.ToLower(conf.Provider) { | ||
| case "docker": | ||
| return ProviderDocker | ||
| case "podman": | ||
| return ProviderPodman | ||
| } | ||
|
|
||
| // Attempt to auto-detect Podman from the the Docker configuration | ||
| if strings.Contains(core.MustExtractDockerHost(context.Background()), "podman.sock") { | ||
| return ProviderPodman | ||
| } | ||
|
|
||
| // When all else fails, default to Docker | ||
| return ProviderDocker | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the MustExtractDockerHost implementation
find . -name "docker_host.go" -type f | head -5Repository: testcontainers/testcontainers-go
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Search for MustExtractDockerHost implementation
rg -n "func.*MustExtractDockerHost" -A 20Repository: testcontainers/testcontainers-go
Length of output: 1669
🏁 Script executed:
#!/bin/bash
# Also look for extractDockerHost (the underlying function)
rg -n "extractDockerHost|MustExtractDockerHost" -B 2 -A 15Repository: testcontainers/testcontainers-go
Length of output: 22408
MustExtractDockerHost will panic if no Docker/Podman host is configured.
The auto-detect path at line 112 calls core.MustExtractDockerHost(context.Background()), which is documented to panic when the Docker host cannot be discovered (see internal/core/docker_host.go:86). Simply resolving the provider type will crash if no Docker/Podman infrastructure is available, even before attempting container operations.
Consider using a non-panicking alternative or adding error handling to make provider resolution more resilient in config-only scenarios.
🤖 Prompt for AI Agents
In `@provider.go` around lines 96 - 118, The auto-detect path in
ProviderType.UnderlyingProviderType calls core.MustExtractDockerHost which
panics when no host is configured; replace that call with a non-panicking
variant (or call core.ExtractDockerHost(context.Background()) and check its
error) and only run strings.Contains on the host if no error was returned; if
host resolution fails, skip the Podman auto-detect and fall back to returning
ProviderDocker so provider resolution is safe in config-only scenarios.
There was a problem hiding this comment.
Other existing places use MustExtractDockerHost and so by time this can be called it will have already paniced. If this needs to be changed it should be changed in all places, including potentially with removing the MustExtractDockerHost function all together and replacing with a function which propagates the error (while maintaining the do once behavior).
I see this as outside the scope of this PR.
There was a problem hiding this comment.
@mkeeler, that's a fair point—you're right that MustExtractDockerHost is already used throughout the codebase, so this PR isn't introducing a new risk. A broader refactoring to propagate errors instead of panicking would indeed be outside the scope of this change. I'll leave this as-is. 👍
|
Both the string normalization and octal specifying issue have been fixed. |
2 participants
What does this PR do?
This PR allows for setting the Provider type (Podman or Docker) in either the properties file (e.g.
provider=podman) or as an environment variable (e.g.TESTCONTAINERS_PROVIDER=docker)Why is it important?
Using
WithProviderin tests results in tests that are not portable to other developer environments when not all contributors to a codebase all use the same container engine. This PR allows for user/environment settings to control the provider type ensuring that tests remain portable.Related issues
How to test this PR
WithProvideroption.TESTCONTAINERS_PROVIDER=podmanenv var.provider=podmanto the properties file.