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

capturing stdout and stderr for docker volume prune #311

Conversation

dvizzini
Copy link
Contributor

I made the stdout/stderr handling for docker.volume.prune the same as the others.

The larger issue of how to uniformly handle stderr and stdout in an interesting one but well outside the scope of this PR.

@gabrieldemarmiesse
Copy link
Owner

gabrieldemarmiesse commented Mar 13, 2022

Thank you for the pull request!

While I didn't give a lot of thoughts at first about how to handle stdout and stderr, I believe the default behavior should be this one:

We capture both stderr and stdout. In case of error, we display both. This is the default behavior of run(...).
If stdout is needed by the caller, for example in docker.container.list() then return stdout to the caller, but capture stderr in case an error message must be displayed. This is still the default behavior of run(...).

If the docker command rely extensively on a tty, for exemple with docker.buildx.build(), then, don't capture stdout and stderr to allow the TTY to work. Options should be provided for the user to change this and possibly disable the tty and capture the output. docker.buildx.build() does this currently.

In case the user needs the output, for exemple with docker.run() options should be given so that the user can manipute the output.

It's somewhat related to #292

@gabrieldemarmiesse gabrieldemarmiesse merged commit 304b519 into gabrieldemarmiesse:master Mar 13, 2022
@dvizzini
Copy link
Contributor Author

@gabrieldemarmiesse Thank you for the thorough explanation.

@dvizzini
Copy link
Contributor Author

@gabrieldemarmiesse When do you think you will bump this? Are you waiting on anything from me?

@gabrieldemarmiesse
Copy link
Owner

No worries, I though that the other PR will be merged quickly, but I don't mind making a release now. You can expect the new release in pypi in ~10 min

@dvizzini
Copy link
Contributor Author

Thank you! Thank you ! Thank you!

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

Successfully merging this pull request may close these issues.

2 participants