-
Notifications
You must be signed in to change notification settings - Fork 142
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
feat: reusable containers #757
base: main
Are you sure you want to change the base?
feat: reusable containers #757
Conversation
…experimental` flag
✅ Deploy Preview for testcontainers-rust ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…containers.*` namespace
…/reusable-containers # Conflicts: # testcontainers/src/runners/async_runner.rs
…containers.*` namespace
…iners # Conflicts: # testcontainers/src/core/containers/request.rs # testcontainers/src/runners/async_runner.rs
@DDtKey Dug into the test failures, and it looks like it's just I updated the CI action to fix it, but (for obvious reasons) GHA won't run actions from forks / PR branches (usually). Not sure how to proceed here, got any thoughts? |
Actually, I think we can avoid usage of But regarding your question, CI works with your changes (if I got you correctly):
It fails on |
Fair enough. Removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all efforts! ❤️
I've left a few comments, please take a look
testcontainers/src/core/client.rs
Outdated
.into_iter() | ||
.next() | ||
.and_then(|container| container.id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to ensure there is no other containers in the result.
I mean ensure next()
call returns None
, so we can return an error if it's somehow not-unique result
Or, at least print a warning to logs if we wanna choose the first one matching conditions 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree at all; just to clarify though (cause it's super easy to miss 😅) - there's a limit of 1
applied to the filter options passed to the docker socket. In theory, there can only ever be either zero or one results. I 100% agree with the defensive coding spirit though, and am obviously happy to refactor accordingly.
Way-forward-wise, do you think we can trust in bollard
(and docker's) filtration logic and Keep It Simple™ (i.e. leave it as is)? Alternatively, do you think we should remove the limit and add something like:
{
// ...
let mut containers = self
.bollard
.list_containers(options)
.await
.map_err(ClientError::ListContainers)?
.into_iter();
let container_id = containers.next().and_then(|container| container.id);
if let Some(container) = containers.next() {
warn!("insert some warning here")
}
Ok(container_id)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking suggestion below into account, it might be more than 1 container if we have had a code with CurrentSession
and then changed it to Always
. There might be multiple running versions and we most likely need to choose the latest one.
So I'd prefer to keep this logic explicit: remove limit
and introduce our own checks (e.g if Always
- choose the latest, if CurrentSession
- ensure only 1 exists)
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. One small point - remove limit
and in our check Always
== latest, CurrentSession
== container w/ the current session id, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggestion along with the changes discussed below.
Let me know what you think 😁
/// A unique identifier for the currently "active" `testcontainers` "session". | ||
/// | ||
/// This identifier is used to ensure that the current "session" does not confuse | ||
/// containers it creates with those created by previous runs of a test suite. | ||
/// | ||
/// For reference: without a unique-per-session identifier, containers created by | ||
/// previous test sessions that were marked `reuse`, (or where the test suite was | ||
/// run with the `TESTCONTAINERS_COMMAND` environment variable set to `keep`), that | ||
/// *haven't* been manually cleaned up could be incorrectly returned from methods | ||
/// like [`Client::get_running_container_id`](Client::get_running_container_id), | ||
/// as the container name, labels, and network would all still match. | ||
#[cfg(feature = "reusable-containers")] | ||
pub(crate) fn session_id() -> &'static ulid::Ulid { | ||
TESTCONTAINERS_SESSION_ID.get_or_init(ulid::Ulid::new) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I get the session_id
intent here - I mean it's definitely can be useful (for example to sort them if many)
But why do we want to filter by session_id
label?
The idea of reusable containers is to be able to use in next test run again
E.g:
cargo test # `session1`: creates containers
# do something
cargo test # `session2`: reuses containers of `session1`
Moreover, I might want to run parallel test suites with the same containers (I explicitly enabled re-usability)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - this originally came about after I ran into some trouble with the test suite failing and subsequently trying to reuse some containers incorrectly and then causing failures on subsequent runs.
I think a unique session identifier is a good idea / feature, memory is failing me as to why I didn't this up the same parallel test suite edge case you did though 🤔.
In either case, what do you think about keeping the ability, but making it's inclusion in filtration user-configurable? e.x.:
let reusable_container = GenericImage::new("alpine", "latest")
.with_reuse(true)
.start()
.await?;
let session_unique_container = GenericImage::new("alpine", "latest")
.with_reuse(true)
.current_session(true)
.start()
.await?;
That, or change the type of ContainerRequest.reuse
from bool
to something like ReuseDirective
, e.x.:
#[derive(Copy, Clone, Debug, Default)]
pub enum ReuseDirective {
#[default]
Never,
Always,
CurrentSession,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think enum
option is a perfect fit here actually, because I see all of these options useful and looks more extendable to me
However it looks like CurrentSession
option should be able to clean up containers, and we can't provide this properly without ryak
support (or mangling with ctor
)
Perhaps, we may start with Always
only and then extend it with CurrentSession
once ryak
is introduced 🤔
But, generally it's fine for now, as we mention about experimental nature of the feature. So we can keep both documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the more I thought about it after leaving the comment, the more I liked the idea.
Implemented it, pushed the changes. Please take a look at your convenience and let me know what you think 😁
PR implements reusable containers inline with the existing reusable containers feature(s) of
testcontainers
for other languages.Important
This PR follows from (and is branched from) #756. PR 756 must be merged prior to merging this PR