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

Re-work public API to be Container centric #386

Closed
thomaseizinger opened this issue Jul 15, 2022 · 9 comments · Fixed by #575
Closed

Re-work public API to be Container centric #386

thomaseizinger opened this issue Jul 15, 2022 · 9 comments · Fixed by #575
Assignees
Labels
enhancement New feature or request

Comments

@thomaseizinger
Copy link
Collaborator

thomaseizinger commented Jul 15, 2022

Since its inception, testcontainers-rs had an API that focused on first creating a Client before one could run a Container. This was deemed acceptable as a Client is typically only created once at the top of the test.

Recent feature requests such as #354, show some of the limitations of this API. One of the primary design goals of testcontainers-rs is to have a really small API such that creating and running a new container is as easy as possible.

To accommodate feature requests such as #354 but still adhere to this design goal, I am thinking to re-design the API to something like this:

use testcontainers::RunViaCli; // Extension trait for running an image using the `Cli` client.

let container = MyImage::default().run();
use testcontainers::RunViaHttp; // Extension trait for running an image using the `Http` client.

let container = MyImage::default().run().await;

The client would be started on-demand and kept in a global variable with a Weak reference so that it shuts down once all containers are gone. The client itself would likely be made completely private to the crate to further reduce the public API (which allows us to make more changes without breaking compatibility).

We'd have to flesh out some details on how to configure the individual clients if they can no longer be constructed directly but that is not too important for this topic.

// CC several people who had issues with the current API in the past: @soulseekeer24 @lcmgh @marlon-sousa @jhershberg @pronvis @ufoscout

Any feedback / input on this idea is much appreciated.

@marlon-sousa
Copy link
Contributor

Hello,

I strongly encourage this implementation.

While this crate was aimed at isolated tests, it is too good and people's needs (beyond this should be this or that way) are too extensive. All the needs can take advantage of this crate should the proposed architecture be implemented.

Real life some times goes further than what "important people", personal believes or famous theories have to say, mature environments should consider all, and I mean really all things a matter of trade offs ratter than right or wrong.

This said, the proposed change will allow a vast range of testing needs to succeed, using an ergonomic, fast and well encapsulated strategy. Everyone wins, nobody is left behind and, independently of what we think testing should be like, everyone can just use the crate for whatever strategies they need to use. This is what a really good crate should be, and your crate just happens to be great, otherwise people would just switch for shell scripts or for invoking process around using std::process::Command instead of coming here and asking for help.

I for example am running a set of integration tests that run sequentially and all use the same database, thus I need to start a container when tests start to work and stop it when the suite ends.

I tried to encapsulate the client and the containers created by this client together, because I wanted to provide a encapsulated interface to the test engine where one just need to start the containers and either stop them or let drop do it when tests ended.

Independently of what "important people" or "important books" say, this is the best strategy for this specific scenarios, I am confident, and so are my team members given the requirements we have. Being able to transparently use the crate on this strategy would really be great. This would also not break the isolation scenarios, which I and others can very well use when appropriate.

@endertunc
Copy link

Is anyone actively working on this issue?

As personal side project, I am currently re-writing this repository in more container-centric way. I am getting inspiration from both testcontainers-go and testcontainers-node which uses very similar container-centric design/approach.

Probably it's a bit premature to ask this but I was wondering if Re-work public API could also be interpreted as re-write?

Current work I am doing would basically be a new library and I was wondering how would people react to such dramatic update. We might be able to find middle ground at some point but I just wanted to test the water.

@thomaseizinger
Copy link
Collaborator Author

Is anyone actively working on this issue?

No.

As personal side project, I am currently re-writing this repository in more container-centric way. I am getting inspiration from both testcontainers-go and testcontainers-node which uses very similar container-centric design/approach.

Probably it's a bit premature to ask this but I was wondering if Re-work public API could also be interpreted as re-write?

I don't think anything around the Image trait needs to change and neither do the clients. "All" we need is invert the dependency between clients and containers by transparently starting the client in the background. My first idea would have been to keep it in a thread-local or static variable and use Arc + Weak to check how many copies of the client are still alive (inside running Containers).

Current work I am doing would basically be a new library and I was wondering how would people react to such dramatic update. We might be able to find middle ground at some point but I just wanted to test the water.

I think it makes sense to be compatible with the Image trait.

@thomaseizinger
Copy link
Collaborator Author

In general though, I am open to big PRs / re-writes although I'd prefer if they are broken down into smaller ones and that we have a reasonable upgrade path for users.

Meta: If we end up landing such a series of PRs where you wrote most / all of the code, I think it makes sense to also make you a maintainer :)

@endertunc
Copy link

Hi @thomaseizinger thanks for your reply. I will try to discover the possibilities you mentioned before going full re-write 👍 re-writes are never a good idea anyway :)

@blandger
Copy link

blandger commented Nov 2, 2023

Any progress on issue ?? Is it still in draft by #503 ? just curious...

@thomaseizinger
Copy link
Collaborator Author

Any progress on issue ?? Is it still in draft by #503 ? just curious...

Yes. What is on GitHub is the current state :)
Nobody is actively working on this to my knowledge.

@endertunc
Copy link

I was suppose to follow up on the draft but life happened so I didn't have much time to look in to it. I will try to make some time for it and let's see how far we can go 👍

@DDtKey
Copy link
Collaborator

DDtKey commented Apr 7, 2024

JFYI: we gonna deprecate the Cli client (#563), and I'd like to actualize this proposal.

@DDtKey DDtKey self-assigned this Apr 7, 2024
@DDtKey DDtKey added enhancement New feature or request and removed help wanted Extra attention is needed labels Apr 12, 2024
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
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants