formatter: Consider empty RepoTags and RepoDigests as dangling#4046
Conversation
cf08fb0 to
7ae1bb3
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4046 +/- ##
=======================================
Coverage 59.16% 59.16%
=======================================
Files 287 287
Lines 24724 24728 +4
=======================================
+ Hits 14627 14631 +4
Misses 9212 9212
Partials 885 885 |
| if cli.imageListFunc != nil { | ||
| return cli.imageListFunc(options) | ||
| } | ||
| return []types.ImageSummary{{}}, nil |
There was a problem hiding this comment.
So yeah, this fixes the tests, but I'm not sure what was the reason behind this.
With the original changes, a completely empty ImageSummary is also considered a dangling image, which causes it to appear in the docker images output - but obviously it's not a valid image.
Was there a reason this is was not a plain empty slice from the beginning?
Wondering if the fake ImageList call not creating a completely empty slice was a deliberate choice?
Can the server really output empty ImageSummary and the listing code should ignore it?
\cc @thaJeztah
There was a problem hiding this comment.
Arf.. was writing up a comment, but didn't put it here. Hold on
There was a problem hiding this comment.
Yes, I agree, this seems like it was not intentional (or at least I could not find a direct reason).
Looks like all of these were added as part of the same commit (e779309), that commit was migrated from the moby/moby repository; original one was moby/moby@b2551c6 (moby/moby#32248)
The only possible reasoning I could think of was if it was some attempt to match that the API returns always returns a slice (even if there's no images); the response of the API (tested on docker 17.06 to be sure) is;
curl --unix-socket /var/run/docker.sock 'http://foo/images/json'
[]
But that also doesn't match, because it doesn't return (e.g.) [{}] (single entry).
Other explanation could be to work with the assertion library used at the time; there's a mention that we switched libraries for that at the time; moby/moby#32248 (comment) so perhaps the old one had some issue?
|
I always get confused on the “ordering” of commits on GitHub;
GitHub shows the first (?) commit as failing currently; |
03afc0b to
d77ee1b
Compare
|
Yeah I always get confused about the order too 😄 The order is just a consequence of the fact that I first pushed the empty RepoTags/RepoDigests change and then fixed the failing tests as another commit. I extracted the test change: #4050 and will rebase this one. |
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
d77ee1b to
89687d5
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thanks!
could you prepare cherry-pick(s) for this one as well?

From API 1.43 version
RepoTagsandRepoDigestswill be empty for dangling images, instead of being an one-item array with magic constant string (<none>:<none>/<none>@<none>).CLI output is not impacted and will still display
<none>strings.- What I did
Consider images with empty
RepoTagsandRepoDigestsas dangling.- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)