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: add default tag ":latest" when using pouch rmi to remove untagged container images. #1191

Merged
merged 1 commit into from
Apr 26, 2018

Conversation

xiechengsheng
Copy link
Contributor

@xiechengsheng xiechengsheng commented Apr 23, 2018

Ⅰ. Describe what this PR did

add default tag ":latest" when using pouch rmi to remove untagged container images.

Ⅱ. Does this pull request fix one issue?

fixes #1104

Ⅲ. Describe how you did it

just use the same way as pouch pull

Ⅳ. Describe how to verify it

frist, pull some images:

pouch pull library/busybox
pouch pull library/hello-world

then, just remove them:

pouch rmi library/busybox library/hello-world

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Apr 23, 2018
@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Pouch, @xiechengsheng
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/alibaba/pouch/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

@allencloud
Copy link
Collaborator

allencloud commented Apr 24, 2018

This is on image side, could you help to review this? @fuweid

And could you squash all three commits into a single one commit and sign off your single commit? @xiechengsheng

@HusterWan
Copy link
Contributor

@xiechengsheng Please execute command blow to fix conflict, thanks a lot

git fetch upstream
git rebase upstream/master 

@xiechengsheng
Copy link
Contributor Author

@fuweid Could your review this pr? Thanks a lot 😊

@fuweid
Copy link
Contributor

fuweid commented Apr 25, 2018

Hi @xiechengsheng

sorry for the late reply.

For the pull API, the tag information can be ignored because the latest is default value. But for the inspect/rmi related commands, the name can be one of the following kinds:

  • name without tag
  • name with tag
  • name with digest
  • name with tag and digest
  • short ID
  • image ID

Based on the user cases, I don't think we should separate tag from image name, because we cannot just add the latest at the end if we missing. It is a little different from pull API, right? How do you think?

Please let me know if you have any ideas about this part. Thank you!

Sorry for the late reply again.

@xiechengsheng
Copy link
Contributor Author

xiechengsheng commented Apr 26, 2018

@fuweid Thanks for your apply~

  1. emmmm, we could do this just because docker rmi command supports adding default latest tag in image name to remove images...
  2. And there still seems to be some bugs in this pr, I will try to fix it ASAP.
  3. BTW, does pouch support removing images via name with digest right now? I kown docker rmi supports removing images with name and digest by using docker rmi repository@digest, but there seems to be no repository concept in pouch images.
# pouch images --digest
IMAGE ID       IMAGE NAME                                           DIGEST                                                                    SIZE
e38bc07ac18e   registry.hub.docker.com/library/hello-world:latest   sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77   5.95 KB

# pouch rmi registry.hub.docker.com/library@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77
Error: failed to remove image: {"message":"image digest: registry.hub.docker.com/library@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77: not found"}

# pouch rmi registry.hub.docker.com/library/hello-world@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77
Error: failed to remove image: {"message":"failed to remove image: image \"registry.hub.docker.com/library/hello-world@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77\": not found"}

# pouch rmi registry.hub.docker.com/library/hello-world:latest@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77
Error: failed to remove image: {"message":"image digest: registry.hub.docker.com/library/hello-world:latest@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77: not found"}

Looking forward to your apply~

@pouchrobot
Copy link
Collaborator

@xiechengsheng Thanks for your contribution. 🍻
Please sign off in each of your commits.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1191      +/-   ##
==========================================
- Coverage   15.68%   15.68%   -0.01%     
==========================================
  Files         171      171              
  Lines       10408    10411       +3     
==========================================
  Hits         1633     1633              
- Misses       8655     8658       +3     
  Partials      120      120
Impacted Files Coverage Δ
daemon/mgr/image.go 31.95% <0%> (-0.51%) ⬇️

@fuweid
Copy link
Contributor

fuweid commented Apr 26, 2018

Hi @xiechengsheng

Thanks for your effort!

The repo name can be composed of name, tag and digest. We have plan to support digest part and tag function. For now, LGTM.

But, could you mind to update the PR title and commit message? thanks

@xiechengsheng xiechengsheng changed the title fix some bugs in docs and codes bugfix: add default tag ":latest" when using pouch rmi to remove untagged container images. Apr 26, 2018
@HusterWan
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 26, 2018
@HusterWan
Copy link
Contributor

HusterWan commented Apr 26, 2018

But, could you mind to update the PR title and commit message? thanks

@xiechengsheng Please squash your PR, and rewrite your commit information according to https://github.com/alibaba/pouch/blob/master/CONTRIBUTING.md#commit-message

Then i will merge this PR, thanks a lot again for your work

@fuweid
Copy link
Contributor

fuweid commented Apr 26, 2018

LGTM. Thanks @xiechengsheng

@fuweid fuweid merged commit dce981e into AliyunContainerService:master Apr 26, 2018
@fuweid
Copy link
Contributor

fuweid commented Apr 26, 2018

@xiechengsheng , welcome to contribute pouch. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] can't rm image without tag
6 participants