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

feat: accept filters and keep_storage in prune_builds #3192

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

emranbm
Copy link
Contributor

@emranbm emranbm commented Dec 3, 2023

Make the api.prune_builds method accept filters and keep-storage args to pass to the docker API. The API details can be found here.

Signed-off-by: Emran Batmanghelich <emran.bm@gmail.com>
Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I requested a couple of small changes:

@@ -276,10 +276,20 @@ def build(self, path=None, tag=None, quiet=False, fileobj=None,
return self._stream_helper(response, decode=decode)

@utils.minimum_version('1.31')
def prune_builds(self):
def prune_builds(self, filters=None, keep_storage=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also all, which is a boolean and was added at the same time as filters and keep-storage - let's add that as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 303 to 308
params = {}
if filters is not None:
params['filters'] = utils.convert_filters(filters)
if keep_storage is not None:
params['keep-storage'] = keep_storage
return self._result(self._post(url, params=params), True)
Copy link
Contributor

Choose a reason for hiding this comment

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

These args were added in API v1.39 - typically, we add a check for them, e.g.

if utils.version_lt(self._version, '1.39'):
    raise errors.InvalidVersion(
        '`keep-storage` arg only available for API version > 1.38'
    )

(if you look for utils.version_lt or utils.version_gte you'll see a bunch of examples - they're not 100% consistent so do whatever makes sense here, e.g. I'm fine if there's a single check if ANY of filters / keep_storage / any are not None since they were all added together in the same API version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@emranbm emranbm requested a review from milas December 4, 2023 15:44
Signed-off-by: Emran Batmanghelich <emran.bm@gmail.com>
Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Thanks for the speedy turn-around

LGTM, just needs a small fix from copy-paste 🙃

docker/api/build.py Outdated Show resolved Hide resolved
milas and others added 2 commits December 5, 2023 00:05
Signed-off-by: Milas Bowman <devnull@milas.dev>
@emranbm
Copy link
Contributor Author

emranbm commented Dec 5, 2023

Thanks for the speedy turn-around

LGTM, just needs a small fix from copy-paste 🙃

Oh sorry 😢
And thanks for your fix.
Now can it be merged?

@milas milas merged commit 3d0a3f1 into docker:main Dec 5, 2023
10 checks passed
@milas
Copy link
Contributor

milas commented Dec 5, 2023

Merged! Thanks for the PR! ❤️

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