What does this PR do?

Deprecate shell-escape based LocalDockerCompose implementation and provide new docker/compose based one using the Docker API. Introduce a new API that takes context.Context into account. Furthermore it exposes more typed options.

#425

@baez90 shall we change the PR to draft? We can start the review once you feel confortable with it

And thanks for your time here, using the native Go libraries for compose is a great addition that none of the other languages can do! :kudos:

We can do that if you prefer. I would actually appreciate a first review! To make the pipeline working I'd have to remove Go versions < 1.16. I could do that but wasn't sure if this is okay?

We can do that if you prefer. I would actually appreciate a first review! To make the pipeline working I'd have to remove Go versions < 1.16. I could do that but wasn't sure if this is okay?

Fine for me about the removal 🙋‍♂️

Codecov Report

Merging #476 (b231f60) into main (4c04bdf) will decrease coverage by 20.96%. The diff coverage is 0.20%.

:exclamation: Current head b231f60 differs from pull request most recent head 88f3c4b. Consider uploading reports for the commit 88f3c4b to get more accurate results

@@             Coverage Diff             @@
##             main     #476       +/-   ##
===========================================
- Coverage   34.50%   13.54%   -20.97%     
===========================================
  Files          13       15        +2     
  Lines        1759     2001      +242     
===========================================
- Hits          607      271      -336     
- Misses       1056     1684      +628     
+ Partials       96       46       -50     

Impacted Files	Coverage Δ
compose.go	0.00% <0.00%> (ø)
compose_api.go	0.00% <0.00%> (ø)
compose_local.go	0.00% <0.00%> (ø)
docker.go	20.25% <10.00%> (-21.02%)	:arrow_down:
reaper.go	3.50% <0.00%> (-71.93%)	:arrow_down:
mounts.go	0.00% <0.00%> (-50.00%)	:arrow_down:
docker_mounts.go	9.75% <0.00%> (-36.59%)	:arrow_down:
container.go	40.96% <0.00%> (-18.08%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Thanks for this PR! Looks super promising!

It would be also awesome if we improve the docs about using compose with the new API design.

Sure, I'm actually wondering if I should try to get a PR into docker/compose to update their docker/docker dependency because this could resolve some replace issues I encountered and which we should otherwise document as well.

I'll give it a shot tomorrow to see if the dependency update would make things easier and update the docs either way accordingly :blush:

@baez90 no updates on this PR as I've been very busy with certain updates that will be public soon. I'll let you know once I'm 100% available to continue with this (hopefully soon)

Don't you worry I'm a bit busy myself as well. As previously mentioned I tried if upgrading the Docker version would help to get rid of the replacement stuff but doesn't work. Probably when Docker 22.xx is released or so 😅

I'll try to update the docs probably tomorrow or begining of next week and I also have to rebase the branch once more. Do you mind if I add something about Podman support as well when I'm already on it or shall I create another PR for that?

Until then good luck with whatever you're currently working on 😉

Do you mind if I add something about Podman support as well when I'm already on it or shall I create another PR for that?

That would be perfect in a separate PR, faster to review and merge.

Until then good luck with whatever you're currently working on 😉

Thanks my friend!!

Alright @mdelapenya I prepared at least some docs, rebased everything once more and updated the dependencies.

Let me know if I should update the description once more or if the docs need further adjustments or anything else :blush: till then I'll have a look at #496

I just wanted to add a reference to an issue I remembered from the Compose project where someone asked if compose can be used as a library and actual API, only to realize that you opened it 🤯

So for future reference, this is the issue where they track the progress on this: https://github.com/docker/roadmap/issues/387

I'd suggest only releasing stable support for this, once upstream provides stable support. However, since you already distinguish by API within tc-go, it might be ok.

Well tc-go itself is also not release yet as stable hence I figured it should be okay to use the (fairly) stable compose API especially because everything compose related is wrapped so breaking changes (at least small-ish ones) could be adapted without breaking the tc-go public API.

I'll resolve the conflicts once more and afterwards we can discuss how to proceed with this PR?

Maybe tc-go is abstracting compose further away as tc-java does it, but Compose V2 has some pretty significant breaking changes, as we found while working on https://github.com/testcontainers/testcontainers-java/pull/5608 (@eddumelendez can probably share the concrete details here).

Most prominent is the container/service naming, using hyphens instead of underscores.

Regarding:

Well tc-go itself is also not release yet as stable

while technically true, every breaking change will reflect on how users (and there are existing users of tc-go of course) perceive the maturity of the library, completely independent on what is actually communicated in the version number.

Funny that you mention the hyphens-vs-underscores problem as I took care of that a while ago :smile: (see here if you're interested) when I noticed that most of the test code was still running with compose V1 while I was running the tests locally with V2 but apart from that I haven't experienced any further problems with compose V1 vs. V2.

I totally understand your concern that the API should be as stable as possible to ensure tc-go users don't get frustrated by having to deal with breaking changes every time they update. I am using it on a daily basis myself and my first contribution was actually to rework a previous 'non-breaking-change' that kept the existing API but changed the behavior regarding bind mounts with the result that the code was still compiling but my tests were failing because mount source and target were inverted (previously the map key was the source and the value the target and suddenly it was the other way around) which is a great example that sometimes breaking the API is better than keeping it :smile:

But as I said previously the new public API does not directly expose any compose details and also only includes some basic compose options so I'm pretty confident almost every breaking change can be hidden. The worst case I can think of is that some options aren't supported in the future and this can still be handled gracefully by deprecating the tc-go option and make it a noop option for a few releases.

Do I miss something?

just want to add that besides the hyphens/underscore change in compose v2 the compose file version supported in v2 starts with 2. There should be services at the root of the yaml file

Is something missing here @mdelapenya ?

Is something missing here @mdelapenya ?

Hey @baez90 we are preparing the next release (0.14.0) and would like to have this refactor of docker-compose ready for the next release (0.15.0), so until we set up the basis for a more formal release process, I'll postpone merging this until the 0.14.0 release is out (really soon)

Therefore we will be able to deliver these great changes, using the native Go APIs for compose, in a separate release without impacting existing users.

These are my plans, that I wanted to share with you before doing anything else.

@baez90 I'm starting the review of this PR, to make it into the project the soonest. Sorry for the delay, and thank you again for your contributions

I went through all the comments and applied them (almost) exactly as suggested, let me know if you find anything else you want me to change :blush:

I'll have a look next week, just noticed, that the CI isn't green anymore :see_no_evil:

I believe the CI errors come from the locks and the wait strategies 🤔

goroutine 39 [semacquire, 9 minutes]:
sync.runtime_Semacquire(0xc0002944e0?)
	/opt/hostedtoolcache/go/1.18.6/x64/src/runtime/sema.go:56 +0x25
sync.(*WaitGroup).Wait(0xc0004fc240?)
	/opt/hostedtoolcache/go/1.18.6/x64/src/sync/waitgroup.go:136 +0x52
golang.org/x/sync/errgroup.(*Group).Wait(0xc0002cc4c0)
	/home/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:53 +0x27
github.com/testcontainers/testcontainers-go.(*dockerCompose).Up(0xc0004f6630, {0x16c2d00, 0xc000090d00}, {0xc0003f26a0, 0x1, 0x171f876?})
	/home/runner/work/testcontainers-go/testcontainers-go/compose_api.go:226 +0x7c5
github.com/testcontainers/testcontainers-go.TestDockerComposeAPIStrategyForInvalidService(0xc000294000)
	/home/runner/work/testcontainers-go/testcontainers-go/compose_api_test.go:43 +0x212
testing.tRunner(0xc000294000, 0x1535a90)
	/opt/hostedtoolcache/go/1.18.6/x64/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.18.6/x64/src/testing/testing.go:1486 +0x35f

You were right, I missed one call to a public function I added a lock call to, to avoid concurrent read-write ops to the container cache. I made this one private, removed the locks and added a public wrapper so that the locks are still in place but the second occurrence is using the private implementation :blush:

Rebased the branch once more onto main and did a final dependency update for docker/compose. As long as there's nothing more, I'd say this is ready :smile:

Weird: seeing this error in mac when running go vet https://github.com/testcontainers/testcontainers-go/actions/runs/3191143864/jobs/5207098574

yeah, I upgraded to latest docker/compose and now both CreateOptions and StartOptions have a Services field :roll_eyes:

Hopefully this works now, otherwise I'll fix it tomorrow evening

I think this is ready. Let me do a last minute review, but other than that, LGTM. Huge PR, profound changes, big thanks

It's weird: i resolved conflicts running go mod tidy locally and get the same errors as in the CI:

# github.com/tonistiigi/fsutil
../../../../pkg/mod/github.com/tonistiigi/[email protected]/walker.go:100:30: undefined: fileutils.MatchInfo
../../../../pkg/mod/github.com/tonistiigi/[email protected]/walker.go:101:30: undefined: fileutils.MatchInfo
../../../../pkg/mod/github.com/tonistiigi/[email protected]/walker.go:156:41: undefined: fileutils.MatchInfo
../../../../pkg/mod/github.com/tonistiigi/[email protected]/walker.go:160:40: includeMatcher.MatchesUsingParentResults undefined (type *fileutils.PatternMatcher has no field or method MatchesUsingParentResults)
../../../../pkg/mod/github.com/tonistiigi/[email protected]/walker.go:191:41: undefined: fileutils.MatchInfo
../../../../pkg/mod/github.com/tonistiigi/[email protected]/walker.go:195:40: excludeMatcher.MatchesUsingParentResults undefined (type *fileutils.PatternMatcher has no field or method MatchesUsingParentResults)
# github.com/docker/buildx/driver/kubernetes/context
../../../../pkg/mod/github.com/docker/[email protected]/driver/kubernetes/context/load.go:31:41: cannot use &EndpointMeta{} (value of type *EndpointMeta) as type command.EndpointDefaultResolver in variable declaration:
        *EndpointMeta does not implement command.EndpointDefaultResolver (wrong type for ResolveDefault method)
                have ResolveDefault() (interface{}, *"github.com/docker/cli/cli/context/store".EndpointTLSData, error)
                want ResolveDefault(command.Orchestrator) (interface{}, *"github.com/docker/cli/cli/context/store".EndpointTLSData, error)
# github.com/docker/buildx/driver/kubernetes/manifest
../../../../pkg/mod/github.com/docker/[email protected]/driver/kubernetes/manifest/manifest.go:91:9: unknown field 'Handler' in struct literal of type "k8s.io/api/core/v1".Probe
../../../../pkg/mod/github.com/docker/[email protected]/driver/kubernetes/manifest/manifest.go:91:25: undefined: v1.Handler

:smile: it's not really weird, the merge removed the replace directives in the go.mod currently necessary to work around the problem you noticed until moby/moby will probably finally release a version with Go modules support and a newer version of github.com/tonistiigi/fsutil :roll_eyes:

😄 it's not really weird, the merge removed the replace directives in the go.mod currently necessary to work around the problem you noticed until moby/moby will probably finally release a version with Go modules support and a newer version of github.com/tonistiigi/fsutil 🙄

Ah thanks! The merge conflicts resolution on mod files is many times a mess 🤦 We merged #548, which included a run of go mod tidy, and that caused it. I'm tempted to add a pre-commit hook to validate the go mod, so it's not run on CI only