Skip to content

Add docker.prune - pruning for Docker subsystems#46176

Merged
terminalmage merged 6 commits into
saltstack:developfrom
The-Loeki:docker-prune
Mar 3, 2018
Merged

Add docker.prune - pruning for Docker subsystems#46176
terminalmage merged 6 commits into
saltstack:developfrom
The-Loeki:docker-prune

Conversation

@The-Loeki

Copy link
Copy Markdown
Contributor

What does this PR do?

Add docker.prune, which is capable of clearing Docker's subsystems of cruft. Which seems necessary, a lot ;)

@The-Loeki

Copy link
Copy Markdown
Contributor Author

Function prune has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

FOR REALZ ROFLMAO I could explain this function to my Cognitively Complexively Challenged neighbours

@rallytime

rallytime commented Feb 26, 2018

Copy link
Copy Markdown
Contributor

@The-Loeki Yeah, we need to tweak the rules for the code-climate build. Can you also add some tests for this new function?

@rallytime rallytime added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Feb 26, 2018
@The-Loeki

Copy link
Copy Markdown
Contributor Author

Hi, I'm afraid I'm going to have to pass on that one due to time constraints... I will however update the function once I get the same function PR'd in docker-py; that should get rid of the hidden function calls.

After that I'm not even sure what there is to test in this case. That is, I'm assuming Docker does it's testing on the API endpoints, _client_wrapper is tested as well, I'm not parsing / evalling any output, so what's left is to test is the if constructs?

Comment thread salt/modules/dockermod.py Outdated
def prune(system=True, containers=False, networks=False, images=False, build=False, volumes=False, **filters):
'''
Prune Docker's various subsystems

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doing a quick sweep of the codebase, these API functions all seem to have been added in docker-py 2.1.0. Can you please add the following here?

    .. note::
        This requires docker-py version 2.1.0 or later.

If pre-2.1.0 docker-py is installed, _client_wrapper() function will handle the API error from the unknown function call, so we won't have a traceback. This is just a bit of extra information that should help users who may be on earlier docker-py releases.

@terminalmage terminalmage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually there are a few things about this which don't actually work at all, it looks like it may not even have been tested since the kwargs argument doesn't exist for these API funcs and trying to run this function on my laptop immediately resulted in a tracecback.

I'm working on some modifications that I can submit against your fork that should clear this up.

@terminalmage

Copy link
Copy Markdown
Contributor

Another thing I noticed as I look deeper into the docker-py codebase is that negative filters don't seem to be supported. I've opened docker/docker-py#1940 upstream to get clarification.

The kwargs argument isn't supported and attempting to pass it to the API
will result in API errors. This fixes that bug and makes improvements to
the docstring.

It also changes the behavior of the `system` argument so that it doesn't
need to be manually disabled to prune only containers/images/etc.

Finally, it moves the prune function higher up to fit with the
organizational structure of the module.
@terminalmage

Copy link
Copy Markdown
Contributor

@The-Loeki Have a look at https://github.com/The-Loeki/salt/pull/1, I made some fixes, changed how system works, and added unit tests.

Bugfixes, tweaks, tests for docker.prune
@The-Loeki

Copy link
Copy Markdown
Contributor Author

@terminalmage thanks for all the help and advice, regarding the older api's: I can probably easily write up the same kind of workarounds for the other prunes?

@terminalmage

Copy link
Copy Markdown
Contributor

You mean posting directly to the API endpoint? Yeah, that should be doable, and I can also work to get them supported in docker-py. Which other prunes are yet to be supported?

@The-Loeki

The-Loeki commented Mar 3, 2018

Copy link
Copy Markdown
Contributor Author

AFAIK just the one I already opened a PR for (https://github.com/docker/docker-py/pull/1925/commits), so that would be purely a backwards compat feature

@terminalmage

Copy link
Copy Markdown
Contributor

Oh, so you were talking about making backwards-compatibility changes for containers, images, etc? If so, I don't think it's necessary in this PR.

@terminalmage terminalmage removed the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Mar 3, 2018
@terminalmage terminalmage merged commit 58afd8f into saltstack:develop Mar 3, 2018
@The-Loeki The-Loeki deleted the docker-prune branch March 5, 2018 09:37
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.

3 participants