Skip to content
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: support native docker compose api #476

Merged

Conversation

baez90
Copy link
Contributor

@baez90 baez90 commented Jul 1, 2022

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

@mdelapenya mdelapenya self-assigned this Jul 5, 2022
@mdelapenya mdelapenya self-requested a review July 5, 2022 02:59
@mdelapenya
Copy link
Collaborator

@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:

@baez90
Copy link
Contributor Author

baez90 commented Jul 8, 2022

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?

@mdelapenya
Copy link
Collaborator

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 🙋‍♂️

.github/workflows/ci-podman.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR @baez90!!!

I'm starting to like the functional approach for optional configurations, although I need to practice more. Thanks for sharing!

At this stage of the review, this PR is a +1. Great great great job with the tests!

compose.go Outdated Show resolved Hide resolved
compose.go Outdated Show resolved Hide resolved
compose_api.go Outdated Show resolved Hide resolved
compose_api.go Outdated Show resolved Hide resolved
compose_api.go Outdated Show resolved Hide resolved
compose_api_test.go Outdated Show resolved Hide resolved
compose_api_test.go Outdated Show resolved Hide resolved
@baez90 baez90 force-pushed the 425-support-native-docker-compose-api branch 3 times, most recently from b80f3c4 to 1d21f17 Compare July 12, 2022 17:18
@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

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

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%) ⬇️
reaper.go 3.50% <0.00%> (-71.93%) ⬇️
mounts.go 0.00% <0.00%> (-50.00%) ⬇️
docker_mounts.go 9.75% <0.00%> (-36.59%) ⬇️
container.go 40.96% <0.00%> (-18.08%) ⬇️

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

@baez90 baez90 force-pushed the 425-support-native-docker-compose-api branch from 44bd0b0 to e3ab14a Compare July 12, 2022 19:44
@mdelapenya
Copy link
Collaborator

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.

@baez90
Copy link
Contributor Author

baez90 commented Jul 13, 2022

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 😊

@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jul 13, 2022
@mdelapenya
Copy link
Collaborator

@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)

@baez90
Copy link
Contributor Author

baez90 commented Jul 27, 2022

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 😉

@mdelapenya
Copy link
Collaborator

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!!

@baez90 baez90 force-pushed the 425-support-native-docker-compose-api branch from e3ab14a to f880dcb Compare July 28, 2022 19:20
@baez90 baez90 force-pushed the 425-support-native-docker-compose-api branch from a2ec5a6 to da7ee20 Compare August 11, 2022 18:40
@baez90 baez90 changed the title WIP: 425 support native docker compose api 425 support native docker compose api Aug 11, 2022
@baez90
Copy link
Contributor Author

baez90 commented Aug 11, 2022

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 😊 till then I'll have a look at #496

@kiview
Copy link
Member

kiview commented Aug 31, 2022

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:
docker/roadmap#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.

@baez90
Copy link
Contributor Author

baez90 commented Aug 31, 2022

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?

@kiview
Copy link
Member

kiview commented Aug 31, 2022

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 testcontainers/testcontainers-java#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.

@baez90
Copy link
Contributor Author

baez90 commented Aug 31, 2022

Funny that you mention the hyphens-vs-underscores problem as I took care of that a while ago 😄 (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 😄

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?

@baez90 baez90 force-pushed the 425-support-native-docker-compose-api branch from da7ee20 to ad322ed Compare August 31, 2022 15:19
@baez90 baez90 requested a review from a team as a code owner August 31, 2022 15:19
@eddumelendez
Copy link
Member

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

@baez90
Copy link
Contributor Author

baez90 commented Sep 6, 2022

Is something missing here @mdelapenya ?

@baez90 baez90 force-pushed the 425-support-native-docker-compose-api branch from ec4d113 to eaa8999 Compare October 18, 2022 19:53
@baez90
Copy link
Contributor Author

baez90 commented Oct 18, 2022

I incorporated the docs changes we spoke of and added another section regarding the service name API.
Also updated to the latest docker-compose version and rebased the branched once more to resolve conflicts.
Let me know if there's anything else missing 😉

@mdelapenya
Copy link
Collaborator

mdelapenya commented Oct 19, 2022

At the moment there is a force-push, I lose track of what I'd already reviewed and what I hadn't. I'd thank if we avoid pushing-force the PRs 🙏

We are squashing commits since the release of 0.14, so no worries about the story of the main branch: it will be now cleaner :)

@baez90
Copy link
Contributor Author

baez90 commented Oct 19, 2022

I added another commit regarding the docs yesterday and did not change any previous commit, I only rebased it onto main again to resolve conflicts 😉 no rewrite of the history happened, you only need to review the latest commit 😊 hope that helps

If you prefer I'll merge main if necessary in the future to resolve conflicts, I'm just used to the rebase workflow 😉

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments or typos in docs


func TestSomething(t *testing.T) {
compose, err := tc.NewDockerCompose("testresources/docker-compose.yml")
assert.NoError(t, err, "NewDockerComposeAPI()")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.NoError(t, err, "NewDockerComposeAPI()")
assert.NoError(t, err, "NewDockerCompose()")

func TestSomethingElse(t *testing.T) {
identifier := tc.StackIdentifier("some_ident")
compose, err := tc.NewDockerComposeWith(tc.WithStackFiles("./testresources/docker-compose-simple.yml"), identifier)
assert.NoError(t, err, "NewDockerComposeAPIWith()")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.NoError(t, err, "NewDockerComposeAPIWith()")
assert.NoError(t, err, "NewDockerComposeWith()")


### Interacting with compose services

To interact with service containers after a stack was started it is possible to get an `*tc.DockerContainer` instance via the `ServiceContainer(...)` function.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
To interact with service containers after a stack was started it is possible to get an `*tc.DockerContainer` instance via the `ServiceContainer(...)` function.
To interact with service containers after a stack was started it is possible to get a `*tc.DockerContainer` instance via the `ServiceContainer(...)` function.

The function takes a **service name** (and a `context.Context`) and returns either a `*tc.DockerContainer` or an `error`.
This is different to the previous `LocalDockerCompose` API where service containers were accessed via their **container name** e.g. `mysql_1` or `mysql-1` (depending on the version of `docker-compose`).

Furthermore, there's the convenience function `Serices()` to get a list of all services **defined** by the current project.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Furthermore, there's the convenience function `Serices()` to get a list of all services **defined** by the current project.
Furthermore, there's the convenience function `Services()` to get a list of all services **defined** by the current project.


## Usage of `docker-compose` binary

_Node:_ this API is deprecated and superseded by `ComposeStack` which takes advantage of `docker-compose` v2 being
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_Node:_ this API is deprecated and superseded by `ComposeStack` which takes advantage of `docker-compose` v2 being
_Node:_ this API is **deprecated** and superseded by `ComposeStack` which takes advantage of `docker-compose` v2 being

@mdelapenya
Copy link
Collaborator

If you prefer I'll merge main if necessary in the future to resolve conflicts, I'm just used to the rebase workflow 😉

Yes please 🙏 that would help!

Apart from that, the CI builds are failing, seems related to TestDockerCreateContainerWithDirs. Could you take a look?

@mdelapenya
Copy link
Collaborator

@baez90 I found #574, which it's fixing the CI

@mdelapenya
Copy link
Collaborator

@baez90 I've noticed this error in the pipeline, related to a concurrent access to a map (compose_api.go:294):

=== FAIL: . TestDockerComposeApiWithWaitForShortLifespanService (unknown)
Network b2e2bce1-ac00-48f3-82fc-8403274bef5c_default  Creating
Network b2e2bce1-ac00-48f3-82fc-8403274bef5c_default  Created
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-falafel-1  Creating
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-tzatziki-1  Creating
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-tzatziki-1  Created
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-falafel-1  Created
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-falafel-1  Starting
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-tzatziki-1  Starting
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-falafel-1  Started
Container b2e2bce1-ac00-48f3-82fc-8403274bef5c-tzatziki-1  Started
fatal error: concurrent map writes

goroutine 710 [running]:
runtime.throw({0x1f5e0d1?, 0x70?})
	/opt/hostedtoolcache/go/1.18.7/x64/src/runtime/panic.go:992 +0x71 fp=0xc0004e7c78 sp=0xc0004e7c48 pc=0x43a631
runtime.mapassign_faststr(0x100?, 0x0?, {0x1f45191, 0x7})
	/opt/hostedtoolcache/go/1.18.7/x64/src/runtime/map_faststr.go:212 +0x39c fp=0xc0004e7ce0 sp=0xc0004e7c78 pc=0x41551c
github.com/testcontainers/testcontainers-go.(*dockerCompose).lookupContainer(0xc000865200, {0x2277d08, 0xc00041d140}, {0x1f45191, 0x7})
	/home/runner/work/testcontainers-go/testcontainers-go/compose_api.go:294 +0x4fe fp=0xc0004e7f20 sp=0xc0004e7ce0 pc=0x19f65de
github.com/testcontainers/testcontainers-go.(*dockerCompose).Up.func1()
	/home/runner/work/testcontainers-go/testcontainers-go/compose_api.go:229 +0x57 fp=0xc0004e7f78 sp=0xc0004e7f20 pc=0x19f5a77
golang.org/x/sync/errgroup.(*Group).Go.func1()
	/home/runner/go/pkg/mod/golang.org/x/sync@v0.1.0/errgroup/errgroup.go:75 +0x64 fp=0xc0004e7fe0 sp=0xc0004e7f78 pc=0x62da84
runtime.goexit()
	/opt/hostedtoolcache/go/1.18.7/x64/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc0004e7fe8 sp=0xc0004e7fe0 pc=0x46da61
created by golang.org/x/sync/errgroup.(*Group).Go
	/home/runner/go/pkg/mod/golang.org/x/sync@v0.1.0/errgroup/errgroup.go:72 +0xa5

Could you take a look? 🙏

@mdelapenya
Copy link
Collaborator

@baez90 test are passing! I pushed 4111cdc and b231f60 with the sole goal of making this PR into main ASAP. Hope you understand.

My plans are: release v0.15.0 this week(end) without this PR, and immediately merge this PR for a dedicated v0.16.0 to isolate the compose changes. Does it sound good to you?

@mdelapenya
Copy link
Collaborator

@baez90 I'm merging this PR now. Big big thanks for the huge refactor which simplifies a lot the maintenance of the compose module. We will eventually remove (and communicate) the local compose code after having the native implementation.

@mdelapenya mdelapenya merged commit f3b2e5e into testcontainers:main Oct 24, 2022
14 checks passed
@baez90
Copy link
Contributor Author

baez90 commented Oct 24, 2022

Thanks for taking care! I just couldn't manage to get some time to work on this PR so thanks a lot! Also for all the support, time for reviews and your valuable input all the time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compose Docker Compose. feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants