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

bugfix: we should get image ID from containerd #1112

Merged
merged 1 commit into from
Apr 15, 2018

Conversation

YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Apr 12, 2018

Signed-off-by: YaoZengzeng yaozengzeng@zju.edu.cn

Ⅰ. Describe what this PR did

Now we get image ID by calculating ourself, unfortunately it's wrong.

For example busybox:1-uclibc and busybox:1 should have same ID, however:

[root@pouch-test pouch]# pouch images
IMAGE ID       IMAGE NAME                                         SIZE
58af67620523   registry.hub.docker.com/library/busybox:1          710.83 KB
b2e60002262d   registry.hub.docker.com/library/busybox:1-uclibc   709.90 KB

So with this PR we get it directly from containerd then:

[root@pouch-test pouch]# pouch images
IMAGE ID       IMAGE NAME                                         SIZE
8ac48589692a   registry.hub.docker.com/library/busybox:1          710.83 KB
8ac48589692a   registry.hub.docker.com/library/busybox:1-uclibc   709.90 KB

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/M labels Apr 12, 2018
@codecov-io
Copy link

codecov-io commented Apr 12, 2018

Codecov Report

Merging #1112 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1112      +/-   ##
==========================================
- Coverage   16.66%   16.66%   -0.01%     
==========================================
  Files         165      165              
  Lines        8911     8918       +7     
==========================================
+ Hits         1485     1486       +1     
- Misses       7321     7326       +5     
- Partials      105      106       +1
Impacted Files Coverage Δ
daemon/mgr/cri.go 0% <0%> (ø) ⬆️
ctrd/image.go 0% <0%> (ø) ⬆️
client/client.go 25% <0%> (-11.37%) ⬇️
pkg/kernel/kernel.go 72.72% <0%> (-7.28%) ⬇️
cli/command.go 0% <0%> (ø) ⬆️

@YaoZengzeng
Copy link
Contributor Author

@allencloud PTAL

Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>
@allencloud
Copy link
Collaborator

I think we need to support tag image, and add an integration test then. @YaoZengzeng

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 15, 2018
@allencloud allencloud merged commit 611837e into AliyunContainerService:master Apr 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants