Skip to content

Conversation

@vvoland
Copy link
Collaborator

@vvoland vvoland commented Feb 23, 2023

From API 1.43 version RepoTags and RepoDigests will 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 RepoTags and RepoDigests as dangling.

- How I did it

- How to verify it

- Description for the changelog

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

@vvoland vvoland added this to the v-next milestone Feb 23, 2023
@vvoland vvoland force-pushed the dangling-images-none branch 2 times, most recently from cf08fb0 to 7ae1bb3 Compare February 23, 2023 15:13
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Merging #4046 (99ad93b) into master (dfb36ea) will increase coverage by 0.00%.
The diff coverage is 0.00%.

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           

@vvoland vvoland marked this pull request as draft February 23, 2023 15:19
if cli.imageListFunc != nil {
return cli.imageListFunc(options)
}
return []types.ImageSummary{{}}, nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Arf.. was writing up a comment, but didn't put it here. Hold on

Copy link
Member

Choose a reason for hiding this comment

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

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?

@vvoland vvoland marked this pull request as ready for review February 24, 2023 11:22
@thaJeztah
Copy link
Member

I always get confused on the “ordering” of commits on GitHub;

  • so the “test fix” commit was needed to make CI pass after the other commit?
  • if that's the case, then the commits should probably be in the reverse order
  • but maybe "cleaner" to open a separate PR with only the "test fix" commit to show that it (in itself) does not make CI fail?

GitHub shows the first (?) commit as failing currently;

Screenshot 2023-02-24 at 14 21 53

@vvoland vvoland force-pushed the dangling-images-none branch 2 times, most recently from 03afc0b to d77ee1b Compare February 24, 2023 14:05
@vvoland
Copy link
Collaborator Author

vvoland commented Feb 24, 2023

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>
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, thanks!

could you prepare cherry-pick(s) for this one as well?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants