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

Adding filter to system prune #306

Merged
merged 9 commits into from
Feb 12, 2022

Conversation

dvizzini
Copy link
Contributor

@dvizzini dvizzini commented Feb 8, 2022

I also updated the documentation.

I could not find any tests to modify. Are there any?

@dvizzini
Copy link
Contributor Author

dvizzini commented Feb 9, 2022

Just realized this PR has an issue:

$ docker system prune --all --volumes --filter "until=1680h"
ERROR: The "until" filter is not supported with "--volumes"

Where can I write unit tests that address this scenario?

@gabrieldemarmiesse
Copy link
Owner

First of all, thank you for the PR that's great :)

You can add tests in this file: https://github.com/gabrieldemarmiesse/python-on-whales/blob/master/tests/python_on_whales/components/test_system.py

Currently we don't have any tests for docker.system.prune() but I would be happy if we could have some.
You can simply create some objects (containers, pulling images, creating volumes), run the prune command and then check that those objects aren't there anymore.

If you need help, don't hesitate to ask!

@dvizzini
Copy link
Contributor Author

Thank you! For one thing we need a --force in the command argument. This will hang as currently implemented. I will add that to the PR.

assert my_net in docker.network.list()
docker.system.prune()
assert my_net not in docker.network.list()
except DockerException:
Copy link
Owner

@gabrieldemarmiesse gabrieldemarmiesse Feb 11, 2022

Choose a reason for hiding this comment

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

I believe too many pieces of code can throw a DockerException. It might hide a real bug in the test. Can you try to make the try:... Except: narrower? Or you could create the network with a random name, meaning that it won't have any effect if, by chance, the prune doesn't remove it. So you don't need the with statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about this being too broad.

I don't quite understand what you are talking about with the random name. I create the network with random_name.

I'll think of a way to make this work.

Choose a reason for hiding this comment

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

Sorry, I wasn't clear. What I meant was that since the network has a random name, you don't need to delete it, meaning that you don't need to use with, meaning that you don't need a try except

Copy link
Contributor Author

@dvizzini dvizzini Feb 12, 2022

Choose a reason for hiding this comment

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

I was deleting it -- twice! Point taken that I don't need to worry too much about a network with a randomly generated name floating around.

I got rid of the context manager. I don't need to worry about __exit__ not being called because cleaning the network is exactly what this test tests, and as you point out this network is pretty harmless even if that test failed.

@gabrieldemarmiesse
Copy link
Owner

Thanks you for this great pull request! It's a really cool contribution!

@gabrieldemarmiesse gabrieldemarmiesse merged commit ee231c1 into gabrieldemarmiesse:master Feb 12, 2022
@dvizzini
Copy link
Contributor Author

@gabrieldemarmiesse Thank you for your review! I am happy python_on_whales is so well maintained.

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