Open matthiasschaub opened 3 months ago
Attention: Patch coverage is 93.33333%
with 3 lines
in your changes missing coverage. Please review.
Please upload report for BASE (
main@0ce4fec
). Learn more about missing BASE report.
Files | Patch % | Lines |
---|---|---|
core/testcontainers/core/config.py | 80.00% | 2 Missing :warning: |
core/testcontainers/core/container.py | 96.55% | 0 Missing and 1 partial :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
will review and merge if you have no other plans for changes
@alexanderankin we would welcome a review from you, thanks. They are no further plans right now except addressing what ever comes up in the code review.
Thanks for looking into this @matthiasschaub 👋 We have no clean spec for this features in Java / across languages, so for now I would suggest, we mostly mirror the Java implementation, including its limitations. We can sync on a cross language spec in the future that provides a better DX, but for now I would strongly favor an implementation that mirrors Java.
I already left some comments within the PR.
Currently to make reuse work ryuk needs to be disabled. Should the user do this manually?
Given the above, if a container has reusable
set, it must not be registered with the Ryuk cleanup label (see tc-java).
Thanks for looking into this @matthiasschaub 👋 We have no clean spec for this features in Java / across languages, so for now I would suggest, we mostly mirror the Java implementation, including its limitations. We can sync on a cross language spec in the future that provides a better DX, but for now I would strongly favor an implementation that mirrors Java.
I already left some comments within the PR.
Currently to make reuse work ryuk needs to be disabled. Should the user do this manually?
Given the above, if a container has
reusable
set, it must not be registered with the Ryuk cleanup label (see tc-java).
Thanks for the review @kiview! I agree. It is sensible to follow the Java implementation.
In commit 1ea9ed16a0dd73e11d762808aed5655d44e270be I do not create a Reaper instance during container start-up if reuse is enabled and container has been started with with_reuse
. This works as expected as long as no other container is started without with_reuse
. In that case a Reaper instances is created which will remove the reuse container as well.
Is there a way to explicitly exclude a container from the Reapers routine?
adresses #109
Todo:
with_reuse
in use but ryuk is disabled.Open questions:
reuse_enable
also be configurable via environment variable?with_reuse
in use but ryuk is disabled.~/.testcontainers.properties
. This file should not be present during test run.DockerContainer.reusable: bool = True
)