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

feature: implement RemoveImage and ImageStatus methods for cri manager #496

Conversation

zeppp
Copy link
Contributor

@zeppp zeppp commented Jan 4, 2018

Signed-off-by: zeppp [email protected]

1.Describe what this PR did
implement RemoveImage and ImageStatus methods for cri manager
2.Does this pull request fix one issue?
fixes part of #420

3.Describe how you did it

4.Describe how to verify it
Use cri-tools to check.

→ # crictl images
IMAGE                       TAG                 IMAGE ID            SIZE
docker.io/library/busybox   latest              bbc3a03235220       720kB

→ # crictl rmi docker.io/library/busybox 
FATA[0000] image status request failed: rpc error: code = Unknown desc = image: docker.io/library/busybox: not found 

→ # crictl rmi docker.io/library/busybox:latest
Deleted: docker.io/library/busybox:latest

→ # crictl images
IMAGE               TAG                 IMAGE ID            SIZE

→ # crictl inspecti docker.io/library/busybox:latest
{
  "status": {
    "id": "sha256:bbc3a03235220b170ba48a157dd097dd1379299370e1ed99ce976df0355d24f0",
    "repoTags": [
      "docker.io/library/busybox:latest"
    ],
    "repoDigests": [
      "docker.io/library/busybox@sha256:bbc3a03235220b170ba48a157dd097dd1379299370e1ed99ce976df0355d24f0"
    ],
    "size": "2699",
    "username": ""
  }
}


5.Special notes for reviews

@codecov-io
Copy link

Codecov Report

Merging #496 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
- Coverage   18.48%   18.34%   -0.15%     
==========================================
  Files          34       34              
  Lines        1747     1761      +14     
==========================================
  Hits          323      323              
- Misses       1389     1403      +14     
  Partials       35       35
Impacted Files Coverage Δ
daemon/mgr/cri.go 0% <0%> (ø) ⬆️

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 fbc6bc1...3cd4b23. Read the comment docs.

@@ -290,7 +290,18 @@ func (c *CriManager) ListImages(ctx context.Context, r *runtime.ListImagesReques

// ImageStatus returns the status of the image, returns nil if the image isn't present.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the status of image mean? The existence of the image, or the ImageInfo as meta data of image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ImageStatus will return the Basic information about a container image includes "ID" and "Size" and extra information about the image if needed. Also, if the image isn't present,it will return nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 4.Describe how to verify it

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is some kind of weird to use status, Maybe info is better. Not blocking this pr.

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 4, 2018
@allencloud allencloud merged commit 555b83d into AliyunContainerService:master Jan 4, 2018
@zeppp zeppp deleted the feature-implement-cri-removeImage branch January 9, 2018 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants