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.

Sign up for GitHub

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

Jump to bottom

Ports shouldn't be cached #392

Closed
schrieveslaach opened this issue Aug 19, 2022 · 3 comments · Fixed by #575
Closed

Ports shouldn't be cached #392

schrieveslaach opened this issue Aug 19, 2022 · 3 comments · Fixed by #575

Comments

@schrieveslaach
Copy link

Since commit 88e18b44 ports will be cached. However, ports can actually change if a container is assigned to a new network.

You can reproduce this issue by checking out this commit of the aixigo/PREvant respository and you can run cargo test nginx --manifest-path api-tests/Cargo.toml -- --test-threads=1.

During the test the traefic container will be connected to the network of an nginx container. This can cause that exposed port mapping will change.

To workaround this issue I have to map the port explicitly:

let traefik = docker.run(RunnableImage::from(Traefik::default()).with_mapped_port((9789, 80)));
@schrieveslaach
Copy link
Author

Workaround is included in aixigo/PREvant#104. Search for comments mentioning the workaround. You can remove it to reproduce.

@thomaseizinger
Copy link
Collaborator

Thanks for opening an issue!

I don't recall whether this on purpose or just an oversight. @yottabytt can you add something to this?
I think it should be fine to change this back to fetch the ports on demand. For ContainerAsync, we actually still do this.

This was referenced Apr 16, 2024
Closed
Closed
DDtKey added a commit that referenced this issue Apr 18, 2024
@DDtKey
feat!: container-centric API
ae9f585 
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
@DDtKey
feat!: container-centric API
f2d27d2 
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 DDtKey mentioned this issue Apr 18, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Apr 22, 2024
@DDtKey
feat!: container-centric API with refactored underlying layer (#575)
9c65640 
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
@schrieveslaach
Copy link
Author

Thanks for fixing. Works with aixigo/PREvant@22cc416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!: container-centric API with refactored underlying layer testcontainers/testcontainers-rs
2 participants
@thomaseizinger @schrieveslaach