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.

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 tests to be run inside a docker container #141

Closed
wants to merge 2 commits into from

Conversation

wngr
Copy link

@wngr wngr commented Dec 13, 2019

This adds the ip_address method to Docker trait in order to to query the ip address of the spawned container. This is derived from a docker inspect call.

This also introduces a new usage pattern when used from the host system:
Rather than relying on a port mapping to the host, the presumably known target port can be directly queried by using the ip address of the container in the internal docker network.
Let me know whether this actually breaks any workflows I didn't think of.
Note: This relies on the fact, that the default docker bridge network is configured for all involved containers.

I looked into shiplift to create a new Client impl, but refrained from doing so, as the current API is completely sync, and it felt rather heavy pulling in shiplift w/ a runtime. However, I'm very much interested in a follow-up discussion, where this crate could offer an additional async API, or a way forward with the sync API.
Futhermore, with the approach presented in this PR, the orchestrating container needs to have the docker cli available. When adding more and more features, it might make sense eventually to use shiplift to interface with the docker daemon, and by this also get rid of this requirement..

Closes #129
Related #139

I also bumped the version in .travis.yml #140.

This adds the `ip_address` method to `Docker` trait in order to
to query the ip address of the spawned container. This is derived
from a `docker inspect` call.

This also introduces a new usage pattern when used from the host system:
Rather than relying on a port mapping to the host, the target port
can be directly queried by using the ip address of the container in the
internal docker network.
Note: This relies on the fact, that the default docker bridge network is
configured for all involved containers.

testcontainers#129
ports
} else {
ports
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question, whether this API should be changed in any way.

&& image.volumes().into_iter().collect::<Vec<_>>().len() > 0
{
// TODO: Panic here or just log and continue?
panic!("Running inside Docker: Unable to map volumes.");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should this be handled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very restrictive. It is still possible to map volumes if they mapped correctly

/// default docker bridge network.
fn ip_address(&self, id: &str) -> String {
let info = self.get_container_info(id);
info.network_settings.ip_address
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if container is connected to a network that is different from the tests' one, you will not be able to use this IP.

This is why in testcontainers-java we always connect to the gateway's IP

@bsideup
Copy link
Member

bsideup commented Dec 15, 2019

Rather than relying on a port mapping to the host, the presumably known target port can be directly queried by using the ip address of the container in the internal docker network.
Let me know whether this actually breaks any workflows I didn't think of.

From testcontainers-java experience I can say that this breaks in many environments. This is why in the previous PR ( #139 ) we did it differently (but the way it works in every case)

@bsideup
Copy link
Member

bsideup commented Dec 15, 2019

Also, I am a bit disappointed that you decided not not listen to the feedback based on testcontainers-java and will let you discover all the fun yourself :)

@thomaseizinger
Copy link
Collaborator

From testcontainers-java experience I can say that this breaks in many environments. This is why in the previous PR ( #139 ) we did it differently (but the way it works in every case)

@bsideup are you only talking about not using the port mapping or also determining the ip address via docker inspect?

@thomaseizinger
Copy link
Collaborator

Also, I am a bit disappointed that you decided not not listen to the feedback based on testcontainers-java and will let you discover all the fun yourself :)

Just to clarify, I am very open to allow testcontainers-rs to become as mature as testcontainers-java! My main concern in #139 was purely implementation-wise.

If the approach in #139 is superior, maybe we can take the best of both worlds and implement #139 with a design similar to this PR?

I am unfortunately not too familiar with the internals of docker, hence I don't exactly know the constraints we are facing here. Would it be too much to ask to create a structured write-up of what our constraints are for implementing this?

@thomaseizinger
Copy link
Collaborator

I looked into shiplift to create a new Client impl, but refrained from doing so, as the current API is completely sync, and it felt rather heavy pulling in shiplift w/ a runtime. However, I'm very much interested in a follow-up discussion, where this crate could offer an additional async API, or a way forward with the sync API.

@wngr That is a good point! Now that async/await is maturing in Rust and we have things like #[tokio::test] and #[async_std::test], we can at some point migrate the API to be async which then should allow us to fairly easily implement a client on top of shiplift. There is also https://github.com/fussybeaver/bollard btw.

@wngr
Copy link
Author

wngr commented Jan 15, 2020

Sorry for being passive on this PR, vacation days took its toll on me :-)..


@bsideup

Also, I am a bit disappointed that you decided not not listen to the feedback based on testcontainers-java and will let you discover all the fun yourself :)

I'm honestly confused now, whether #139 and the PR presented here are actually about the same thing. The scenario I want to realize with the proposed approach is to be able to run the actual testing code inside a docker container itself. Thus, I have to get the container's IP address. You rightfully noted, that this assumes that both containers are on the same network (TODO!).
I'd greatly appreciate if you could elaborate on what you mean by ..

This is why in testcontainers-java we always connect to the gateway's IP
.. and how the scenario I described above can be done in a more robust way.

@wngr wngr mentioned this pull request Jan 15, 2020
@rkudryashov
Copy link

I have just encountered an issue that I think is related to this thread:

---- test_sign_in stdout ----
thread 'test_sign_in' panicked at 'Failed to execute docker command: Os { code: 2, kind: NotFound, message: "No such file or directory" }', /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/testcontainers-0.9.1/src/clients/cli.rs:102:21
Panic in Arbiter thread.

It would be great to make this working! Are there any news on the PR?

@thomaseizinger
Copy link
Collaborator

It would be great to make this working! Are there any news on the PR?

Nothing other than what you can see on this thread unfortunately :)

From my perspective, I think efforts stalled mainly due to confusion of what usecases we actually want to support 😬

Like I said above, it would be very helpful if we could have a write-up of which different usecases we want to solve and what the traps around this are. @bsideup mentioned that they discovered several of those in testcontainers-java :)

@mergify
Copy link

mergify bot commented Sep 30, 2020

This repository is using GitFlow. New features should be merged into dev instead of master. Please open a new pull request that targets dev instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to run tests inside a docker container themselves
4 participants