-
Notifications
You must be signed in to change notification settings - Fork 160
feat: allow TESTCONTAINERS_HOST_OVERRIDE to override docker host #815
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for testcontainers-rust ready!
To edit notification comments on pull requests, go to your Netlify project 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.
Thank you for the contribution!
// allow TESTCONTAINERS_HOST_OVERRIDE to override the docker host | ||
if let Some(host_override) = | ||
<env::Os as env::GetEnvValue>::get_env_value("TESTCONTAINERS_HOST_OVERRIDE") | ||
{ | ||
return url::Host::parse(&host_override) | ||
.map_err(|_| ConfigurationError::InvalidDockerHost(host_override).into()); | ||
} |
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.
There is more appropriate place for this - it should be part of the config
Moreover config already has tc_host
which is prioritized, but we read it only from properties file.
What we can do is to extend this part and read tc_host from env variables too
testcontainers-rs/testcontainers/src/core/env/config.rs
Lines 103 to 119 in d652b38
async fn load_from_env_config<E>() -> Result<Self, ConfigurationError> | |
where | |
E: GetEnvValue, | |
{ | |
let host = E::get_env_value("DOCKER_HOST"); | |
let tls_verify = E::get_env_value("DOCKER_TLS_VERIFY").map(|v| v == "1"); | |
let cert_path = E::get_env_value("DOCKER_CERT_PATH").map(PathBuf::from); | |
let command = E::get_env_value("TESTCONTAINERS_COMMAND") | |
.filter(|v| !v.trim().is_empty()) | |
.map(|v| v.parse()) | |
.transpose()?; | |
let docker_auth_config = read_docker_auth_config::<E>().await; | |
Ok(Config { | |
host, | |
tc_host: None, |
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.
Thanks for the feedback @DDtKey ! The issue here is that tc_host
and docker_host()
are used to resolve the Docker host for both connecting to Docker socket/server and to resolve URLs for containers. For example in the case of Docker on Mac, Docker socket is correctly resolved but we need the host for container addresses to be resolved as host.docker.internal
instead of localhost
. So I think those 2 variables need to be separated?
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'm not sure I got the issue you're trying to explain. tc.host
exists to allow overriding the host for testcontainers only.
But main driver of my suggestion is that tc_host
behaves this way in other languages, e.g in go:
// TestcontainersHost is the address of the Testcontainers host.
//
// Environment variable: TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE
TestcontainersHost string `properties:"tc.host,default="`
It's a value either from TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE
env variable or tc.host
property
This PR adds the option to customize Docker hostname detection with the
TESTCONTAINERS_HOST_OVERRIDE
environment variable.It replicates the behavior of the Java testcontainer package (https://java.testcontainers.org/features/configuration/)