Skip to content

Conversation

amab8901
Copy link
Contributor

@amab8901 amab8901 commented Aug 1, 2025

Attempts to partially solve:
#252

The scope of this PR is only the async containers (but not the sync). At a quick glance it looks like it should plausibly work, but I haven't tested it. This also includes minor refactoring to make clippy stop complaining about arguments in format!() macro.

Feel free to look through the changes and test it and see if there's anything you'd like to modify before merging!

Copy link

netlify bot commented Aug 1, 2025

Deploy Preview for testcontainers-rust ready!

Name Link
🔨 Latest commit b364300
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-rust/deploys/68909f533a9e4c00089fdac8
😎 Deploy Preview https://deploy-preview-826--testcontainers-rust.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@DDtKey
Copy link
Contributor

DDtKey commented Aug 1, 2025

These are useful changes, but we need to cover them with the documentation and tests. Also pay attention to the red CI checks.

Also we shouldn't provide functionality only to async impl except the cases when it's technically not possible. In this case it should be relatively easy to enrich sync implementation.

Thank you for the contribution! ❤️

@amab8901
Copy link
Contributor Author

amab8901 commented Aug 4, 2025

it builds now. But the other tests are kinda flaky. Vast majority of them pass. Maybe 1-2 fail. If I run another time, then everything passes. If I run a 3rd time, then 1-2 fail that are different from the first 1-2 that failed earlier. This issue occurs even in the main branch, so I conclude that this PR probably doesn't contribute to those failures

mbodmer and others added 3 commits August 4, 2025 13:53
For a start I have now documented the relevant parts of the builder API
here.

I have added no_run to examples as the needed resources are missing,
they should be built however to ensure correctness.

---------

Signed-off-by: Marc Bodmer <[email protected]>
Signed-off-by: mbodmer <[email protected]>
@amab8901
Copy link
Contributor Author

amab8901 commented Aug 4, 2025

I think it's ready for merging.

I manually tested it and it behaves like this:

  • for default bridge network, you can run the test fine without .with_net_alias(). If you add .with_net_alias(), then it lets bollard throw this error (which is a bollard issue rather than testcontainers issue): client_error=CreateContainer(DockerResponseServerError { status_code: 400, message: "invalid config for network bridge: invalid endpoint settings:\nnetwork-scoped alias is supported only for containers in user defined networks" })
  • for user-defined bridge-network (i.e. a network with bridge driver but a name other than bridge), the network alias you insert in .with_net_alias() shows up in the container's Aliases and DNSNames:
image
  • for user-defined bridge network, if you don't use .with_net_alias(), then it only uses the default Aliases and DNSNames:
image

@DDtKey
Copy link
Contributor

DDtKey commented Aug 4, 2025

All failing tests are related to network tests with --features ring,ssl, like:

  failures:
  
  ---- runners::async_runner::tests::async_run_command_should_include_network stdout ----
  Error: failed to connect container to network
  
  
  failures:
      runners::async_runner::tests::async_run_command_should_include_network

These aren't flaky in main, we have few flaky ones - but not these ones.

So I guess this change somehow breaks compatibility with ring.
At least it doesn't look like flakiness as all tests are failing

@amab8901
Copy link
Contributor Author

amab8901 commented Aug 5, 2025

how can I locally reproduce the test that led you to conclude that this PR breaks compatibility with ring?

I did the following locally:

  1. go to aa/alias branch
  2. run cargo hack test --feature-powerset --depth 2 --clean-per-run --partition 1/4 5 times
  3. go to main branch
  4. run cargo hack test --feature-powerset --depth 2 --clean-per-run --partition 1/4 5 times

The terminal output says that the command failed in the following places on each attempt:

Test

aa/alias

  1. cargo test --manifest-path testcontainers/Cargo.toml --no-default-features --features blocking

    sync_should_return_error_when_non_bridged_network_selected

  2. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_should_create_network_if_image_needs_it_and_drop_it_in_the_end
    sync_should_return_error_when_non_bridged_network_selected

  3. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_run_command_with_container_network_should_not_expose_ports
    sync_should_return_error_when_non_bridged_network_selected

  4. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_should_return_error_when_non_bridged_network_selected

  5. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    async_start_should_apply_expected_labels
    sync_should_return_error_when_non_bridged_network_selected

main

  1. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_should_return_error_when_non_bridged_network_selected

  2. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_should_return_error_when_non_bridged_network_selected

  3. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_should_return_error_when_non_bridged_network_selected

  4. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    async_start_should_apply_expected_labels
    sync_should_return_error_when_non_bridged_network_selected

  5. cargo test --manifest-path testcontainers/Cargo.toml --all-features

    sync_should_return_error_when_non_bridged_network_selected

Observations

I don't see much difference between main and aa/alias in the above test

@DDtKey
Copy link
Contributor

DDtKey commented Aug 9, 2025

Please check the results of CI actions. All test-related actions are red, and retries don’t seem to be helping (second attempt failed as well with the same results)

While main and other PRs have successfully passed CI checks, this PR definitely behaves in an unexpected way, despite the assumption about the ring.

Do you use fresh upstream?

@amab8901
Copy link
Contributor Author

I might not necessarily have the time to finish this PR. When I originally started this task, it was because I had an issue I was stuck on when trying to make two containers connect with each other. This PR was my attempt to fix the bug I had in the communication between the two containers. I found another workaround so now it works. Feel free to continue where I left off or pass this PR to anyone else who might be interested!

@amab8901 amab8901 closed this Sep 16, 2025
@amab8901 amab8901 deleted the aa/alias branch September 16, 2025 12:27
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.

3 participants