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

add digest sub-flag for list image #163

Merged
merged 1 commit into from
Oct 28, 2017

Conversation

yanxuean
Copy link
Contributor

fix #162
Signed-off-by: yanxuean [email protected]

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2017
@yanxuean
Copy link
Contributor Author

test case

root@ubuntu:/home/cloud/go/src/github.com/kubernetes-incubator/cri-tools# crictl images --digests
DEBU[0000] ListImagesRequest: &ListImagesRequest{Filter:&ImageFilter{Image:&ImageSpec{Image:,},},} 
DEBU[0000] ListImagesResponse: &ListImagesResponse{Images:[&Image{Id:sha256:54511612f1c4d97e93430fc3d5dc2f05dfbe8fb7e6259b7351deeca95eaf2971,RepoTags:[docker.io/library/busybox:latest],RepoDigests:[],Size_:719904,Uid:nil,Username:,} &Image{Id:sha256:b8d7834a2df3a323807d0b97aba3230c2ed7f41f990c971ebf78442649d814aa,RepoTags:[],RepoDigests:[docker.io/library/postgres@sha256:318757ed6291e6a1ef86312ac453b9b4a67b48495b59ca2dece909cb0c688c53],Size_:104133212,Uid:nil,Username:,} &Image{Id:sha256:320aedbfaad3d719b5e6a711834d2d06e2b0a172ce6fdadf84c640b6136914ed,RepoTags:[gcr.io/google_containers/pause:3.0],RepoDigests:[],Size_:312194,Uid:nil,Username:,} &Image{Id:sha256:c30178c5239f2937c21c261b0365efcda25be4921ccb95acd63beeeb78786f27,RepoTags:[docker.io/library/busybox:1.26],RepoDigests:[docker.io/library/busybox@sha256:be3c11fdba7cfe299214e46edc642e09514dbb9bbefcd0d3836c05a1e0cd0642],Size_:701277,Uid:nil,Username:,}],} 
IMAGE                            TAG                 DIGEST                                                                    IMAGE ID            SIZE
docker.io/library/busybox        latest              <none>                                                                    54511612f1c4d       720kB
docker.io/library/postgres       <none>              sha256:318757ed6291e6a1ef86312ac453b9b4a67b48495b59ca2dece909cb0c688c53   b8d7834a2df3a       104MB
gcr.io/google_containers/pause   3.0                 <none>                                                                    320aedbfaad3d       312kB
docker.io/library/busybox        1.26                sha256:be3c11fdba7cfe299214e46edc642e09514dbb9bbefcd0d3836c05a1e0cd0642   c30178c5239f2       701kB

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2017
@yanxuean
Copy link
Contributor Author

/assign feiskyer
/assign random-liu

@yanxuean
Copy link
Contributor Author

@feiskyer @Random-Liu PTAL

@yanxuean
Copy link
Contributor Author

@Random-Liu @feiskyer PTAL

}
repoDigest := normalizeRepoDigest(image.RepoDigests)
repoTagPairs := normalizeRepoTagPair(image.RepoTags, image.RepoDigests)
size := units.HumanSizeWithPrecision(float64(image.GetSize_()), 3)
trunctedImage := strings.TrimPrefix(image.Id, "sha256:")[:truncatedImageIDLen]
for _, repoTagPair := range repoTagPairs {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should combine repoDigest list and repoTag list, so that we don't need to handle repo digest separately in normalizeRepoTagPair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2017
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 23, 2017
@yanxuean
Copy link
Contributor Author

@Random-Liu PTAL

@Random-Liu
Copy link
Contributor

Random-Liu commented Oct 26, 2017

@yanxuean

  1. If we have an image that has both repo tag a:b and repo digest a@sha256:c, I guess docker images shows a b and docker images --digest shows a b c.
  2. If we have an image that only has repo digest a@sha256:c, I guess docker images shows a <none> and docker images --digest shows a <none> c.
  3. If we have an image that has 2 repo tags a:b1 and a:b2 and repo digest a@sha256:c, I guess docker images shows a b1 and a b1, and docker images --digest shows a b1 c and a b2 c.
  4. Will an image have 2 repo digests? (I don't think it will happen in anytime soon, so I think we could ignore this case for now and assume an image only has one repo digest).

If my assumption above is correct, then it seems that 2) is not handled in this PR. @yanxuean Please first make sure my assumption is correct, I'm not sure about it actually. :)

We only need to:

  1. pass both repoTags and repoDigests into a function.
  2. The function should return a list of (id, tag, digest).
  3. If the image has repoTags, then the list only need to contain (id, tag, digest) for all repoTags.
  4. If the image doesn't have repoTags, then the list should contain (id, <none>, digest) for repoDigests.

@feiskyer
Copy link
Member

@Random-Liu Could you find some example images to convert those cases?

@Random-Liu
Copy link
Contributor

Random-Liu commented Oct 27, 2017

Note that I removed all busybox image before each case.

Case 1:

$ docker pull busybox:latest
$ docker pull busybox@sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7
$ docker images | grep busybox
busybox                                                     latest                               54511612f1c4        6 weeks ago         1.129 MB
$ docker images --digest | grep busybox
busybox                                                     latest                               sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7   54511612f1c4        6 weeks ago         1.129 MB

Case 2:

$ docker pull busybox@sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7
$ docker images | grep busybox
busybox                                                     <none>                               54511612f1c4        6 weeks ago         1.129 MB
$ docker images --digests | grep busybox
busybox                                                     <none>                               sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7   54511612f1c4        6 weeks ago         1.129 MB

Case 3:

$ docker pull busybox:latest
$ docker pull busybox@sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7
$ docker pull busybox:1.27.2
$ docker images | grep busybox
busybox                                                     1.27.2                               54511612f1c4        6 weeks ago         1.129 MB
busybox                                                     latest                               54511612f1c4        6 weeks ago         1.129 MB
$ docker images --digests | grep busybox
busybox                                                     1.27.2                               sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7   54511612f1c4        6 weeks ago         1.129 MB
busybox                                                     latest                               sha256:3e8fa85ddfef1af9ca85a5cfb714148956984e02f00bec3f7f49d3925a91e0e7   54511612f1c4        6 weeks ago         1.129 MB

In this PR case 2 is not handled.

As I mentioned, we should do:

  • Pass both repoTags and repoDigests into a function.
  • The function should return a list of (id, tag, digest).
  • If the image has repoTags, then the list only need to contain (id, tag, digest) for all repoTags.
  • If the image doesn't have repoTags, then the list should contain (id, , digest) for repoDigests.

@yanxuean
Copy link
Contributor Author

ok, I will study it late.

@yanxuean
Copy link
Contributor Author

The case 2 has been handled.
see the second case below:

root@ubuntu:/home/cloud/go/src/github.com/kubernetes-incubator/cri-tools# crictl images --digests
DEBU[0000] ListImagesRequest: &ListImagesRequest{Filter:&ImageFilter{Image:&ImageSpec{Image:,},},} 
DEBU[0000] ListImagesResponse: &ListImagesResponse{Images:[&Image{Id:sha256:54511612f1c4d97e93430fc3d5dc2f05dfbe8fb7e6259b7351deeca95eaf2971,RepoTags:[docker.io/library/busybox:latest],RepoDigests:[],Size_:719904,Uid:nil,Username:,} &Image{Id:sha256:b8d7834a2df3a323807d0b97aba3230c2ed7f41f990c971ebf78442649d814aa,RepoTags:[],RepoDigests:[docker.io/library/postgres@sha256:318757ed6291e6a1ef86312ac453b9b4a67b48495b59ca2dece909cb0c688c53],Size_:104133212,Uid:nil,Username:,} &Image{Id:sha256:320aedbfaad3d719b5e6a711834d2d06e2b0a172ce6fdadf84c640b6136914ed,RepoTags:[gcr.io/google_containers/pause:3.0],RepoDigests:[],Size_:312194,Uid:nil,Username:,} &Image{Id:sha256:c30178c5239f2937c21c261b0365efcda25be4921ccb95acd63beeeb78786f27,RepoTags:[docker.io/library/busybox:1.26],RepoDigests:[docker.io/library/busybox@sha256:be3c11fdba7cfe299214e46edc642e09514dbb9bbefcd0d3836c05a1e0cd0642],Size_:701277,Uid:nil,Username:,}],} 
IMAGE                            TAG                 DIGEST                                                                    IMAGE ID            SIZE
docker.io/library/busybox        latest              <none>                                                                    54511612f1c4d       720kB
docker.io/library/postgres       <none>              sha256:318757ed6291e6a1ef86312ac453b9b4a67b48495b59ca2dece909cb0c688c53   b8d7834a2df3a       104MB
gcr.io/google_containers/pause   3.0                 <none>                                                                    320aedbfaad3d       312kB
docker.io/library/busybox        1.26                sha256:be3c11fdba7cfe299214e46edc642e09514dbb9bbefcd0d3836c05a1e0cd0642   c30178c5239f2       701kB

@Random-Liu
Copy link
Contributor

@yanxuean Sorry, I missed that part. Then we are handling all the cases I could come up with. Thanks!

@Random-Liu Random-Liu merged commit 8e989f8 into kubernetes-sigs:master Oct 28, 2017
@yanxuean yanxuean deleted the digest branch October 30, 2017 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support to show digest for imagelist
4 participants