Skip to content

c8d/list: Support dangling image filter#128

Merged
rumpl merged 1 commit intorumpl:c8dfrom
vvoland:c8d-list-dangling-filter
Jan 23, 2023
Merged

c8d/list: Support dangling image filter#128
rumpl merged 1 commit intorumpl:c8dfrom
vvoland:c8d-list-dangling-filter

Conversation

@vvoland
Copy link
Collaborator

@vvoland vvoland commented Jan 23, 2023

Implements dangling filter, which has been mentioned a lot in customer feedbacks.

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

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland requested review from rumpl and thaJeztah January 23, 2023 09:03
@vvoland vvoland changed the title c8d/list: Support dangling filter c8d/list: Support dangling image filter Jan 23, 2023
return nil, err
}

danglingFilter := opts.Filters.Contains("dangling")
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The behavior is the same actually - dangling=false is not equivalent to empty dangling.

dangling=false <- list only non-dangling images
dangling=true <- list only dangling
dangling= <- list all images regardless if they're dangling or not

Copy link
Owner

Choose a reason for hiding this comment

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

Someone will ask you to make it the same eventually, might as well do it now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But it's already the same 😄

It doesn't look the same, looking at the code you linked, because old ImageService code doesn't handle it in one place. First it evaluates the danglingOnly, so it looks like only dangling=true does anything meaningful, but then checks the dangling flag on every image separately: https://github.com/rumpl/moby/blob/master/daemon/images/image_list.go#L155

Or are you talking about something else?

danglingValue = false
} else {
err := fmt.Errorf("invalid filter 'dangling=%s'", opts.Filters.Get("dangling"))
return nil, errdefs.InvalidParameter(err)
Copy link
Owner

Choose a reason for hiding this comment

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

Should this really return errdefs? The graph driver one doesn't, it might be a good idea to extract the invalidFilter from the package and reuse?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

invalidFilter implements InvalidParameter so graph driver still indirectly returns an errdefs.InvalidParameter in this case.

As for invalidFilter - I'd like to get rid of it in this PR: moby#44003. This would allow to avoid manual creation of this error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants