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

Removing images with multiple tags #29316

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

ronnielai
Copy link
Contributor

@ronnielai ronnielai commented Jul 20, 2016

If an image has multiple tags, we need to remove all the tags in order to make docker image removing successful.
#28491

@ronnielai ronnielai added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 20, 2016
@ronnielai ronnielai self-assigned this Jul 20, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 20, 2016
@ronnielai ronnielai force-pushed the docker-image-remove branch 2 times, most recently from 10bd67c to c44df3c Compare July 20, 2016 20:51
@ronnielai ronnielai assigned Random-Liu and unassigned ronnielai Jul 20, 2016
@ronnielai ronnielai added this to the v1.4 milestone Jul 20, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2016
@dims
Copy link
Member

dims commented Jul 21, 2016

Looks good to me 👍

@@ -45,7 +45,7 @@ import (
func verifyCalls(t *testing.T, fakeDocker *FakeDockerClient, calls []string) {
fakeDocker.Lock()
defer fakeDocker.Unlock()
verifyStringArrayEquals(t, fakeDocker.called, calls)
verifyStringArrayEquals(t, fakeDocker.getCalledNames(), calls)
Copy link
Member

Choose a reason for hiding this comment

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

Change this to use AssertCalls?

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

@Random-Liu
Copy link
Member

Random-Liu commented Jul 21, 2016

LGTM with a nit.

@ronnielai ronnielai force-pushed the docker-image-remove branch from c44df3c to e25da21 Compare July 21, 2016 21:15
@Random-Liu
Copy link
Member

LGTM. Feel free to apply the label after the test passes. :)

@k8s-bot
Copy link

k8s-bot commented Jul 21, 2016

GCE e2e build/test passed for commit e25da21.

@ronnielai ronnielai added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate labels Jul 21, 2016
@vishh
Copy link
Contributor

vishh commented Jul 21, 2016

cc @roberthbailey

@ronnielai ronnielai added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jul 21, 2016
@ronnielai
Copy link
Contributor Author

Bumped to P0 to be cherry-picked to 1.3

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 22, 2016

GCE e2e build/test passed for commit e25da21.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 480e8a3 into kubernetes:master Jul 22, 2016
@fabioy fabioy added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 22, 2016
@fabioy fabioy modified the milestones: v1.3, v1.4 Jul 22, 2016
@fabioy fabioy removed the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 22, 2016
k8s-github-robot pushed a commit that referenced this pull request Jul 22, 2016
…316-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of #29316

Cherry pick of #29316 on release-1.3.
@fabioy fabioy added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed release-note-none Denotes a PR that doesn't merit a release note. cherrypick-candidate labels Jul 22, 2016
k8s-github-robot pushed a commit that referenced this pull request Oct 12, 2016
Automatic merge from submit-queue

CRI: Image pullable support in dockershim

For #33189.

The new test `ImageID should be set to the manifest digest (from RepoDigests) when available` introduced in #33014 is failing, because:
1) `docker-pullable://` conversion is not supported in dockershim;
2) `kuberuntime` and `dockershim` is using `ListImages with image name filter` to check whether image presents. However, `ListImages` doesn't support filter with `digest`.

This PR:
1) Change `kuberuntime.IsImagePresent` to use `runtime.ImageStatus` and `dockershim.InspectImage` instead. ***Notice an API change: `ImageStatus` should return `(nil, nil)` for non-existing image.***
2) Add `docker-pullable://` support.
3) Fix `RemoveImage` in dockershim #29316.

I've tried myself, the test can pass now.

@yujuhong @feiskyer @yifan-gu 
/cc @kubernetes/sig-node
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…ick-of-#29316-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#29316

Cherry pick of kubernetes#29316 on release-1.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants