Skip to content

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:


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

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