feat: recut a rootfs#13
Conversation
Signed-off-by: Paul Mars <paul.mars@canonical.com>
letFunny
left a comment
letFunny
left a comment
There was a problem hiding this comment.
I reviewed the changes we discussed: moving to a different file, function flow, getting the new manifest, etc. To be completely honest, it looks extremely good to me. I am sure there are minor comments, rewordings and small refactors to be done, but I think you nailed the core structure
| // Skip the entry if both previous and new one are identical, except for | ||
| // manifests. No Size/SHA256/FinalSH2A56 are recorded for manifests, so make | ||
| // sure they are NOT skipped. | ||
| if prevEntry.Mode == entry.Mode && |
There was a problem hiding this comment.
Style-wise I would create an auxiliary var called isManifest. This condition is very hard to read.
| "github.com/canonical/chisel/public/manifest" | ||
| ) | ||
|
|
||
| // merge applies the content from the workDir to the targetDir. This |
There was a problem hiding this comment.
Comments in this function are AMAZING, very good job there.
| } | ||
| } | ||
|
|
||
| // Step 4: Apply WorkDir content to TargetDir. |
There was a problem hiding this comment.
Nit: "Apply" is a little bit imprecise IMO.
| }() | ||
| } | ||
|
|
||
| err := cut(cutTargetDir, options.Selection, options.Archives) |
There was a problem hiding this comment.
If you want to make it even more clear you could try the approach where the branching decides what to call, e.g.
if options.PreviousManifest != nil {
mkdir_tmp_dir()
cut()
merge()
} else {
cut(normal_args)
}Maybe this will help address Gustavo's comments. Personally I think the difference is minor and stylistic, I am okay with both.
| // previously cut by Chisel in the targetDir. | ||
| func merge(targetDir string, workDir string, prevManifest *manifest.Manifest, release *setup.Release) error { | ||
| logf("Merging cut in %s...", targetDir) | ||
| newManifest, err := SelectValidManifest(workDir, release) |
There was a problem hiding this comment.
This is a bit sad to see in terms of re-doing work, but this looks very good in terms of more encapsulation. Great idea Paul!
2 participants
This PR enables easily reviewing changes until
feat/select-manifestis merged.