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

Prune named but untagged images if danglingOnly=true #30330

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

mlaventure
Copy link
Contributor

@mlaventure mlaventure commented Jan 20, 2017

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--
Closes #30307

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

Maybe we can add a quick test (do we already have a pull-by-digest test somewhere?) for this?

continue
for _, ref := range refs {
if _, ok := ref.(reference.NamedTagged); ok {
repoTagsCount++
Copy link
Member

Choose a reason for hiding this comment

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

Can just be a boolean and break can't it?

// Not a dangling image
continue
}
if !danglingOnly {
Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify this a bit?

Seems like we could do something like

shouldDelete := !danglingOnly
if !shouldDelete {
   for _, ref := range refs {
       if _, ok := ref.(reference.NamedTagged); ok {
           shouldDelete = true
           break
       }
    }

    if shouldDelete {
        // delete all the things
    }
}

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure
Copy link
Contributor Author

@cpuguy83 have a look at this version, it's indeed cleaner, thanks.

continue
shouldDelete := !danglingOnly
if !shouldDelete {
hasTag := false
Copy link
Member

Choose a reason for hiding this comment

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

This is really an extra variable since you can set shouldDelete in the loop below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldDelete needs to stay false in that case. I only want to set it to true if there's no NamedTagged. If I were to set to true here it would delete all images.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, missed that subtlety.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 25, 2017

LGTM

@LK4D4 LK4D4 merged commit a76572b into moby:master Jan 25, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 25, 2017
@mlaventure mlaventure deleted the prune-named-untagged branch February 13, 2017 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants