Skip to content

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Dec 8, 2023

c8d/prune: Default dangling filter to true

If no dangling filter is specified, prune should only delete dangling
images.

This wasn't visible by doing docker image prune because the CLI
explicitly sets this filter to true.

c8d/prune: Familiarize image names that were untagged

To align with the graphdriver implementation.

Signed-off-by: Paweł Gronowski pawel.gronowski@docker.com

- How to verify it
docker-py tests.integration.api_image_test.PruneImagesTest should pass now

- Description for the changelog

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

If no `dangling` filter is specified, prune should only delete dangling
images.

This wasn't visible by doing `docker image prune` because the CLI
explicitly sets this filter to true.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
To align with the graphdriver implementation.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added status/2-code-review area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Dec 8, 2023
@vvoland vvoland added this to the 25.0.0 milestone Dec 8, 2023
@vvoland vvoland self-assigned this Dec 8, 2023
@thaJeztah
Copy link
Member

This wasn't visible by doing docker image prune because the CLI explicitly sets this filter to true.

Did this also fail with the graph-driver? (from the above, it sounds like it's an existing bug, is that correct?)

@thaJeztah
Copy link
Member

Ah, never mind I see the graph-driver implementation has the default set correctly already;

danglingOnly, err := pruneFilters.GetBoolOrDefault("dangling", true)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@vvoland for a follow-up, should we remove that default from the CLI? (tempted to say that defaults should be either on the CLI or on the daemon, not "both"

@thaJeztah
Copy link
Member

Guessing that this is the offending line https://github.com/docker/cli/blob/623d9b1f68171e3ff9899b21392ac4f102ba533b/cli/command/image/prune.go#L63

	pruneFilters.Add("dangling", strconv.FormatBool(!options.all))

Which means we unconditionally set the dangling filter, but depending on --all, we set it to either true or false

@thaJeztah thaJeztah merged commit 312fbb6 into moby:master Dec 8, 2023
@vvoland
Copy link
Contributor Author

vvoland commented Dec 11, 2023

I think that the current CLI behavior is correct - if the image prune (without --all) is expected to only prune the dangling images, it should explicitly pass dangling=true instead of assuming that server will also default to true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/images Image Distribution containerd-integration Issues and PRs related to containerd integration status/2-code-review
Projects
Development

Successfully merging this pull request may close these issues.

2 participants