fix(compose): fix compose destroys right after Up #2679
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
kezhenxu94
force-pushed
the
bugfixcompose
branch
7 times, most recently
from
7b7f1fb to
bf21276
Compare
modules/compose/compose_api.go
Outdated
| @@ -267,12 +267,10 @@ func (d *dockerCompose) Down(ctx context.Context, opts ...StackDownOption) error | |||
| return d.composeService.Down(ctx, d.name, options.DownOptions) | |||
| } | |||
|
|
|||
| func (d *dockerCompose) Up(ctx context.Context, opts ...StackUpOption) error { | |||
| func (d *dockerCompose) Up(ctx context.Context, opts ...StackUpOption) (err error) { | |||
There was a problem hiding this comment.
Is there a way to avoid naked returns?
There was a problem hiding this comment.
Is there a way to avoid naked returns?
If we really want to avoid that, we have to check err != nil or not in every single return statement, and if err != nil cleanup by sending a termSignal, this is more error prune in my opinion, what do you think?
There was a problem hiding this comment.
I try to avoid naked returns 😅 The verbosity of Go comes with readability, which comes with maintainability
There was a problem hiding this comment.
I refactor the code to avoid naked return, let me know how you like it 😄
|
I have one question regarding my example project for compose: https://github.com/mdelapenya/testcontainers-go-compose Can you elaborate a repro snippet based on that project? 🙏 |
You can replace the entire file package main
import (
"context"
"fmt"
"log"
"path/filepath"
"time"
tccompose "github.com/testcontainers/testcontainers-go/modules/compose"
)
func ExampleCompose() {
compose, err := tccompose.NewDockerCompose(filepath.Join("testdata", "docker-compose.yml"))
if err != nil {
log.Fatal(err)
}
defer func() {
if err := compose.Down(context.Background(),
tccompose.RemoveOrphans(true), tccompose.RemoveImagesLocal); err != nil {
log.Fatal(err)
}
}()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
err = compose.Up(ctx, tccompose.Wait(true))
if err != nil {
log.Fatal(err)
}
serviceNames := compose.Services()
fmt.Println(serviceNames)
time.Sleep(20 * time.Second)
mysqlContainer, err := compose.ServiceContainer(context.Background(), "mysql")
fmt.Println(err)
_, err = mysqlContainer.Inspect(ctx)
fmt.Println(err) // I expect there is a container for mysql service, which is not the case
// Output:
// [mysql nginx]
// <nil>
// <nil>
}and run |
kezhenxu94
changed the title
kezhenxu94
force-pushed
the
bugfixcompose
branch
from
f4af01f to
197da11
Compare
|
There's actually a more in depth fix for this in #2664 @kezhenxu94 would you mind testing that branch to make sure if fixes your issue please. |
|
I've extracted the compose fix from the bug bash and its here #2722 let me know if that fixes the issues. |
Are you sure a chore issue "Remove unused parameters from private functions." also fix a bug? |
Sorry @kezhenxu94 right link but wrong cherry-pick commit id included, have fixed that now. |
|
@kezhenxu94 could you please verify if the main branch, which contains #2722, works for you? 🙏 If so, let's close this one, thanks! |
|
Thanks @kezhenxu94 for taking the time to contribute to the project, and for verifying the fix. Very much appreciated 🙇 |
What does this PR do?
The
NewDockerCompose.UpsendstermSignalas soon as the function finishes, this causes the compose project to be destroyed and future usages of it is impossible, this PR only sendstermSignalwhen there is any error when invokingUpmethod, otherwise do not send terminal signal.Why is it important?
Without this patch, the docker compose API is useless, without this patch it just starts the docker compose project and then immediately destroys it, no container can be used after calling
Upfunction.Related issues
How to test this PR
It can be easily reproduced by reverting the fix in the
compose_api.goand run thecompose_api_test.go, an error will be thrown:while the container must exist as it is what we aim to start up