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

[Discussion] Reason to maintain two different implementation? #508

Closed
endertunc opened this issue Jul 8, 2023 · 1 comment · Fixed by #575
Closed

[Discussion] Reason to maintain two different implementation? #508

endertunc opened this issue Jul 8, 2023 · 1 comment · Fixed by #575

Comments

@endertunc
Copy link

This library currently supports two way to communicate with docker;

  • CLI using docker command
  • HTTP using bollard (unofficial) Rust SDK for Docker

I do think that this approach bring some unnecessary complexity without a real benefit (I would like to be corrected on this one if I missed something). Every feature has to be implemented twice with different approach, for example, sync vs async.

To my understanding, testcontainers-rs is the only one which depends on docker command directly. I have been looking some of the popular implementation like java, golang and node, they all seem to take the second approach which is using SDK for Docker.

Given the fact that wherever you have docker command available, I believe you also have your docker daemon running on UNIX socket therefore there is no need to directly talk with docker command. (that's what docker command talks to as well if I am not mistaken)

I would assume that CLI implementation is around for historical reasons but I don't see any benefits of keeping it around in the long term.

Of course this is my humble opinion and I would like to hear what others think. :)

cc @thomaseizinger

@thomaseizinger
Copy link
Collaborator

I would assume that CLI implementation is around for historical reasons but I don't see any benefits of keeping it around in the long term.

Correct, it was the first implementation and allowed us to progress quickly.

I am open to having it removed. One problem is that we will force an async runtime onto every user which isn't great. There will be plenty of users that don't write async Rust code.

DDtKey added a commit that referenced this issue Apr 18, 2024
Quite large refactoring as part of project revamp #563, and also the long-awaited refactoring #386

Closes #386
Closes #326
Closes #475
Closes #508
Closes #392
Closes #561
Closes #559

MSRV was bumped to `1.70` in order to use `std::sync::OnceLock`. This should NOT be a problem, tests usually executed on more recent versions (also see [this ref](https://github.com/testcontainers/testcontainers-rs/pull/503/files#r1242651354)).
DDtKey added a commit that referenced this issue Apr 18, 2024
Quite large refactoring as part of project revamp #563, and also the long-awaited refactoring #386

Closes #386
Closes #326
Closes #475
Closes #508
Closes #392
Closes #561
Closes #559

MSRV was bumped to `1.70` in order to use `std::sync::OnceLock`. This should NOT be a problem, tests usually executed on more recent versions (also see [this ref](https://github.com/testcontainers/testcontainers-rs/pull/503/files#r1242651354)).
github-merge-queue bot pushed a commit that referenced this issue Apr 22, 2024
Quite large refactoring as part of project revamp #563, and also the
long-awaited refactoring #386

Now it's really simple API, e.g:
```rs
let container = GenericImage::new("redis", "latest")
        .with_exposed_port(6379)
        .with_wait_for(WaitFor::message_on_stdout("Ready to accept connections"))
        .start();
```

I find this new API much easier to use, solves a lot of problems, and
seems flexible enough to be extended.
This also works regardless of the tokio runtime flavor (multi-thread vs
current-thread). And the sync API is still available, right under
`blocking` feature (just like `reqwest` does).

From a maintainer's perspective this also simplifies the code, we don't
have to worry about two different clients and their differences.

### Docker host resolution
The host is resolved in the following order:

1. Docker host from the `tc.host` property in the
`~/.testcontainers.properties` file.
2. `DOCKER_HOST` environment variable.
3. Docker host from the "docker.host" property in the
`~/.testcontainers.properties` file.
4. Else, the default Docker socket will be returned.

### Notes
- MSRV was bumped to `1.70` in order to use `std::sync::OnceLock`. This
should NOT be a problem, tests usually executed on more recent versions
(also see [this
ref](https://github.com/testcontainers/testcontainers-rs/pull/503/files#r1242651354)).
- `Cli` client is removed, instead we provide `sync` (under `blocking`
feature) and `async` impls based on HTTP client (bollard)
- tested with
[modules](https://github.com/testcontainers/testcontainers-rs-modules-community)

## Migration guide 

- Sync API migration (`Cli` client)
  - Add `blocking` feature
  - Drop all usages of `clients::Cli`
  - Add `use testcontainers::runners::SyncRunner;`
  - Replace `client.run(image)` with `image.start()`
- Async API migration (`Http` client)
  - Remove `experimental` feature
  - Drop all usages of `clients::Http`
  - Add `use testcontainers::runners::AsyncRunner;`
  - Replace `client.run(image)` with `image.start()`

## References

Closes #386
Closes #326
Closes #475
Closes #508
Closes #392
Closes #561
Closes #559
Closes #564
Closes #538
Closes #507
Closes #89
Closes #198
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 a pull request may close this issue.

2 participants