Issue 439: use an existing container #464
Conversation
Codecov Report
@@ Coverage Diff @@
## main #464 +/- ##
==========================================
+ Coverage 69.30% 70.20% +0.89%
==========================================
Files 21 21
Lines 1942 2014 +72
==========================================
+ Hits 1346 1414 +68
- Misses 477 480 +3
- Partials 119 120 +1
Continue to review full report at Codecov.
|
VladimirStepanov
changed the title
|
I did not mention, but great job while elaborating the PR! Thanks for your time here |
docs/features/creating_container.md
Outdated
| Reusing will work only if you pass an existing container's name via 'req.Name' field. | ||
| If the name is empty, or it is not in a list of existing containers, the function will create a new generic container |
There was a problem hiding this comment.
Reading this description, which is pretty clear, this thought came to my mind: why adding a new type of container that could be used in a wrong manner, like creating a GenericReusableContainer but with a name that is not reusable?
This is thinking out loud: do you think adding the same logic to the reusable container into the generic container would make sense? Please take a look at how the Java folks do it: https:/testcontainers/testcontainers-java/blob/de1324ed2800eff4da326d0c23d281399d006bc0/core/src/main/java/org/testcontainers/containers/GenericContainer.java#L384
The GenericContainer, if defined as reusable, reuses the container ID. I think this API would be simpler in terms of usability: as a consumer I don't have to remember the different types of Containers, simply configure the generic one.
Again, this is an opinion, as valid as any other, so please tell us your thoughts
There was a problem hiding this comment.
@mdelapenya I want to suggest next behavior: we add Reuse optional flag to GenericContainerRequest, because maybe a user doesn't want to reuse an existing container..
c, err := GenericContainer(ctx, GenericContainerRequest{
ContainerRequest: ContainerRequest{
Image: "nginx:1.17.6",
ExposedPorts: []string{"80/tcp"},
WaitingFor: wait.ForListeningPort("80/tcp"),
Name: "my_reusable_container",
},
Started: true,
Reuse: true,
})And we will get next possible options:
Reuseis true. We will reuse an existing container byNameor create a new one if it not exists . If we reuse an existing container, we will ignore flagStarted, because the container is already started.Reuseis false. Try to create a new container or return an error if a container already exists.
Wdyt?
There was a problem hiding this comment.
I like it! Please go for it 🚀
|
@mdelapenya updated:) |
mdelapenya
added
the
feature
| sessionID := uuid.New() | ||
| var termSignal chan bool | ||
| if !req.SkipReaper { | ||
| r, err := NewReaper(ctx, sessionID.String(), p, req.ReaperImage) |
There was a problem hiding this comment.
Why do we need reaper for reused container?
There was a problem hiding this comment.
The reaper will only be created if it does not exist, so the code for the reaper creation is there, but will check if a real reaper container already exists.
Close #439