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

Support deploying with docker compose using API #425

Closed
bamsammich opened this issue Apr 6, 2022 · 19 comments
Closed

Support deploying with docker compose using API #425

bamsammich opened this issue Apr 6, 2022 · 19 comments
Labels
compose Docker Compose. good first issue Want to contribute to testcontainers? Start from here

Comments

@bamsammich
Copy link
Contributor

I'm attempting to run tests from a container without docker installed. This isn't an issue for ContainerRequests since it's using the API, but compose relies on docker-compose being present on the machine. It would be beneficial to be able to deploy a compose file without the requirement for external dependencies.

@mdelapenya
Copy link
Collaborator

Thanks for creating this issue! Yes, it's on the plans, exactly the same as the Java folks do with the ContainerisedDockerCompose (https://github.com/testcontainers/testcontainers-java/blob/06fc95f5233b0c11d2ba319b4b02587db0b5028b/core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java#L610)

We are currently using the Local docker compose choice, but it would be more than helpful if you want to contribute it, and we will be glad to review it

@prskr
Copy link
Contributor

prskr commented Jun 21, 2022

I'm actually wondering why not directly 'including' upstream docker-compose as version 2 is implemented in Go. I had a 'quick look' how hard it would be and I think it's actually not that hard to integrate it directly without having to pull any docker-compose in a container whatsoever. I'd be willing to give it a shot if there's some interest. This would actually make a few things easier if we'd not have to deal with different docker-compose versions as the service names are constructed in different ways and stuff...

@mdelapenya
Copy link
Collaborator

Great feedback!! I think the original purpose of the issue is too match the Java APIs, but in Go we have the Docker SDK natively.

I'd see with great eyes having it implemented natively.

@fiftin
Copy link
Contributor

fiftin commented Jun 22, 2022

Hi @mdelapenya

I added NativeDockerComposeExecutor to my PR which uses docker-compose as library. It works.
But I faced problem with environment variables. We can't pass env vars to it separately from testcontainers. As result 2 tests failed.

Please check my another PR #468

@prskr
Copy link
Contributor

prskr commented Jun 23, 2022

@mdelapenya I know you're not going to like this but: how about another breaking change/deprecating the DockerCompose interface? 😄

The current interface:

type DockerCompose interface {
	Down() ExecError
	Up() ExecError
	WaitForService(string, wait.Strategy) DockerCompose
	WithCommand([]string) DockerCompose
	WithEnv(map[string]string) DockerCompose
	WithExposedService(string, int, wait.Strategy) DockerCompose
}

Isn't really idiomatic Go and doesn't really fit when working with the compose Go API.
I'm thinking about something like this:

type ComposeStack interface {
	Up(ctx context.Context) error
	Down(ctx context.Context) error
	WaitForService(string, wait.Strategy) ComposeStack
	WithEnv(map[string]string) ComposeStack
	WithExposedService(string, int, wait.Strategy) ComposeStack
}

type name is only a placeholder, I'm not so fond of it tbh and I'm also wondering if the fluent/builder approach is actually that good. Personally I prefer options not only because the map a lot better to the underlying compose API but also because with the builder approach you're always 'loosing' type information without having a generic interface and with having a generic interface it is actually rather ugly 😄 but I'm open for opinions.

@fiftin I had a look at your PR with the NativeDockerComposeExecutor but I've to admit I do not really understand why you're invoking the CLI commands instead of directly using the api.Service. The required amount of code is almost the same (just gave it a try) and I think it's better to understand.

@fiftin
Copy link
Contributor

fiftin commented Jun 23, 2022

@baez90 because docker-compose has a lot of commands: 'up', 'down', etc and a lot of flags which parsed by cobra library. Do you want to do it yourself? What if authors of docker-compose add new flag in new version, you will modify your code?

Now I just send all args received from user to docker-compose CLI object and it works. Yes, this solution require some hack with environment variables (I will add it if @mdelapenya agrees this solution), but it is reason why dockerized docker-compose much better.

@prskr
Copy link
Contributor

prskr commented Jun 24, 2022

  1. I'm not sure if we actually need to support all commands docker-compose supports. From my perspective up, down and optionally run would suffice at least for the beginning.
  2. at least for up and run the amount of options you can configure isn't really a lot and no, I don't intend to manually expose them all myself but I'd stick with the a very basic set and expose an option interface to let testcontainers-go users add their own option if they specifically need another option to be set
  3. I honestly don't see why creating another new container (in host mode), mounting your Docker/Podman socket into it (if you have one, not possible on Windows with native pipes), starting the container with a few options and try to parse the container output is better than reusing the compose code which works perfectly on every platform where Docker is supported, also the environment variables aren't really a big problem, I'll push my code later this day to illustrate how it can be done.

Apart from all that: you're free to do what you want. I only tried to give some feedback/hint on what could be done. What you make out of it is totally you thing, no reason to be offended or whatsoever...

EDIT: please see here a still very rough example on how to directly use the compose API to up and down a compose project. It's not 100% compatible with the previous API as mentioned earlier and does some assumptions regarding service orphans and stuff but it already supports environment options and I also ported most of the unit tests already to see if something's broken.
It wouldn't be very hard to e.g. also add some variadic options to manipulate the CreateOptions and StartOptions to the Up() function.

I thought about some more convenience methods e.g. to be able to get a *DockerContainer for a given service name to be able to interact directly with the container and stuff but that's also something that could be added later on.

Probably this helps a bit.

@fiftin
Copy link
Contributor

fiftin commented Jun 26, 2022

@baez90 ,

I just follow current API. And it has methods Invoke() and WithCommand() which accepts whole range of docker-compose commands and flags. I'm not a core team member to offer breaking changes to interface. If it possible and backward compatibility isn't a priority, yes, your solution may be better.

@fiftin
Copy link
Contributor

fiftin commented Jun 26, 2022

@baez90

Did you find public documentation for docker compose API. I'm didn't. Why you sure authors don't change API soon? I'm not sure that it is designed to be used as a library in external projects.

But you can be sure that CLI interface will not change.

@prskr
Copy link
Contributor

prskr commented Jun 26, 2022

I'm aware that the current API isn't compatible with my proposal (even though it could be adopted for the time being) but we released breaking changes earlier and I think having an API that takes context.Context into account would be good no matter if spawning a process, creating a container or something else.

Regarding your concerns with the public API of compose I rather feel it the other way around. Everything I use (if I remember correctly) is part of the pkg part of compose which should be stable and semantically versioned (at least if they honor Go principles) whereas your code uses the cmd package which I wouldn't expect to be stable as long as the CLI UX isn't changing the underlying code may very well do so.

Last but not least: either way the public API may change and break our API in the future but that can be (at least partially) avoided by encapsulation, right? So I wouldn't worry about that too much yet as long as Testcontainers-Go 1.0 isn't released?

Eventually it's a matter of taste, I would like to introduce a breaking change - if necessary - as soon as possible and I generally think it would be helpful no matter which approach(es) are to be supported in the future.

@mdelapenya
Copy link
Collaborator

@baez90 @fiftin I LOVE where this discussion is going driven by you two 👏

Wdyt if you both work on a complete revamp of the compose code to support the native API? @mazitovt already created a PR in #463, so probably could be of some help here.

If you all agree, we can freeze all compose-related PRs until this discussion is finished and we as a community like your proposal.

@prskr
Copy link
Contributor

prskr commented Jun 27, 2022

I wouldn't mind to work on this together? I'd like to wait what the docker-compose guys are going to tell us regarding using it as a library and depending on the output I'd love to come up with a new API covering context.Context, probably specific functions for Up(...) and Down(...) and some convenience functions like Services() []string, ServiceContainer(serviceName string) (*DockerContainer, error) and probably a few more.

Regarding the other commands: run is actually only up with a reduced number of services (could be achieved with a option for Up(), I'll have a look how up --build is handled but I think it should be something similar and most other commands would be covered already by exposing an API to interact with a certain container I think? (watching logs, run a command in container, ...).

I would also keep in mind the option to run docker-compose in a container in scenarios where this is probably preferred although I'm not sure if we shouldn't move this to another PR to not blow it up again 😄 .

AFAIK there's also no reaper integration for docker-compose right now (I mean no reaper instance started for a certain compose stack) that could also be nice to be added as a feature.

@prskr
Copy link
Contributor

prskr commented Jun 30, 2022

It appears so far neither of the approaches guarantees a stable API of docker/compose. While this isn't ideal I don't think this is a reason to dump the idea completely as it shouldn't be a real problem to hide the docker/compose API behind a custom API just like it is done already with the Docker client API. Of course breaking changes would require to adopt the testcontainers-go code a bit but I don't expect testcontainers-go users noticing these adoptions in most (if not all) cases.

What do you think? Shall we give it a try, Introduce a new API and add a docker/compose and a container based implementation? We could even keep the current API for the time being and deprecate it to allow users to migrate to the new API gracefully and while they're migrating we can gather some experience with the docker/compose API to see if we encounter breaking changes frequently. If this is the case we can still stick to the container based implementation with the new API.

@mdelapenya
Copy link
Collaborator

I like deprecations, so if we go that way, I'm in 🙋

@prskr
Copy link
Contributor

prskr commented Jul 1, 2022

To improve further discussions I created a WIP PR containing the API based implementation I've been working on. I also duplicated the old test suite and adopted it for the API based one (including some additional tests).

I also added some options to showcase how it can be done in an docker/compose independent way which should make sure we can maintain a stable API even with breaking changes in the upstream project.

The API is more or less like I described it above and 'felt' not too bad while working on the tests.

Due to dependencies to Go 1.16 in docker/compose this breaks the backwards compatibility of testcontainers-go although I wouldn't mind to much about that because Go 1.16 has already reached End-of-Life. But if this is a problem we can still go for the container based implementation.

Let me know what you think 😊

@mdelapenya
Copy link
Collaborator

How do you feel if I convert this thread to a Discussion?

@prskr
Copy link
Contributor

prskr commented Aug 25, 2022

Fine for me if there's more to discuss?

@mdelapenya
Copy link
Collaborator

I'd like to give a bit of traction leveraging GH discussions, therefore I'm moving issues that are actually discussions to there, and this one seemed a great candidate

@mdelapenya mdelapenya added the compose Docker Compose. label Sep 29, 2022
@mdelapenya
Copy link
Collaborator

Given #476 has been merged, I'm closing this issue.

Thanks all for participating in the discussion! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compose Docker Compose. good first issue Want to contribute to testcontainers? Start from here
Projects
None yet
Development

No branches or pull requests

4 participants