feat: add the ability to configure the provider type in the properties file or as an env var#3494
feat: add the ability to configure the provider type in the properties file or as an env var#3494mkeeler wants to merge 5 commits intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a public Config.Provider field (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
2 times, most recently
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.
|
Both the string normalization and octal specifying issue have been fixed. |
… 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
7d85fd3 to
31c89db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
provider.go (1)
130-131:ProviderDefaultcase is now unreachable.
UnderlyingProviderType()never returnsProviderDefault— it always resolves toProviderDockerorProviderPodman. TheProviderDefaultcase on line 131 is dead code. It's harmless since it's grouped withProviderDocker, but you could remove it for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider.go` around lines 130 - 131, The switch in the provider selection checks t.UnderlyingProviderType() and includes the now-unreachable ProviderDefault constant; remove ProviderDefault from the case group so only ProviderDocker (and other real values) are matched (i.e., change the case list that currently contains ProviderDefault, ProviderDocker to just ProviderDocker), and run/update any tests or references involving ProviderDefault to ensure no other code relies on that constant in provider selection; keep references to UnderlyingProviderType() and ProviderPodman unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider_test.go`:
- Around line 90-103: The test harness in provider_test.go doesn't ensure
TESTCONTAINERS_PROVIDER is cleared when a subtest's tt.env doesn't set it, so an
existing environment value can leak into tests; inside the test loop (the for _,
tt := range tests { ... } block where you set tmpDir and other envs) add logic
to explicitly unset TESTCONTAINERS_PROVIDER when it's not present in tt.env
(e.g., check if _, ok := tt.env["TESTCONTAINERS_PROVIDER"]; !ok {
os.Unsetenv("TESTCONTAINERS_PROVIDER") } or use t.Setenv to clear), so each
subtest runs with the intended environment.
---
Duplicate comments:
In `@provider.go`:
- Line 111: Update the comment "Attempt to auto-detect Podman from the the
Docker configuration" to correct the typo by removing the duplicate "the" so it
reads "Attempt to auto-detect Podman from the Docker configuration"; locate and
edit the comment in provider.go near the Podman/Docker auto-detection logic.
---
Nitpick comments:
In `@provider.go`:
- Around line 130-131: The switch in the provider selection checks
t.UnderlyingProviderType() and includes the now-unreachable ProviderDefault
constant; remove ProviderDefault from the case group so only ProviderDocker (and
other real values) are matched (i.e., change the case list that currently
contains ProviderDefault, ProviderDocker to just ProviderDocker), and run/update
any tests or references involving ProviderDefault to ensure no other code relies
on that constant in provider selection; keep references to
UnderlyingProviderType() and ProviderPodman unchanged.
|
@mdelapenya I think I fixed all the issues and this should be ready. |
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. Additionally, the currentWithProvidersolution doesn't work in all cases such as when attempting to use thegithub.com/testcontainers/testcontainers-go/networkpackage (due to their not being a correspondingWithProviderNetworkCustomizer).Related issues
How to test this PR
WithProvideroption.TESTCONTAINERS_PROVIDER=podmanenv var.provider=podmanto the properties file.