Convert DanglingOnly to Filters for `docker image prune` by yongtang · Pull Request #28535 · moby/moby

yongtang · 2016-11-17T06:00:38Z

- What I did

This fix is part of fix for #28497

This fix convert DanglingOnly in ImagesPruneConfig to Filters, so that it is possible to maintain API compatibility in the future.

A follow up to this PR will be done once this PR is merged.

- How I did it

- How to verify it

Several integration tests have been added to cover changes.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This fix is related to #28497.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

AkihiroSuda · 2016-11-17T19:09:02Z

SGTM

It would be great if we can support --until="5min ago" as well.

Also it would be great if we can have support for network (another PR)

thaJeztah · 2016-11-18T01:12:34Z

Should we use --filter for this? I think it's more consistent, and expandable (docker prune --filter label=foo at some point?)

thaJeztah · 2016-11-18T01:12:47Z

/cc @mlaventure

AkihiroSuda · 2016-11-18T06:46:34Z

IMO we should have both filter and until for CLI, but for API, we only need filter.

vdemeester · 2016-11-18T11:16:33Z

I do agree with @thaJeztah, it's the same as images where we had --until and --since and we unified it under --filter. Let's jump to --filter=until=… already and not add --until to deprecate it later 😛

justincormack · 2016-11-18T13:57:44Z

SGTM, current prune has not been very useful for CI machines so far, really want prune by age for that use case.

mlaventure · 2016-11-18T15:26:20Z

I'm with @vdemeester and @thaJeztah can we have a --filter instead?

yongtang · 2016-11-18T22:17:23Z

Thanks all. Let me update the PR based on the comments.

yongtang · 2016-11-20T02:20:09Z

I noticed that currently, images/prune has a DanglingOnly field:

// POST "/images/prune"
type ImagesPruneConfig struct {
    DanglingOnly bool   
  }

However, if we takes a filter, then this field could very well be replaced with dangling=true, which will be consistent with docker volume ls --filter dangling=true.

Should we convert DanglingOnly tofilter as well?

mlaventure · 2016-11-20T19:28:24Z

@yongtang sure, why not.

thaJeztah · 2016-11-21T12:26:19Z

Would dangling=false work though (for prune)?

mlaventure · 2016-11-21T16:34:09Z

@thaJeztah both should work.

Currently danglingOnly default to true and -a switch it. It also only affects images atm.

yongtang · 2016-11-27T22:58:18Z

The PR has been updated. Now the --filter flag takes both dangling=<true|false> and until=<timestamp>, replacing DanglingOnly bool. That could be an API change. Depending on if this PR is considered for 1.13 or later, additional update might be required to maintain API compatibility.

AkihiroSuda · 2016-11-28T01:59:19Z

api/server/router/image/image_routes.go

nit: how about Filters: pruneFilters

AkihiroSuda · 2016-11-28T02:53:35Z

cli/command/container/prune.go

i.e. -> e.g.

AkihiroSuda · 2016-11-28T02:55:09Z

cli/command/system/prune.go

Let's dedup this "all vs dangling" logic

AkihiroSuda · 2016-11-28T02:55:20Z

cli/command/system/prune.go

i.e. -> e.g.

AkihiroSuda · 2016-11-28T02:56:48Z

cli/command/system/prune.go

can you support filter for volume and network as well?
(can be another PR)

AkihiroSuda · 2016-11-28T08:36:42Z

cli/command/image/prune.go

I think we don't need to deprecate the --all CLI option, because it is short and easy to use

I also vote to keep --all

AkihiroSuda · 2016-11-28T08:53:58Z

That could be an API change. Depending on if this PR is considered for 1.13 or later, additional update might be required to maintain API compatibility.

Since 1.13 has been already freezed, I do think the until=<timestamp> should not be in 1.13.
However, I also understand the issue about maintaining compatibility of the API.
So, how about the following plan?

1.13

API: Replace ImagePruneConfig.DanglingOnly with ImagePruneConfig.Filters right now. However, the filter API should only support dangling=true/false at the moment, and should not support until=<timestamp>.
CLI: Do not add --filter at the moment. However, the --all option should be internally converted to ImagePruneConfig.Filters.

1.14

API: Add until=<timestamp> (and so on, if we want)
CLI: Add --filter.

WDYT?

mlaventure · 2016-11-28T17:19:03Z

I know we've frozen the branch yet, but I think i'd rather we make an exception and replace DanglingOnly with Filter to avoid having to maintain both in the future since this API wasn't in any stable release yet.

thaJeztah · 2016-11-28T17:43:30Z

Agree with @AkihiroSuda and @mlaventure (discussed briefly with @AkihiroSuda earlier today). It's not released yet, so we can still modify. It's better done now, than having to officially deprecate / change after release

yongtang · 2016-11-28T20:43:32Z

Thanks all for the review. I am also in favor of replacing DanglingOnly with Filter and get it in before 1.13 release (just to avoid future deprecate/compatibility issue).

I will update the PR based on the comments shortly.

This fix convert DanglingOnly in ImagesPruneConfig to Filters, so that it is possible to maintain API compatibility in the future. Several integration tests have been added to cover changes. This fix is related to 28497. A follow up to this PR will be done once this PR is merged. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

cpuguy83

LGTM

ping @thaJeztah

vdemeester

LGTM 🐸

This fix is a follow up for comment moby#28535 (comment) This fix provides `--filter until=<timestamp>` for `docker container/image prune`. This fix adds `--filter until=<timestamp>` to `docker container/image prune` so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp. Related docs has been updated Several integration tests have been added to cover changes. This fix fixes moby#28497. This fix is related to moby#28535. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Convert DanglingOnly to Filters for `docker image prune`

This fix is a follow up for comment moby/moby#28535 (comment) This fix provides `--filter until=<timestamp>` for `docker container/image prune`. This fix adds `--filter until=<timestamp>` to `docker container/image prune` so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp. Related docs has been updated Several integration tests have been added to cover changes. This fix fixes #28497. This fix is related to #28535. Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Upstream-commit: e2416af0136b72d369de53ff7928d7cc603d4f46 Component: cli

This fix is a follow up for comment moby/moby#28535 (comment) This fix provides `--filter until=<timestamp>` for `docker container/image prune`. This fix adds `--filter until=<timestamp>` to `docker container/image prune` so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp. Related docs has been updated Several integration tests have been added to cover changes. This fix fixes #28497. This fix is related to #28535. Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Upstream-commit: 337483496b355b61d3aa4fd4b4f4853e59be646a Component: cli

This fix is a follow up for comment moby/moby#28535 (comment) This fix provides `--filter until=<timestamp>` for `docker container/image prune`. This fix adds `--filter until=<timestamp>` to `docker container/image prune` so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp. Related docs has been updated Several integration tests have been added to cover changes. This fix fixes #28497. This fix is related to #28535. Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Upstream-commit: fdd6879b68d2587d1c88282a9b5e2da6566af25d Component: cli

This fix is a follow up for comment moby/moby#28535 (comment) This fix provides `--filter until=<timestamp>` for `docker container/image prune`. This fix adds `--filter until=<timestamp>` to `docker container/image prune` so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp. Related docs has been updated Several integration tests have been added to cover changes. This fix fixes #28497. This fix is related to #28535. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

nicosingh · 2019-07-23T03:53:55Z

Hi,

I'm very very sorry in advance for this newbie question 😬, but I can't understand the definition of dangling parameter in /images/prune endpoint (ref):

dangling=<boolean>:
- When set to true (or 1), prune only unused and untagged images
- When set to false (or 0), all unused images are pruned

The usage of the words only and all brings my confusion. I'd expect to read something like this (if my understanding is correct):

dangling=<boolean>:
- When set to true (or 1), both unused and untagged images are pruned
- When set to false (or 0), only unused images are pruned

Or:

dangling=<boolean>:
- When set to true (or 1), only untagged images are pruned
- When set to false (or 0), only unused images are pruned

Or:

dangling=<boolean>:
- When set to true (or 1), only untagged images are pruned
- When set to false (or 0), both unused and untagged images are pruned

By the way, which is the default value for dangling field?. I'd expect to think it's false.

Thanks in advance for your help! My apologies if these questions are duplicated (I couldn't find them elsewhere).

GordonTheTurtle added the status/0-triage label Nov 17, 2016

yongtang mentioned this pull request Nov 17, 2016

Prune based on age #28497

Closed

vdemeester added status/1-design-review and removed status/0-triage labels Nov 17, 2016

yongtang force-pushed the 28497-prune-until branch from 6d2d708 to 778d074 Compare November 27, 2016 00:26

AkihiroSuda added the rebuild/windowsRS1 label Nov 28, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Nov 28, 2016

AkihiroSuda added this to the 1.14.0 milestone Nov 28, 2016

AkihiroSuda reviewed Nov 28, 2016

View reviewed changes

api/server/router/image/image_routes.go Outdated

Copy link
Copy Markdown

Member

AkihiroSuda Nov 28, 2016

Choose a reason for hiding this comment

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

nit: how about Filters: pruneFilters

AkihiroSuda removed this from the 1.14.0 milestone Nov 28, 2016

AkihiroSuda requested changes Nov 28, 2016

View reviewed changes

AkihiroSuda reviewed Nov 28, 2016

View reviewed changes

yongtang force-pushed the 28497-prune-until branch from 778d074 to d259199 Compare November 30, 2016 00:00

yongtang force-pushed the 28497-prune-until branch 2 times, most recently from 03c8104 to 5426403 Compare December 3, 2016 04:05

yongtang force-pushed the 28497-prune-until branch from 5426403 to a6be56b Compare December 3, 2016 04:09

mlaventure approved these changes Dec 5, 2016

View reviewed changes

cpuguy83 approved these changes Dec 5, 2016

View reviewed changes

vdemeester approved these changes Dec 6, 2016

View reviewed changes

vdemeester merged commit 745795e into moby:master Dec 6, 2016

yongtang deleted the 28497-prune-until branch December 6, 2016 15:34

yongtang mentioned this pull request Dec 7, 2016

Add --filter until=<timestamp> for docker container/image prune #29226

Merged

vieux added process/cherry-picked and removed process/cherry-pick labels Dec 8, 2016

vieux mentioned this pull request Dec 8, 2016

1.13.0-rc4 cherry-picks: part1 #29229

Merged

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28535 from yongtang/28497-prune-until

652b457

Convert DanglingOnly to Filters for `docker image prune`

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28535 from yongtang/28497-prune-until

7ca372f

Convert DanglingOnly to Filters for `docker image prune`

fcrisciani mentioned this pull request Nov 16, 2017

[Backport 17.03] Adding ipam options to ipam driver requests moby/swarmkit#2449

Closed

Conversation

yongtang commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Nov 17, 2016

Uh oh!

thaJeztah commented Nov 18, 2016

Uh oh!

thaJeztah commented Nov 18, 2016

Uh oh!

AkihiroSuda commented Nov 18, 2016

Uh oh!

vdemeester commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justincormack commented Nov 18, 2016

Uh oh!

mlaventure commented Nov 18, 2016

Uh oh!

yongtang commented Nov 18, 2016

Uh oh!

yongtang commented Nov 20, 2016

Uh oh!

mlaventure commented Nov 20, 2016

Uh oh!

thaJeztah commented Nov 21, 2016

Uh oh!

mlaventure commented Nov 21, 2016

Uh oh!

yongtang commented Nov 27, 2016

Uh oh!

AkihiroSuda Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 28, 2016

1.13

1.14

Uh oh!

mlaventure commented Nov 28, 2016

Uh oh!

thaJeztah commented Nov 28, 2016

Uh oh!

yongtang commented Nov 28, 2016

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

nicosingh commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

yongtang commented Nov 17, 2016 •

edited

Loading

vdemeester commented Nov 18, 2016 •

edited

Loading

AkihiroSuda Nov 28, 2016 •

edited

Loading