Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image/list: Add --tree flag #4982

Merged
merged 7 commits into from
Aug 16, 2024
Merged

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Apr 4, 2024

Add --tree flag to docker image list

image

Update:
Added some spacing around images:
image

- Description for the changelog

- containerd image store:  `docker image ls` now supports `--tree` flag that shows a multiplatform-aware image list. This is experimental and may change at any time without any backwards compatibility.

@vvoland vvoland added impact/changelog area/api area/ux containerd-integration Issues and PRs related to containerd integration labels Apr 4, 2024
@vvoland vvoland self-assigned this Apr 4, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 1.49254% with 198 lines in your changes missing coverage. Please review.

Project coverage is 60.94%. Comparing base (bbce5a0) to head (a9b78da).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4982      +/-   ##
==========================================
- Coverage   61.51%   60.94%   -0.57%     
==========================================
  Files         303      304       +1     
  Lines       21130    21331     +201     
==========================================
+ Hits        12998    13001       +3     
- Misses       7203     7400     +197     
- Partials      929      930       +1     

func runTree(ctx context.Context, dockerCLI command.Cli, opts treeOptions) error {
images, err := dockerCLI.Client().ImageList(ctx, imagetypes.ListOptions{
All: opts.all,
ContainerCount: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think we have the per-image list of containers now, correct? In that case; do we still need that PR? I was always a slight bit on the fence on that one, as only a number of containers didn't provide a ton of useful information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have, but it's only filled if the ContainerCount option is set (we can still change that though).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I remove the Usage column for now?

@vvoland vvoland force-pushed the c8d-multiplatform-list branch 2 times, most recently from 4662670 to 6ec9439 Compare June 5, 2024 10:15
@vvoland vvoland force-pushed the c8d-multiplatform-list branch 2 times, most recently from cd11dc8 to 9cf3aac Compare June 20, 2024 18:54
@vvoland
Copy link
Collaborator Author

vvoland commented Jul 18, 2024

Combined multiple images with the same target under one entry:

tree.mp4

cc @jalonsogo

@vvoland vvoland force-pushed the c8d-multiplatform-list branch 2 times, most recently from c0aec20 to 2210865 Compare July 18, 2024 13:06
@vvoland vvoland force-pushed the c8d-multiplatform-list branch 2 times, most recently from 44c9a10 to 0fd1cd8 Compare August 6, 2024 11:30
@vvoland vvoland added this to the 28.0.0 milestone Aug 6, 2024
cli/command/image/list.go Outdated Show resolved Hide resolved
cli/command/image/list.go Outdated Show resolved Hide resolved
cli/command/image/tree.go Outdated Show resolved Hide resolved
@vvoland vvoland force-pushed the c8d-multiplatform-list branch 2 times, most recently from af28277 to 2b46c60 Compare August 6, 2024 15:35
Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vvoland
Copy link
Collaborator Author

vvoland commented Aug 14, 2024

Also, perhaps we could already include the Content size column to start making users aware of the distinction?

image

@laurazard
Copy link
Member

Whole columns left-aligned looks better to me! (but that's subjective, I'm just casting my vote here)
(this image)

Add tree flag

I'm also a +1 on adding content size, I think this is a great place to introduce this without breaking anything – and users will already be expecting new things here so less confusing.

@vvoland
Copy link
Collaborator Author

vvoland commented Aug 14, 2024

So, I just bugged @jalonsogo for advice, and got a couple of guildelines to follow, tldr:

  • Number aligned right
  • Text aligned left
  • Header and cell content alignment should match

and ended up with:
image

I made Used centered as it can only either be empty or ✓. But that's 4 letters so isn't perfectly centered 🙈

@laurazard
Copy link
Member

Nice! I didn't know about right-alined numbers, but it makes sense – this way, the decimal places are aligned, which also makes it easier to parse the table and find larger numbers

@vvoland vvoland requested a review from thaJeztah August 14, 2024 17:45
@thaJeztah
Copy link
Member

So, I just bugged @jalonsogo for advice, and got a couple of guildelines to follow, tldr:

  • Number aligned right
  • Text aligned left
  • Header and cell content alignment should match

I made Used centered as it can only either be empty or ✓. But that's 4 letters so isn't perfectly centered 🙈

Ah, yes, I like that variant!

The numbers have always been a bit tricky, and I know we (still) have some bugs in those; the intent was to have a fixed number of decimals so that values align (and pick the most appropriate unit to make that happen), but there's still some bugs there (see #3091)

@thaJeztah
Copy link
Member

OK; one more thing I noticed (but maybe something to also fix on the daemon side not sure?) is the order in which variants are shown;

Output with only arm64;

docker image ls --tree
Image                        ID        Disk usage   Content size   Used
alpine:latest           0a4eaa0eecf5     13.6MB        4.09MB
├─ linux/arm64/v8       24ba417e25e7     13.6MB        4.09MB
├─ linux/amd64          eddacbc7e24b       0B            0B
├─ linux/arm/v6         5c7e326e3c8a       0B            0B
├─ linux/arm/v7         fda9b1b812b2       0B            0B
├─ linux/386            fa66aa594ffa       0B            0B
├─ linux/ppc64le        a01843eb870e       0B            0B
├─ linux/riscv64        e99a4d9aa9f9       0B            0B
└─ linux/s390x          14da06d3a895       0B            0B

Pull s390x;

docker image pull --platform=linux/s390x alpine

New output;

docker image ls --tree
Image                        ID        Disk usage   Content size   Used
alpine:latest           0a4eaa0eecf5     25.5MB        7.55MB
├─ linux/s390x          14da06d3a895     11.9MB        3.46MB
├─ linux/arm64/v8       24ba417e25e7     13.6MB        4.09MB
├─ linux/amd64          eddacbc7e24b       0B            0B
├─ linux/arm/v6         5c7e326e3c8a       0B            0B
├─ linux/arm/v7         fda9b1b812b2       0B            0B
├─ linux/386            fa66aa594ffa       0B            0B
├─ linux/ppc64le        a01843eb870e       0B            0B
└─ linux/riscv64        e99a4d9aa9f9       0B            0B

I wondered where the sort-order is based on; perhaps sorting is happening on "Created" date, but it looks like we omit that in the new view;

docker image ls
REPOSITORY                  TAG               IMAGE ID       CREATED         SIZE
docker-dev                  latest            e1ca8a3ea183   6 hours ago     2.2GB
localhost:5001/owner/name   latest            0fea7086b9c0   8 hours ago     17.2MB
docker-cli-dev              latest            88c125e27aee   3 days ago      749MB
golang                      1.22              2bd56f00ff47   8 days ago      1.21GB
moby/buildkit               buildx-stable-1   0a5641c72659   3 weeks ago     298MB
docker                      dind              a69069397655   3 weeks ago     491MB
alpine                      latest            0a4eaa0eecf5   3 weeks ago     25.7MB
registry                    3.0.0-beta.1      efb271da5cd0   5 weeks ago     61.8MB
nginx                       alpine            208b70eefac1   7 weeks ago     68.4MB
registry                    2                 12120425f07d   10 months ago   36.1MB
docker                      20.10-cli         2967f0819c84   15 months ago   209MB
registry                    2.7               169211e20e2f   2 years ago     35.4MB
registry                    2.6               c4bdca23bab1   4 years ago     38.5MB

Maybe we should

  • have the variants sorted by the first column
  • the overall list of images could still be (as default) sorted by created date
  • ☝️ that order can sometimes be relevant (e.g. cut the last X images)
  • ☝️ BUT also admitted that "created" date may be different with containerd (IIRC it uses the "local" date the image was added, not when the image was built not sure?)
  • ☝️ Even if it shows the "created" date ("built" date); reproducible builds make that ... less useful (1-1-1970 or something along those lines 😂)

Final thought; some what related to moby/moby#48264 (comment) - wondering if we should have --all (or some equivalent) to control whether "non available" variants should be shown / hidden.

  • It's good to have the option to show all variants of the image (including those "not present")
  • At the same time, it makes the list really long, and sometimes you may only want to see "what's present"
  • ☝️ for that we'd have to decide on a default

@vvoland
Copy link
Collaborator Author

vvoland commented Aug 16, 2024

Variants are sorted by the "availability" so that present variants are always on top.
Added sorting the top level images by the created (like the regular list does). Not showing it though for now.
I think we can revisit sorting and all to show the unavailable platform in a follow-up.

@vvoland
Copy link
Collaborator Author

vvoland commented Aug 16, 2024

Also, wondering if we need an additional warning like:

image

Comment on lines +63 to +64
// Mark top-level parent image as used if any of its subimages are used.
details.Used = true
Copy link
Member

Choose a reason for hiding this comment

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

I forgot now; did we also add the list of container-id's that used it somewhere? (Maybe not to be printed by default for sure)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the ImageData has a list of containers. It's only available in the manifest summary though.
Also, see: #4982 (comment)

Copy link
Contributor

@krissetto krissetto left a comment

Choose a reason for hiding this comment

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

i agree with @laurazard that maybe some things could be refactored, but looks like a good start to me :)

One design consideration though: I tend to think that some whitespace could go a long way in making the output easier to parse (since this is for humans more than for machines, right?). For example when using the command a couple times i find it hard to visually parse the beginning of the output from the command that the user typed, and also to read the column titles from the image names listed since they're all back to back (and title+first image name are both bold).

Maybe to separate column titles from the column contents they could:

  • have some whitespace around them, separating them from contents and the command the user typed;
  • be a different color;
  • have a line under them like an old school table;

Just throwing some thoughts out there. WDYT here @jalonsogo? Maybe some discussions have already been made around this, if so feel free to ignore this comment :)

Given the experimental nature, we can definitely iterate on these things. It'd also be nice to consolidate our design and usage of columns/tables etc amongst all our other commands as well

This command has nice (imho) whitespace in it's output, for example

Screenshot 2024-08-16 at 12 52 32

docs/reference/commandline/image_ls.md Outdated Show resolved Hide resolved
@vvoland
Copy link
Collaborator Author

vvoland commented Aug 16, 2024

Initially there was a newline between header and the first item, but removed it after a small discussion here: #4982 (comment)

Regarding the newline before the command content.. I think it might work:
image

@vvoland
Copy link
Collaborator Author

vvoland commented Aug 16, 2024

Removed the Used column for now, as it won't be populated correctly without moby/moby#47501

EDIT: Also an alternative PR that allows the Used column to work: moby/moby#48345
EDIT2: moby/moby#48345 got merged so restored the Used column

@jalonsogo
Copy link

In the CLI design guidelines, I recommend using capitalization+bold for the headers.
Also, in my designs, I use them in combination with colors:

  • Bright White \u001b[37m;1m for the primary chunks of information
  • Write-default \u001b[37m for the secondary chucks of information

CleanShot 2024-08-16 at 13 28 15

@vvoland
Copy link
Collaborator Author

vvoland commented Aug 16, 2024

Restored the new-line and capitalized column headers:

image

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@laurazard laurazard merged commit cab8a2e into docker:master Aug 16, 2024
89 checks passed
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.

None yet

8 participants