fix: reuse container requires name

[tisonkun]

... also sync Container should reuse ContainerAsync's drop impl.

This resolves #742.

cc @DDtKey @the-wondersmith

[netlify]

✅ Deploy Preview for testcontainers-rust ready!

Name	Link
🔨 Latest commit	499d5c6
🔍 Latest deploy log	https://app.netlify.com/projects/testcontainers-rust/deploys/692d43a922732600080d6777
😎 Deploy Preview	https://deploy-preview-887--testcontainers-rust.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[tisonkun]

This is why containers are always removed as reported in #742 (comment)

I'm using sync Containers and its Drop impl isn't properly forwarded to the async impl.

[tisonkun]

However, this can't handle the case where an existing container is stopped. It would then failed with:

called `Result::unwrap()` on an `Err` value: Client(CreateContainer(DockerResponseServerError { status_code: 409, message: "Conflict. The container name \"/postgres\" is already in use by container \"5d2940fb2bd042cf859598932aae34aa5004b19d38797165806282b4ada66157\". You have to remove (or rename) that container to be able to reuse that name." }))
thread 'test_simple_pubsub' panicked at tests/toolkit/src/container.rs:150:10:
called `Result::unwrap()` on an `Err` value: Client(CreateContainer(DockerResponseServerError { status_code: 409, message: "Conflict. The container name \"/postgres\" is already in use by container \"5d2940fb2bd042cf859598932aae34aa5004b19d38797165806282b4ada66157\". You have to remove (or rename) that container to be able to reuse that name." }))

I resolve this issue in my bollard based solution by removing a stopped container with the same name, but not sure how to make it more smoothly in testcontainers.

Anyway, this can be a follow-up to improved:

async fn maybe_remove_container(&self, docker: &Docker, container_name: &str) {
    let options = Some(RemoveContainerOptions {
        v: true,
        force: true,
        link: false,
    });

    // best effort to remove existing container; ignore errors
    let _ = docker.remove_container(container_name, options).await;
}

[tisonkun]

stopped container

This is common when a lasting test container gets stopped during the dev machine reboot.

[DDtKey]

It looks like a good follow-up!
However need to decide which policy should be applicable, because it looks like stopped container can be reused too - just restarted on demand.
Thanks!

[tisonkun]

Let me fix the format issue.

... also it seems that tests rely on selecting by labels without container name, let me try to fine tune the impl.

[tisonkun]

@DDtKey failure resolved. Please help in triggering the CI.

[tisonkun]

Besides, we may later drop the "reusable-containers" flag and just deliver it by default. There is no extra dependency requirement.

[the-wondersmith]

@tisonkun I need to dig in to confirm when I get back to my desk, but I think there's some overlap here with the fix branch I'd previously asked you to test. Come to think of it actually, I need to actually get that branch up to date with main 🤔😅.

tl;dr I'll block some time out today to give this PR a closer look, and some time this week to get that branch fixed up and get a proper PR opened.

[tisonkun]

@the-wondersmith 👍🏻

I think this PR can resolve my remaining issue now. If @DDtKey or other maintainer can get it reveiwed, you may put your changes upon this one.

That said, I'm glad to learn any concrete suggestion to improve the patch in place. And there is still a pending issue (#887 (comment)) we need to decide before testcontainers-rs can go back to my testkit ><

[tisonkun]

Reminder @DDtKey maybe retrigger the CI workflow?

[DDtKey]

Sorry for the delay with review @tisonkun!
That looks like a valid fix regardless of reuse container functionality and helps to reuse the code

[tisonkun]

Great! Thanks for your reviewing.