-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add the ability for images to retain history if provided #202
Conversation
remote/save.go
Outdated
if len(cfg.History) != len(layers) { | ||
cfg.History = make([]v1.History, len(layers)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need this as we've been careful to update the history when instantiating an image and adding layers, but you never know...
463e531
to
3822f51
Compare
Signed-off-by: Natalie Arellano <narellano@vmware.com>
3f20ac4
to
97c0a45
Compare
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
97c0a45
to
275a68c
Compare
Pointers to interfaces are weird Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
This should be done except for updating the fakes. I'm going to try to pull this into the lifecycle and then I will update the fakes with whatever is needed. In the meantime this should be ready for some initial feedback. |
Signed-off-by: Natalie Arellano <narellano@vmware.com>
image.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
retImage := image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok this is maybe my own confusion(s) here. but v1.Image
is (probably) an interface, right? which means here you're doing a pointer copy, right? so below when you call mutate
on the retImage
doesn't that also change the image
param that was passed in? (that would be a reason to do a deepcopy here, imo, if you're trying not to change your input param)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing offline discussion - I think we agreed that this is weird, but it is the way to do it unless you want to use a pointer to interface, which is also weird
…er explicitly asked for history when the image was constructed Signed-off-by: Natalie Arellano <narellano@vmware.com>
d47a25c
to
e99e711
Compare
Signed-off-by: Natalie Arellano <narellano@vmware.com>
…quested Signed-off-by: Natalie Arellano <narellano@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more work than I originally expected. Great stuff!
additions := make([]mutate.Addendum, 0) | ||
for _, layer := range layers { | ||
if len(history) != len(layers) { | ||
history = make([]v1.History, len(layers)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this lead to an off by N type of association if we get here with partial history? Not super worried about that but curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be okay... we "normalize" the history (remove empty layers, check that the lengths are equal) at every stage - on initialization, when adding a layer, on save.
We should fetch the config file AFTER we've normalized history, to avoid writing invalid history Signed-off-by: Natalie Arellano <narellano@vmware.com>
This is a first step toward buildpacks/lifecycle#411
I think this does more or less what is expected, but I need to add tests 😈