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

Upgrade testcontainers to 0.16 #3063

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

divergentdave
Copy link
Contributor

Testcontainers-rs published a big refactor over the weekend, this PR updates to adapt to the API changes. The biggest changes are that testcontainer clients are no longer part of the API surface, and the library now takes care of lazily instantiating them, plus the non-async client now wraps the HTTP-based client in a new Tokio runtime, rather than shelling out to Docker's CLI. We have to switch to the async API in several places where we are already running inside a Tokio runtime.

@divergentdave divergentdave requested a review from a team as a code owner April 29, 2024 16:36
@divergentdave
Copy link
Contributor Author

divergentdave commented Apr 29, 2024

The next release of testcontainers should fix the cargo-deny issues by pushing the serde-java-properties dependency behind a Cargo feature.

@divergentdave divergentdave marked this pull request as draft April 29, 2024 17:23
Cargo.toml Outdated
@@ -98,7 +98,7 @@ signal-hook-tokio = "0.3.1"
sqlx = "0.7.4"
stopper = "0.2.7"
tempfile = "3.10.1"
testcontainers = "0.15.0"
testcontainers = { version = "0.16.3", features = ["blocking"] }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason not to use the AsyncRunner everywhere -- it looks like you already used it everywhere except for EphemeralDatabase? Dropping the use of SyncRunner would let us drop the blocking feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I was thinking of doing that as a follow-up once everything was working

@@ -57,8 +57,7 @@ impl EphemeralDatabase {
let shutdown_barrier = Arc::clone(&shutdown_barrier);
move || {
// Start an instance of Postgres running in a container.
let container_client = testcontainers::clients::Cli::default();
let db_container = container_client.run(RunnableImage::from(Postgres::default()));
let db_container = RunnableImage::from(Postgres::default()).start();
Copy link
Member

Choose a reason for hiding this comment

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

If the reason we're still using the sync interface in this one place is because of the use of a (synchronous) thread, I think this would be pretty easy to replace with an async task -- other than the testcontainers interface & the async channel (which are already async), we use a Barrier to communicate shutdown. This Barrier could pretty easily be replaced by another oneshot channel, for example, to allow us to async-ify all of the blocking operations.

Copy link
Member

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

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

Very nice, I like dropping the thread/task entirely. (I don't remember why we used a separate thread in the first place; hopefully that functionality wasn't unnecessary from the start.)

@divergentdave divergentdave marked this pull request as ready for review April 29, 2024 21:24
@divergentdave
Copy link
Contributor Author

Looks like Container wasn't Send + Sync at the time, so that's why it had to be owned by its own thread. This changed in version 0.15.0. See #999 (comment).

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.

2 participants