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.

Sign up for GitHub

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

Jump to bottom

handle empty RepoTags for ListImage #156

Merged
merged 1 commit into from
Oct 19, 2017
Merged

handle empty RepoTags for ListImage #156

merged 1 commit into from
Oct 19, 2017

Conversation

yanxuean
Copy link
Contributor

fix #155
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/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2017
@yanxuean yanxuean force-pushed the imagelist branch 2 times, most recently from cfb3109 to 8e87a59 Compare October 13, 2017 04:55
Random-Liu
Random-Liu reviewed Oct 13, 2017
View reviewed changes
cmd/crictl/image.go
@@ -125,11 +125,7 @@ var listImageCommand = cli.Command{
printHeader = false
fmt.Fprintln(w, "IMAGE\tTAG\tIMAGE ID\tSIZE")
}
repoTags := "<none>"
if image.RepoTags != nil {
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 change this to if len(image.RepoTags) > 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only this is insufficient. repoTagsPair[1] will still cause panic when it is "" or "tag1"

cmd/crictl/image.go Outdated
@@ -270,6 +266,21 @@ func getAuth(creds string) (*pb.AuthConfig, error) {
}, nil
}

// Support these cases as below:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to assume that this should always be image:tag or image:digest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if the repotag is not image:tag, we could log an error. Ideally repo tag should always be image:tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the cri-containerd or docker-shim should return "image:tag"?
In any case crictl should not panic. Crictl need to protect self.

Copy link
Member

Choose a reason for hiding this comment

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

@yanxuean In which case repoTags will be :tag2 or tag1:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know :)
I only protect self.

@yanxuean
Copy link
Contributor Author

/retest

@yanxuean
Copy link
Contributor Author

yanxuean commented Oct 13, 2017 •
edited
Loading

Who can tell me how trigger a new test? I need to do it when the reason of fail checking is not belong to me.

Random-Liu
Random-Liu reviewed Oct 13, 2017
View reviewed changes
Copy link
Contributor

@Random-Liu Random-Liu left a comment

Choose a reason for hiding this comment

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

The code LGTM.

The only problem is that I'm not sure whether and how we should display image without repotag. How does docker handle it today?

cmd/crictl/image.go Outdated
repoTags = image.RepoTags[0]
}
repoTagsPair := strings.Split(repoTags, ":")
repoTagsPair := parseRepoTagPair(image.RepoTags)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

The name is perfect.

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

@yanxuean
Copy link
Contributor Author

docker image for image pulled by repoDigest

root@ubuntu:/home/cloud/go/src/k8s.io/kubernetes# docker pull gcr.io/google_containers/pause@sha256:0d093c962a6c2dd8bb8727b661e2b5f13e9df884af9945b4cc7088d9350cd3ee
sha256:0d093c962a6c2dd8bb8727b661e2b5f13e9df884af9945b4cc7088d9350cd3ee: Pulling from google_containers/pause
a3ed95caeb02: Pull complete 
f11233434377: Pull complete 
Digest: sha256:0d093c962a6c2dd8bb8727b661e2b5f13e9df884af9945b4cc7088d9350cd3ee
Status: Downloaded newer image for gcr.io/google_containers/pause@sha256:0d093c962a6c2dd8bb8727b661e2b5f13e9df884af9945b4cc7088d9350cd3ee
root@ubuntu:/home/cloud/go/src/k8s.io/kubernetes# docker images
REPOSITORY                       TAG                 IMAGE ID            CREATED             SIZE
jpetazzo/nsenter                 latest              c16fe938c1a5        15 months ago       371MB
gcr.io/google_containers/pause   <none>              99e59f495ffa        17 months ago       747kB
root@ubuntu:/home/cloud/go/src/k8s.io/kubernetes#

@Random-Liu
Copy link
Contributor

For image which only has repo digest, we should:

  1. Add --digests option to show image digest.
  2. Show name with <none> tag if --digests is not specified.

I'm fine with adding --digests in another PR, but in this PR can we include the image name if a image has repo digest but not repo tag?

@feiskyer
Copy link
Member

include the image name if a image has repo digest but not repo tag

+1. @yanxuean Could you add this in this PR?

@yanxuean
Copy link
Contributor Author

I am adding it. :)

@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 17, 2017
@yanxuean
Copy link
Contributor Author

@Random-Liu @feiskyer PTAL
I don't know how to test the case for one image with multi repoTag.

@feiskyer
Copy link
Member

LGTM, thanks

image

@feiskyer feiskyer added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2017
@feiskyer
Copy link
Member

Also find a bug in dockershim, see kubernetes/kubernetes#54122

@yanxuean
Copy link
Contributor Author

@feiskyer Tks

@feiskyer feiskyer merged commit 973eaea into kubernetes-sigs:master Oct 19, 2017
@yanxuean yanxuean deleted the imagelist branch October 19, 2017 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Random-Liu Random-Liu Random-Liu left review comments

@feiskyer feiskyer feiskyer left review comments

Assignees

@feiskyer feiskyer

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

panic when list image that be pulled by digest.
4 participants
@yanxuean @Random-Liu @feiskyer @k8s-ci-robot