Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Rework docker app image ls and add the --digests flag #709

Merged
merged 3 commits into from
Nov 6, 2019

Conversation

zappy-shu
Copy link
Contributor

@zappy-shu zappy-shu commented Oct 24, 2019

- WIP
Need to get an e2e or unit test that covers the digest.

- What I did

Reworked the image ls output to better match the docker image ls command. The output now has separate columns for REPOSITORY and TAG as well as an APP IMAGE ID column.

Added the --digests flag to image ls. This makes the image ls command print the image digests (if present) along with the other columns.

- How I did it

Made the columns list for image ls dynamic depending on what options are sent.
The digest is retrieved by attempted to cast to a Digested type (i.e. the image has been stored by digest rather than by tag or ID). If not then just prints <none>.

- How to verify it

Pull an image from hub by digest and then run app image ls --digest. The image in question should show the digest prefixed by the algorithm type. E.g.

REPOSITORY   TAG    DIGEST          APP IMAGE ID APP NAME
<repository> <none> sha256@<digest> <id>         <name>

- Description for the changelog

Added the --digests flag to the app image ls command. When present, the command will print the image's digest or <none> if the image does not have a digest, along with other image details.

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

pp

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #709 into master will increase coverage by 0.1%.
The diff coverage is 86.36%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #709     +/-   ##
========================================
+ Coverage    71.4%   71.5%   +0.1%     
========================================
  Files          59      59             
  Lines        3175    3201     +26     
========================================
+ Hits         2267    2289     +22     
- Misses        602     604      +2     
- Partials      306     308      +2
Impacted Files Coverage Δ
internal/commands/image/list.go 83.67% <86.36%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9042ab2...24ced43. Read the comment docs.

e2e/images_test.go Outdated Show resolved Hide resolved
a-simple-app:latest <none> simple
b-simple-app:latest <none> simple
`
expectedOutput := fmt.Sprintf(expected, info.registryAddress+"/c-myapp:latest")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an image by digest to check the digest column has something else than <none>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I wanted to pair with you to figure out how to do

Copy link
Member

Choose a reason for hiding this comment

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

At one point the insertBundle function pushed and pulled an image by digest, you can see it here: https://github.com/docker/app/pull/648/files#diff-d8de3905e4eb053a6e7c6a6bda133499R23

You can reuse this kind of logic to get a digested output.

@zappy-shu zappy-shu force-pushed the add-digest-flag-to-ls branch 3 times, most recently from c64589c to c5d0f5e Compare October 25, 2019 11:06
@zappy-shu zappy-shu changed the title WIP Add --digests flag to image ls WIP Rework docker app image ls and add the --digests flag Oct 29, 2019
@zappy-shu zappy-shu force-pushed the add-digest-flag-to-ls branch 7 times, most recently from a64a013 to ea4d7d7 Compare October 30, 2019 09:03
@zappy-shu zappy-shu changed the title WIP Rework docker app image ls and add the --digests flag Rework docker app image ls and add the --digests flag Oct 30, 2019
Added the --digests flag to image ls. This makes the image ls command
print the image digests (if present) along with the app name and image.

Signed-off-by: Nick Adcock <nick.adcock@docker.com>
Removed the temporary directories from being created in the app image ls
tests as they are not used.

Signed-off-by: Nick Adcock <nick.adcock@docker.com>
Split the app image ls output up so that the REPOSITORY and TAG are
separate columns, the same as the standard docker image ls.

The APP IMAGE ID column is now a default column, same as the ID column
in docker image ls.

As this ID is randomly generated the e2e tests have
been updated to use regexes when asserting the command output.

Signed-off-by: Nick Adcock <nick.adcock@docker.com>
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@chris-crone
Copy link
Member

chris-crone commented Nov 6, 2019

Is the spacing between the columns the same as for container images? From the following, It looks to me like app has less spaces which makes it tricky to read.

➜  app git:(add-digest-flag-to-ls) docker app image ls
REPOSITORY       TAG  APP IMAGE ID APP NAME
ccrone/cnab-demo yves 6cca8f6428a9 coins
ccrone/cnab-demo yves 6cca8f6428a9 coins
➜  app git:(add-digest-flag-to-ls) docker images
REPOSITORY             TAG                           IMAGE ID            CREATED             SIZE
docker/cnab-app-base   v0.9.0-zeta1-66-g24ced43b0d   f5d2426e6a7f        2 minutes ago       47.7MB
ccrone/cnab-demo       <none>                        51a16e185d83        59 minutes ago      47.2MB
ccrone/cnab-demo       <none>                        7cc9617f5c36        About an hour ago   218MB
ccrone/cnab-demo       <none>                        de25a81a5a0b        2 weeks ago         98.2MB

@zappy-shu
Copy link
Contributor Author

@chris-crone

I haven't touched the spacing as part of this PR but the tabwriter for the CLI does have different settings to the one used here:

cli := tabwriter.NewWriter(c.Output, 20, 1, 3, ' ', 0)
app := tabwriter.NewWriter(dockerCli.Out(), 0, 0, 1, ' ', 0)

Do we want to update the app tabwriter so the spacing match the CLI?
Note that the CLI uses a generic formatter so we will probably need to change it in multiple places to completely match. Probably best do the others as part of a separate PR though

@chris-crone
Copy link
Member

Do we want to update the app tabwriter so the spacing match the CLI?

Yes. That will make the feel the same.

Probably best do the others as part of a separate PR though

👍 Please just create a ticket to track

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

@chris-crone chris-crone merged commit 8acd0a8 into docker:master Nov 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants