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

image/cache: fix isValidParent logic #31189

Merged
merged 1 commit into from
Feb 21, 2017
Merged

Conversation

runcom
Copy link
Member

@runcom runcom commented Feb 20, 2017

Fix #31186

Signed-off-by: Antonio Murdaca runcom@redhat.com

@runcom
Copy link
Member Author

runcom commented Feb 20, 2017

I'll add a test asap, in the middle of something else now.

@vdemeester
Copy link
Member

/cc @tonistiigi @cpuguy83

@tonistiigi
Copy link
Member

LGTM, but needs a test

@runcom
Copy link
Member Author

runcom commented Feb 21, 2017

Yes, as noted above, I'll need to add one.

@aaronlehmann
Copy link
Contributor

I think isValidParent should be renamed, since it's no longer checking for a parent/child relationship.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Member Author

runcom commented Feb 21, 2017

I think isValidParent should be renamed, since it's no longer checking for a parent/child relationship.

I'll add another commit to change that or follow up (do you have any suggestion on the new name?)
@tonistiigi added a test

@aaronlehmann
Copy link
Contributor

I'll add another commit to change that or follow up (do you have any suggestion on the new name?)

Actually, I might have misunderstood the fix.

I see that isValidParent still checks if len(parent.History) >= len(img.History) { (not >).

So it seems like it still is checking for a parent, but you fixed a problem that happens if the parent has the same number of filesystem layers. If this is the case, the name doesn't need to change.

LGTM

Copy link
Contributor

@LK4D4 LK4D4 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 LK4D4 merged commit 093867b into moby:master Feb 21, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone Feb 21, 2017
@runcom runcom deleted the fix-build-cache-from branch February 22, 2017 08:10
@vieux vieux mentioned this pull request Feb 22, 2017
@vieux vieux modified the milestones: 17.03.0, 17.04.0 Feb 22, 2017
@tonistiigi tonistiigi mentioned this pull request Mar 2, 2017
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.

7 participants