Skip to content

feat: accept filters and keep_storage in prune_builds#3192

Merged
milas merged 4 commits into
docker:mainfrom
emranbm:main
Dec 5, 2023
Merged

feat: accept filters and keep_storage in prune_builds#3192
milas merged 4 commits into
docker:mainfrom
emranbm:main

Conversation

@emranbm

@emranbm emranbm commented Dec 3, 2023

Copy link
Copy Markdown
Contributor

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>

@milas milas 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.

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

Comment thread docker/api/build.py Outdated

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

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.

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment thread docker/api/build.py
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
Copy Markdown
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
Copy Markdown
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>

@milas milas 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.

Thanks for the speedy turn-around

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

Comment thread docker/api/build.py Outdated
milas and others added 2 commits December 5, 2023 00:05
Signed-off-by: Milas Bowman <devnull@milas.dev>
@emranbm

emranbm commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

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
@milas

milas commented Dec 5, 2023

Copy link
Copy Markdown
Contributor

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

2 participants