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

import docker-compose and docker-py rather than fork subprocesses #19

Closed
waynr opened this issue Jan 7, 2018 · 11 comments
Closed

import docker-compose and docker-py rather than fork subprocesses #19

waynr opened this issue Jan 7, 2018 · 11 comments

Comments

@waynr
Copy link

waynr commented Jan 7, 2018

Hi, this plugin looks really cool! After briefly looking over the code and some of the issues on this project I thought I'd pop in and point out that many of the problems (not having access to service logs for example) would be simplified if it were possible to have direct access to the objects exposed in the docker-compose or docker-py library APIs. Specifically, you should be able to do almost everything you can do from the command line using this class which should provide the following benefits for this codebase:

  • allow you and pytest-docker plugin users to programmatically access service and container info
  • allow you to dispense with subprocess management in your code and focus on implementing new ways to interact with docker in pytest-idiomatic ways

Even something as simple as a more generic version of this would be fairly helpful. Here I use the docker-py library to start up container, yield it as the test fixture object, then tear it down post-yield. In an approach like this providing access to service/container logs as requested in #13 could be a configurable behavior that occurs post-yield.

I hope you find my suggestions helpful. I would like to try my hand at implementing this if you don't have time or don't want to yourself.

@waynr waynr changed the title import compose rather than fork subprocesses import docker-compose and docker-py rather than fork subprocesses Jan 7, 2018
@AndreLouisCaron
Copy link
Contributor

Thanks for taking the time to write this up :-)

I have to admit that I'm a bit surprised by everybody's interest in this project. I posted this here so I could save 20ish lines of code that I kept copy-pasting from one project to another. It does what I need it to (spawn databases for quick integration tests) and some other obligations seriously limit the time I can put into maintenance for this project. Even taking the time to review contributions is quite hard for me at the moment.

I'd be glad to see a community-driven pytest plug-in for Docker that addresses all the different use cases, but I can't put the time that project would need in terms of maintenance, so it would have to be another project. If you want to publish a more advanced pytest-docker plug-in that does that, please let me know and I can flag this one as deprecated.

@nfk
Copy link

nfk commented Feb 19, 2018

Hi everyone,

I have a similar docker fixture which is based on compose library and it returns a compose.Project object. I will submit a merge request in few days. My aim is to use pytest-docker instead of my customs fixtures. @AndreLouisCaron: You can assign this issue to me?

@butla
Copy link

butla commented Feb 19, 2018

@nfk will it be easy to capture the output of Docker Compose with your change? I have this hacky fork (mentioned in #13) in which I made pytest-docker spill out the output from containers on test errors. But I'm working on refactoring the codebase so that changes like mine can be more elegant and better integrated. When my code is mature enough I'll be submitting pull requests, of course.

@nfk
Copy link

nfk commented Feb 19, 2018

@butla: In my side I have a dedicated fixture to run docker-compose logs -f because to run this command it is required to fork the process with Popen in background, this is similar of your hacky fork but you can use this fixture on demand without break the current pytest-docker behaviour for example.

You can also have a function fixture which use directly docker API library to get logs for a given service with since parameter, but the limitation of this way is that the logs are displayed at the end of the test not in stream mode.

@butla
Copy link

butla commented Feb 20, 2018

@nfk Being able to display the output of Compose as it's running is something that we'd really like (and is something that I have in my backlog). That's because Compose on our CI machine sometimes takes long, and it's really annoying to have to wait until the test finally times out to see what was wrong. Setting too short timeouts was also a problem.

I'll see what I can actually do in this regard after I put some more time into the code, but I was thinking of running compose in capsys.disabled() until the ports are ready, so that any problems during startup are immediately visible.

As for a separate fixture for seeing the logs - I think that seeing happened in the containers for a failed test should be the default. Though I see that there are cases when you want to test stdout and stderr, so it needs to be configurable.

@nfk
Copy link

nfk commented Feb 26, 2018

@butla and @AndreLouisCaron the first draft here. It changes the tests behaviour and a little rework about the classes DockerComposeExecutor and Services should be done.

@butla
Copy link

butla commented Feb 28, 2018

@nfk I've added a few comments on the commits, I hope they're visible in Github. All in all, I think your changes are positive :)

@nfk
Copy link

nfk commented Mar 1, 2018

@butla Thank for the review, I will rework a little thing before to submit the MR.

@butla
Copy link

butla commented Aug 21, 2018

You know what? I gave this some more thought, and I think pytest-docker should abstain from using Docker Compose as a Python API and keep treating it as a CLI that is launched with subprocess. I think this will be the clean solution in the long run, since the team supporting Docker Compose explicitly states that they don't intend to maintain it as a stable Python library.

@AndreLouisCaron So even though there was some good work done on part of @nfk, I'd suggest closing this issue.

@AndreLouisCaron
Copy link
Contributor

@butla Nice find, very interesting!

I have to admit I find the docker CLI tool a much better anchor than "compose as a library" or even the Docker Python client anyways as the CLI tool has such a widespread usage that it's much less likely to have API breaks.

Closing this now :-)

@butla
Copy link

butla commented Aug 21, 2018

@AndreLouisCaron "We don't support any other API other than CLI" was the impression I got from a few of the maintainers' comments sprinkled around the repo, but it took me a while to find the specific one to post here. They have a shit-ton of issues, many with heated debates :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants