Conversation
Signed-off-by: Paul Mars <paul.mars@canonical.com>
|
upils
changed the title
letFunny
left a comment
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
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.
| srcPath := filepath.Clean(filepath.Join(tempDir, path)) | ||
| dstPath := filepath.Clean(filepath.Join(targetDir, path)) | ||
|
|
||
| if err := upgradeParentDirs(tempDir, targetDir, path); err != nil { |
There was a problem hiding this comment.
I am missing a comment explaining why we do this instead of calling MkDirAll which looks like the obvious approach. It took a non-trivial amount of discussion among the two of us who are used to seeing this code everyday, so imagine if you know nothing about the project. Can you capture the reasoning in a comment? "We put a lot of care intro extracting parent directories with the appropriate mode when slicing packages even if they are not recorded in the manifest we will use them here to sustain the same guarantees as a normal cut" (please reword it, this is just a crude example)
internal/slicer/slicer_test.go
Outdated
| }, | ||
| noMatch: true, | ||
| }, { | ||
| summary: "Unknown schema error ignored", |
There was a problem hiding this comment.
See also my first comment in this thread, it has a different set of details which is also not obvious.
internal/slicer/slicer_test.go
Outdated
| summary string | ||
| build func() *setup.Release | ||
| setup func(c *C, targetDir string, release *setup.Release) | ||
| noMatch bool |
There was a problem hiding this comment.
Shouldn't this be the chosen path? The tests are only testing error or not, not which one is actually returned.
There was a problem hiding this comment.
The promise of SelectValidManifest is that the behavior is deterministic, and if a manifest is returned it was consistent with any other valid ones. So the actual path read is not meaningful. There is a test making sure that encountering 2 inconsistent manifests returns an error, and another one making sure that encountering 2 consistent manifests is fine.
Why knowing which one was returned be meaningful in that case?
internal/slicer/slicer_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func buildReleaseWithManifestDirs(dirs ...string) *setup.Release { |
There was a problem hiding this comment.
I like this helper, this avoids repetition in the tests. A different way to model this instead of a closure is that your test has a list of manifest dirs, []string. Then your setup is the one building the release with those directories. This makes the tests a little bit easier to read.
There was a problem hiding this comment.
Building upon this suggestion I reworked it in 6e6b09e. This helper is not really needed anymore now. Test cases are simpler. I did not change the setup functions because my intent was it be only used to populate the target dir and not prepare the release.
| 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.
2 participants
This commit enables Chisel detecting 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 content (files, symlinks) is
systematically replaced, even if identical. As a consequences:
different and thus the new OCI layer contains duplicated files.