Conversation
Signed-off-by: Paul Mars <paul.mars@canonical.com>
|
letFunny
left a comment
There was a problem hiding this comment.
First review. I am missing a couple of files but I wanted to publish it as quickly as possible so we have more time to iterate. This is looking great Paul, many things are looking dead simple which is always an extremely good sign. I have written some comments about how to make the PR shorter and how to make it, hopefully, even simpler. Let me know what you think.
I see some of the comments are similar to past discussions we had in upils#1. We spent some effort there, so please make sure all the comments were carried over to this one.
| if !os.IsExist(err) { | ||
| return err | ||
| } | ||
| err = os.RemoveAll(dstPath) |
There was a problem hiding this comment.
[Note to reviewer]: This can silently remove a directory full of user content. This is inline with the simple strategy implemented in this PR but it should likely evolve when a more fine-grained strategy is implemented to "spare" user content.
letFunny
left a comment
There was a problem hiding this comment.
Thanks Paul, this is almost ready. I added a bunch of minor suggestions and a couple important things. Also, please reply to my suggestions from previous reviews some of which are still open and are applicable. Once we close all previous comments and the one in this review I think we are good to go.
| optsCopy := *options | ||
| installOpts := &optsCopy | ||
| installOpts.TargetDir = targetDir | ||
| if options.Manifest != nil { |
There was a problem hiding this comment.
[Note to reviewer]: Should we log a message in that case to inform the user a "recut" will happen?
Deeper, the upgrade function currently logs a message stating an upgrade will happen. I suppose only one of these messages should be logged, since they will always be either present or absent.
letFunny
left a comment
There was a problem hiding this comment.
Thank you Paul for all the back and forth and all the effort put in the PR. I added some nitpicky comments but looking at the whole PR I cannot spot anything big pending; I see some areas where I expect minor discussion, but in general it looks very good. I think it is ready for Gustavo to review, so please resolve the comments in the PR as I don't have permission to do so, and move it to the review queue.
FWIW: If using |
This commit enables Chisel upgrading an existing rootfs.
It detects the target directory contains the result of a previous execution
to then operate an upgrade of the content. This initial simple implementation
has the limitation that existing content (files, symlinks) is systematically
replaced, even if identical. As a consequences:
different and thus the new OCI layer contains duplicated files.