feat: create manifest.wall #142

letFunny · 2024-07-01T08:02:10Z

This commit adds support for the creation of the manifest.wall file. The manifest file(s) are generated at the specified paths, marked with the "generate: manifest" keyword. The files are ZSTD compressed and have the basename "manifest.wall". The format of the Chisel DB follows the RK009 spec.

Have you signed the CLA?

This commit adds support for the creation of the manifest.wall file. The manifest file(s) are generated at the specified paths, marked with the "generate: manifest" keyword. The files are ZSTD compressed and have the basename "chisel.db". The format of the Chisel DB follows the RK009 spec. Co-authored-by: Rafid Bin Mostofa <[email protected]>

rebornplusplus

Looks quite nice to me! Thank you.

internal/manifest/manifest.go

Co-authored-by: Rafid Bin Mostofa <[email protected]>

letFunny · 2024-07-02T10:54:20Z

After the meeting we agreed to split the PR into 2 that could go in in parallel and one last one. The downside is that we can no longer see the full picture at once, but we have discussed it enough that it is not needed.

Depends on #143 and #144.

This commit merges the changes of 'feat: create manifest.wall'(canonical#142) into this branch. We need the changes of this commit to include the updated testing framework and changes. Co-authored-by: Alberto Carretero <[email protected]>

niemeyer

Thanks Alberto. Final details below.

niemeyer · 2024-09-27T10:11:36Z

internal/slicer/slicer.go

+	targetDir   string
+}
+
+func generateManifests(options *generateManifestsOptions) error {


The earlier comment was about generateManifest itself, not just LocateManifestSlices. Per earlier thread, that's also why I was mentioning that the testing of what is in here can be done in the manifest, so we don't need to involve the slicer on everything.

internal/archive/archive.go

internal/manifest/report.go

niemeyer

Much better, but still one pass to go I think. Details below:

internal/manifest/report.go

internal/manifest/report_test.go

internal/manifest/manifest.go

niemeyer · 2024-09-30T09:39:42Z

internal/manifest/manifest.go

@@ -158,6 +164,65 @@ func Validate(manifest *Manifest) (err error) {
 	return nil
 }

+// LocateManifestSlices finds the paths marked with "generate:manifest" and
+// returns a map from the manifest path to all the slices that declare it.
+func LocateManifestSlices(slices []*setup.Slice, manifestFileName string) map[string][]*setup.Slice {


Let's please rename this function to manifest.FindPaths (per docs), and have it taking the slices as its single argument. The manifest filename should be in this package since the slicer has no use for it given this function is the one pointing to the targets.

Done in 087d09d.

The remaining part about the manifestFileName I wonder whether you should be able to customize it. If I want to write it to somewhere that is not manifest.wall then I should still be able to use this function. Maybe instead of taking manifestFileName we could return the directory and the caller is the one expected to concatenate the filename.

This function is pointing out what is the standard path given the following slices. We can write manifests anywhere using manifest.Write.

Do I misunderstand the issue?

No, just that I thought the helper should be more generic and that in our previous conversations we said to move the manifest filename to slicer and keep manifest generic. I will move it back here.

Done in 8445042.

I think the conversation was about moving the function itself to slicer, for reasons related to what we've been discussing in this iteration. The comment now was about the fact we chose not to move the function out of manifest, and yet the filename is still there.

internal/manifest/manifest.go

niemeyer · 2024-09-30T10:03:46Z

internal/manifest/manifest.go

+		err := dbw.Add(&Path{
+			Kind:   "path",
+			Path:   path,
+			Mode:   fmt.Sprintf("0%o", unixPerm(manifestMode)),


How do we know this mode? The answer is we don't, not here. That's a relatively small issue, but it's also a hint of a larger issue that is quite clear: there's an unfortunate cross-dependency and disconnect right now between the manifest package and the slicer package about how to write stuff to disk.

Let's please try to fix this by:

Introduce fsutil.CreateWriter, which returns (io.WriterCloser, *Entry, error). Let's please do it right, in the sense that we'll have a writerProxy, similar to the readerProxy that we have today, to fill up the hash and size as we go, as that's trivial to do. We won't depend on it, though. See below.

Add the *Entry to the actual Report as soon as we create the *Entry. At this point the entry will still be empty as far as the hash goes, and that's the way it should be as otherwise we'd need to add it at the end, which doesn't work since entries are sorted.

Remove manifestAddManifestPaths and the respective field from the options type.

With these steps, there's nothing special anymore about manifest files here: the slicer cuts it, the report holds it, the manifest writes it down, just like everything else.

One extra detail here: CreateWriter can take the same Options parameter we have in the Create function.

Done in dad0ed7. Hopefully now everything should be unified and there is nothing special about manifest files.

internal/slicer/slicer.go

niemeyer

Very nice, thank you!

Just one detail missing from the prior review.

niemeyer · 2024-09-30T14:24:20Z

internal/manifest/manifest.go

+	Selection   []*setup.Slice
+	// Map manifest path to all the slices that declare it. See
+	// manifest.FindPaths.
+	ManifestSlices map[string][]*setup.Slice


Per earlier comments, it doesn't make sense to have this here anymore. See how it's being used below.

Done in a9ee6ac, my bad for missing it earlier.

niemeyer · 2024-09-30T14:56:45Z

internal/manifest/manifest.go

@@ -270,6 +263,9 @@ func manifestAddSlices(dbw *jsonwall.DBWriter, slices []*setup.Slice) error {
 }

 func manifestAddReport(dbw *jsonwall.DBWriter, report *Report) error {
+	if report == nil {
+		return nil


Creating a manifest without a report looks like a bug. What code elsewhere is doing this and why?

niemeyer

That Write function is beautiful now. Thank you!

rebornplusplus approved these changes Jul 2, 2024

View reviewed changes

internal/manifest/manifest.go Outdated Show resolved Hide resolved

letFunny and others added 2 commits July 2, 2024 10:44

address review comments

07ca4d7

feat: parse generate property in sdf

af60a6f

Co-authored-by: Rafid Bin Mostofa <[email protected]>

letFunny added the Blocked Waiting for something external label Jul 2, 2024

letFunny added 4 commits July 2, 2024 12:07

redo manifest package to not unmarshall everything

55bd81d

feat: internal manifest package

e96e87b

fix slicer tests

26022fd

lint errors

dfba7b3

letFunny added 18 commits July 2, 2024 16:40

remove unnecessary constants

1ea56d1

do not aggregate manifest twice for tests

1f1177a

minor rename

fe51371

add generate to README

655b17a

make validate optional and use structs for prefixes

401c86e

indent manifest jsonwall in tests

8600db6

address review comments

e2c2645

change maps for structs with omitempty, jsonwall iterator

140ab91

move path specification to function

3faa139

remove redundant comment

94ff54a

conflict resolution works properly with generate and solve bugfix

131472b

make GetGeneratePath private and change name

b8f91bf

remove incomplete error messages

426ca94

performance optimization

e16eb2e

remove redundant check that could never occur

eff44b1

avoid using slices.Concat which is not in Go 1.21

01f01f2

refactor manifest iteration with generic helper

2ee0286

restrict generic with interface union type

a0200f1

rebornplusplus mentioned this pull request Jul 16, 2024

feat: support multiple archives with "priority" field letFunny/chisel#12

Merged

cjdcordeiro mentioned this pull request Jul 17, 2024

Concerns about the validity of vulnerability scan results with slices/partial packages #149

Closed

letFunny added 3 commits September 26, 2024 17:54

make LocateManifestSlices public and tested

9b03b95

lint: remove unused variable

962566a

add comment to clarify conflict resolution

aaf70b1

letFunny removed the Blocked Waiting for something external label Sep 26, 2024

letFunny requested a review from niemeyer September 26, 2024 16:07

niemeyer requested changes Sep 27, 2024

View reviewed changes

letFunny added 2 commits September 27, 2024 14:51

move info logic into helper function

189a3bf

move report and generateManifest to manifest package

7fa6ee9

letFunny commented Sep 27, 2024

View reviewed changes

internal/manifest/report.go Outdated Show resolved Hide resolved

letFunny added 3 commits September 27, 2024 17:23

lint: check error of WalkDir function

07a0549

manifest.Write interface accepts a Writer

573427c

lint: check error in tests

f9d1aaf

rebornplusplus mentioned this pull request Sep 30, 2024

include package metadata in chiseled images #148

Open

niemeyer requested changes Sep 30, 2024

View reviewed changes

letFunny added 8 commits September 30, 2024 13:32

create report entries for the manifest files to unify logic

dad0ed7

rename LocateManifestSlices to FindPaths

087d09d

manifestAddReport now takes Report

a0920b5

better root handling report

2a0371c

Hash -> SHA256

86c6e7b

move manifest generation to auxiliary function

e070332

better comment

b39cfe7

default manifest filename for FindPaths

8445042

niemeyer requested changes Sep 30, 2024

View reviewed changes

remove unused ManifestSlices

a9ee6ac

niemeyer reviewed Sep 30, 2024

View reviewed changes

report cannot be null when writing manifest

88a11fa

niemeyer approved these changes Sep 30, 2024

View reviewed changes

niemeyer merged commit 33bed22 into canonical:main Sep 30, 2024
14 checks passed

rebornplusplus mentioned this pull request Oct 3, 2024

[RFC] Record installed slices in --root and install only what's missing #10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create manifest.wall #142

feat: create manifest.wall #142

letFunny commented Jul 1, 2024 •

edited

Loading

rebornplusplus left a comment

letFunny commented Jul 2, 2024

niemeyer left a comment

niemeyer Sep 27, 2024

niemeyer left a comment

niemeyer Sep 30, 2024

letFunny Sep 30, 2024

letFunny Sep 30, 2024

niemeyer Sep 30, 2024

letFunny Sep 30, 2024

letFunny Sep 30, 2024

niemeyer Sep 30, 2024

niemeyer Sep 30, 2024

niemeyer Sep 30, 2024

letFunny Sep 30, 2024 •

edited

Loading

niemeyer left a comment

niemeyer Sep 30, 2024

letFunny Sep 30, 2024

niemeyer Sep 30, 2024

niemeyer left a comment

feat: create manifest.wall #142

feat: create manifest.wall #142

Conversation

letFunny commented Jul 1, 2024 • edited Loading

rebornplusplus left a comment

Choose a reason for hiding this comment

letFunny commented Jul 2, 2024

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letFunny Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

letFunny commented Jul 1, 2024 •

edited

Loading

letFunny Sep 30, 2024 •

edited

Loading