Use shiplift HTTP Docker client library instead of CLI #216

ywalterh · 2020-09-30T23:30:34Z

Followed the discussion of issue #204

Added shiplift library implementing basic Docker trait
Make shiplift implementation feature equivilent
Replace Cli in library and pass all test
Figure out Drop pattern
Implement GenericImageAsync
Implement a canned image in Async trait
Add more integration test to shiplift client

Worklog:

Creating and running container working, using tokio::runtime to get around the fact tha shiplift is mostly async
Created DockerAsync trait and all associated type in async
Figured out how to handle logging in async.
Use futures::executor::block_on to run Drop rm

Creating and running container working, using tokio::runtime to get around the fact tha shiplift is mostly async

ywalterh · 2020-09-30T23:34:34Z

Hey @thomaseizinger , shiplift library is pretty good so far. (At least the git repo is, 0.6.0 is pretty outdated)

I have a directional question: shiplift is async by nature and testcontainers-rs aren't doing that yet. Do you think I should start making the Docker trait async or use tokio::runtime to hack around it to emulate what Cli module is doing right now.

I attached some WIP code, the Docker trait is very helpful.

thomaseizinger · 2020-10-05T21:44:57Z

I have a directional question: shiplift is async by nature and testcontainers-rs aren't doing that yet. Do you think I should start making the Docker trait async or use tokio::runtime to hack around it to emulate what Cli module is doing right now.

Yes, I think making the trait async is the way to go. The migration of existing code while certainly possible would be quite tedious.

Maybe we can start with adding a DockerAsync trait that mimics the current interface but has all its functions async? Users could then choose which trait to import and we can deprecate the old one eventually (or maybe just leave it).

For async-traits, you will need https://docs.rs/async-trait/0.1.41/async_trait/.

thomaseizinger · 2020-10-05T21:46:29Z

Also, thanks for picking this up, I am excited to land this :)

ywalterh · 2020-10-05T22:13:00Z

I have a directional question: shiplift is async by nature and testcontainers-rs aren't doing that yet. Do you think I should start making the Docker trait async or use tokio::runtime to hack around it to emulate what Cli module is doing right now.

Yes, I think making the trait async is the way to go. The migration of existing code while certainly possible would be quite tedious.

Maybe we can start with adding a DockerAsync trait that mimics the current interface but has all its functions async? Users could then choose which trait to import and we can deprecate the old one eventually (or maybe just leave it).

For async-traits, you will need https://docs.rs/async-trait/0.1.41/async_trait/.

Got it, I'll get started on async trait route then. It'll save me a lot of dancing around with log handling too. I think it's a good approach.

src/clients/shiplift.rs

src/core/container_async.rs

Also fixed a lifetime error: Box by detaul assumes static

make test use tokio::test

src/core/container_async.rs

…nto dev

src/clients/shiplift.rs

tests/shiplift_client.rs

ywalterh · 2020-11-17T15:08:34Z

@thomaseizinger ready for some constructive feedback!

thomaseizinger

First, thank you so much for working on this @ywalterh! :)
Using an HTTP-based docker client is something I've envisioned from the very beginning! It is nice to see it happening now!

I've left several comments with many of them being fairly small.

There is one bigger topic regarding refactoring how wait_until_ready works. I believe it would be worthwhile try and get rid of the ImageAsync trait so that we don't have to redefine all images and they are automatically compatible with several clients.

Let me know if you are still keen to work on this! If you do, it would be good to have this refactoring as a separate commit at the beginning of this PR (or maybe even as a completely separate PR and we rebase this on top of that) so that I can easily revert the async stuff in the releases branches until shiplift cuts a release!

thomaseizinger · 2020-11-17T21:43:38Z

Cargo.toml

@@ -18,6 +18,10 @@ rand = "0.7"
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 sha2 = "0.9"
+shiplift =  {git = "https://github.com/softprops/shiplift" } 
+tokio = { version = "0.2", features = ["macros"] }
+futures = "0.3.6"


Can we reduce the dependency footprint here by disabling default features and only taking what we need?

thomaseizinger · 2020-11-17T21:43:52Z

Cargo.toml

@@ -32,4 +36,5 @@ rusoto_sqs = "0.45"
 spectral = "0.6"
 reqwest = { version = "0.10", features = ["blocking"] }
 json = "0.12"
-tokio = { version = "0.2", features = ["macros"] }
+tokio-test = "0.2.1"


I think we are not actually using this!

thomaseizinger · 2020-11-17T21:44:06Z

Cargo.toml

@@ -18,6 +18,10 @@ rand = "0.7"
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 sha2 = "0.9"
+shiplift =  {git = "https://github.com/softprops/shiplift" } 
+tokio = { version = "0.2", features = ["macros"] }


I believe we are only using tokio::test, correct? Then this can move to dev-dependencies!

thomaseizinger · 2020-11-17T21:44:49Z

src/clients/http.rs

+    // this is doing unnecessary stuff
+    let network_list_optons = NetworkListOptions::default();
+    let networks = client.networks().list(&network_list_optons).await.unwrap();
+    println!("{:?}", networks);


This should be a log statement if we want to keep it.

thomaseizinger · 2020-11-17T21:46:17Z

src/clients/http.rs

+    fn drop(&mut self) {
+        let guard = self.created_networks.read().expect("failed to lock RwLock");
+        for network in guard.iter() {
+            block_on(async { self.client.networks().get(network).delete().await.unwrap() });


Nit: This should do the trick as well :)

Suggested change

block_on(async { self.client.networks().get(network).delete().await.unwrap() });

block_on(self.client.networks().get(network).delete()).unwrap();

thomaseizinger · 2020-11-17T21:53:54Z

src/core/docker_async.rs

+}
+
+/// Log streams of running container (Stream<Item = Vec<u8>>).
+#[derive(derivative::Derivative)]


I've dropped the Derivative dependency in dev already. Given that we are not actually including any fields within the debug output, we can easily construct it manually and can therefore avoid this dependency!

thomaseizinger · 2020-11-17T21:56:17Z

src/images/generic_async.rs

+        self
+    }
+
+    pub fn with_mapped_port<P: Into<Port>>(mut self, port: P) -> Self {


We are moving this functionality into the client with #228 to make images easier to implement. Please adapt this implementation accordingly!

thomaseizinger · 2020-11-17T22:12:28Z

src/core/container_async.rs

+    pub async fn logs<'a>(&'a self) -> LogsAsync<'a> {
+        let id_clone: String = String::from(&self.id);
+        self.docker_client
+            .logs(Box::leak(id_clone.into_boxed_str()))


This Box::leak is a bit concerning. I think we have a memory leak here if I am reading the docs correctly.

Feel free to change the signature of .logs if you don't want to mess around with manual dropping here.

thomaseizinger · 2020-11-17T22:13:04Z

src/core/image_async.rs

+    ///
+    /// Most implementations will very likely want to make use of this to wait for a particular
+    /// message to be emitted.
+    async fn wait_until_ready<D: DockerAsync + Sync>(


This is the only function within the ImageAsync trait that is actually async. I think we should refactor this whole "waiting" concept in a way that works for blocking and async clients. That would allow us to reuse the same image definition for both blocking and async clients. Currently, we need to redefine every image with the ImageAsync trait to use it with an async client :/

One idea on how to achieve this would be to remove some flexibility in what images can do here. In particular, there seem to be primarily three things that images do to determine that they are ready:

Wait for a message on stdout

Wait for a message on stderr

Wait for an arbitrary amount of time

I think we can model that be having a function that returns a Vec<WaitFor> where WaitFor is:

enum WaitFor { Message { message: String, stream: Stream }, Time { ms: u64 } }

thomaseizinger · 2020-11-17T22:20:11Z

Cargo.toml

@@ -18,6 +18,10 @@ rand = "0.7"
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 sha2 = "0.9"
+shiplift =  {git = "https://github.com/softprops/shiplift" } 


Let's hope they are cutting a release soon! We can't release with git dependencies so if we merge this before hand, we will need to do some manual work to exclude this code on every release. That shouldn't be too hard to so it is not actually a blocker for merging this PR!

GitFlow for the win! 😁

To make this easier, could you squash all of the commits into a single one once we ready to merge this so I can just revert that when we make releases?

ywalterh · 2020-11-30T13:39:52Z

First, thank you so much for working on this @ywalterh! :)
Using an HTTP-based docker client is something I've envisioned from the very beginning! It is nice to see it happening now!

I've left several comments with many of them being fairly small.

There is one bigger topic regarding refactoring how wait_until_ready works. I believe it would be worthwhile try and get rid of the ImageAsync trait so that we don't have to redefine all images and they are automatically compatible with several clients.

Let me know if you are still keen to work on this! If you do, it would be good to have this refactoring as a separate commit at the beginning of this PR (or maybe even as a completely separate PR and we rebase this on top of that) so that I can easily revert the async stuff in the releases branches until shiplift cuts a release!

No problem at all. Sorry I was on Thanksgiving break and didn't reply to this sooner. Let me address these issues first and look into the feasibility of removing ImageAsync.

Making this feature declarative has several advantages: - It reduces the amount of code one needs to write for a new image. - It avoids the need for an `ImageAsync` trait once we introduce that, see discussions in #216 for details. - It allows for a re-usable abstraction of waiting for a specific amount of time, which fixes #39.

@ywalterh

253: Replace `wait_until_ready` with declarative `ready_conditions` r=thomaseizinger a=thomaseizinger Making this feature declarative has several advantages: - It reduces the amount of code one needs to write for a new image. - It avoids the need for an `ImageAsync` trait once we introduce that, see discussions in #216 for details. cc @ywalterh - It allows for a re-usable abstraction of waiting for a specific amount of time, which fixes #39. Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

thomaseizinger · 2021-02-18T07:11:57Z

Closing this in favor of #268.

Thank you for all the hard work @ywalterh !

Making this feature declarative has several advantages: - It reduces the amount of code one needs to write for a new image. - It avoids the need for an `ImageAsync` trait once we introduce that, see discussions in #216 for details. - It allows for a re-usable abstraction of waiting for a specific amount of time, which fixes #39.

Added client.rs with shiplift client.

b76c4a1

Creating and running container working, using tokio::runtime to get around the fact tha shiplift is mostly async

ywalterh added 3 commits October 2, 2020 11:39

Implement start stop rm and ports

b885c73

Still not able to get Box<dyn Read> back from async block

7d1fc84

Use Arc to pass things along

a4a4894

ywalterh added 4 commits October 5, 2020 22:18

Added async trait and some stub code

a42b1a8

test fix

656ec13

Added container / image async APIs

5acccae

Fix the API for a simple test using Async trait

79bf69a

thomaseizinger reviewed Oct 11, 2020

View reviewed changes

src/clients/shiplift.rs Outdated Show resolved Hide resolved

src/core/container_async.rs Outdated Show resolved Hide resolved

ywalterh and others added 5 commits October 12, 2020 19:00

Incremental progress to make logging working

4f15d2a

Use a different LogsAsync with stderr/stdout separated

2ecdc40

Also fixed a lifetime error: Box by detaul assumes static

Fix test; Passed first simple test

7ec30af

Adding back Drop capability but it blocks now

9c00431

make test use tokio::test

Attempted at Drop, no luck yet

740d844

thomaseizinger reviewed Oct 14, 2020

View reviewed changes

src/core/container_async.rs Outdated Show resolved Hide resolved

Walterh Huang and others added 5 commits October 14, 2020 23:20

After using threaded_scheduler, block_on works now

68c9c76

Added a few tests

186d856

Added a few tests

adec6d7

Implement GenericImageAsync and wrote test against it

0b70af6

Merge branch 'dev' of https://github.com/ywalterh/testcontainers-rs i…

d540919

…nto dev

thomaseizinger reviewed Oct 20, 2020

View reviewed changes

src/clients/shiplift.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Oct 20, 2020

View reviewed changes

tests/shiplift_client.rs Outdated Show resolved Hide resolved

ywalterh added 4 commits October 20, 2020 20:42

Organized tests and add a failing test for network

2461d2e

Implement network handling and Drop for client; Added all the tests

6996d3c

Changed Shiplift client to Http client

983fa12

Update documentation, getting ready for review

87e2cc5

thomaseizinger mentioned this pull request Nov 12, 2020

Allow to pass string args to docker run #233

Closed

ywalterh changed the title ~~[WIP] Use shiplift Docker client library instead of cli~~ Use shiplift HTTP Docker client library instead of CLI Nov 17, 2020

thomaseizinger requested changes Nov 17, 2020

View reviewed changes

thomaseizinger linked an issue Nov 17, 2020 that may be closed by this pull request

Provide an async API #142

Closed

thomaseizinger mentioned this pull request Jan 27, 2021

Postgres image only wait for first "ready" line in the log #248

Closed

thomaseizinger mentioned this pull request Feb 1, 2021

Replace wait_until_ready with declarative ready_conditions #253

Merged

thomaseizinger closed this Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use shiplift HTTP Docker client library instead of CLI #216

Use shiplift HTTP Docker client library instead of CLI #216

ywalterh commented Sep 30, 2020 •

edited

ywalterh commented Sep 30, 2020

thomaseizinger commented Oct 5, 2020

thomaseizinger commented Oct 5, 2020

ywalterh commented Oct 5, 2020

ywalterh commented Nov 17, 2020

thomaseizinger left a comment •

edited

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020

thomaseizinger Nov 17, 2020 •

edited

ywalterh commented Nov 30, 2020

thomaseizinger commented Feb 18, 2021

	block_on(async { self.client.networks().get(network).delete().await.unwrap() });
	block_on(self.client.networks().get(network).delete()).unwrap();

Use shiplift HTTP Docker client library instead of CLI #216

Use shiplift HTTP Docker client library instead of CLI #216

Conversation

ywalterh commented Sep 30, 2020 • edited

ywalterh commented Sep 30, 2020

thomaseizinger commented Oct 5, 2020

thomaseizinger commented Oct 5, 2020

ywalterh commented Oct 5, 2020

ywalterh commented Nov 17, 2020

thomaseizinger left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Nov 17, 2020 • edited

Choose a reason for hiding this comment

ywalterh commented Nov 30, 2020

thomaseizinger commented Feb 18, 2021

ywalterh commented Sep 30, 2020 •

edited

thomaseizinger left a comment •

edited

thomaseizinger Nov 17, 2020 •

edited