i/statemachine: add manifest-v2 to align with livecd-rootfs format#288
Conversation
2031a11 to
b3228f7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
==========================================
- Coverage 93.49% 93.47% -0.03%
==========================================
Files 19 19
Lines 3552 3602 +50
==========================================
+ Hits 3321 3367 +46
- Misses 151 153 +2
- Partials 80 82 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
(I'll do the coverage if you do accept the modification) |
| } | ||
|
|
||
| // GenerateClassicManifest generate the classic manifest file for the given rootfs | ||
| func GenerateClassicManifest(rootfs string, outputPath string, debug bool) error { |
There was a problem hiding this comment.
does this need to be exported?
There was a problem hiding this comment.
Yes, it is used in classic_state.go.
There was a problem hiding this comment.
that's in the same package though? my Go is a bit rusty but I'm not that out of it yet!
There was a problem hiding this comment.
My Go is a bit too new, so I did not know it works like this :)
mwhudson
left a comment
There was a problem hiding this comment.
i do want you to unexport GenerateClassicManifest please
| } | ||
|
|
||
| // GenerateClassicManifest generate the classic manifest file for the given rootfs | ||
| func GenerateClassicManifest(rootfs string, outputPath string, debug bool) error { |
There was a problem hiding this comment.
that's in the same package though? my Go is a bit rusty but I'm not that out of it yet!
b3228f7 to
7d550e6
Compare
upils
left a comment
There was a problem hiding this comment.
The manifest currently produced is very likely consumed by some tools by some people. So this change would be a breaking change due to the 2 changes you mention in the description. I would like to avoid that if possible.
I am more inclined to post-process the output than change it. We have unfortunately no way to express the format version so this will be a surprising change for users.
I assume the space->tab issue could be solved with a call to sed. Is that so or is that more complex?
Also, in the README, it is clearly stated that the manifest lists packages, the current proposition would change this definition.
I agree the snaps list is currently missing and that would be nice to extract it. What do you think of generating another separate "snaps.manifest" file, listing snaps (and maybe using the tab separator?)? A bit a thinking is needed to define how that would be expressed in the image definition file but we can discuss it.
With that, the post-process would also be trivial before calling sbom-generate.
It is what I'm afraid of.
I guess the tab/space issue could be also solved on sbom side (i.e. accepting both). But we do need the snap list.
Could we add another artifact entry and generates that manifest (tab + snap) if specified? Not sure about a name tho, as it is also a manifest ^^´ |
This is another possibility but as you pointed out it might be tricky to find a good name (because the concept might be hard to express, which might indicate something is fishy). Side note: this is unlikely, but if we ever add another kind of object, the "full-manifest" name might be misleading or might see its definition change. Another approach would be to add a new "manifest-v2" (name TBD here too) that would accept a list of "kinds" (
|
The fishy part is that we have different tools that output different manfiests and we're trying to align them ^^´
Yeah, the point is to have the same manifest format everywhere, so I don't think it is a good idea to give user customization. |
c726e9b to
b937ee8
Compare
|
Added manifest-v2 (we can change the name later). This way it doesn't break anything. And I don't understand what is failing from the tests, there is no |
There was a problem hiding this comment.
Pull request overview
This PR updates classic image manifest generation to optionally produce a “v2” manifest that matches the livecd-rootfs format (tab-separated package entries and inclusion of snap seed entries).
Changes:
- Add
manifest-v2as a new classic artifact and implementgenerateClassicManifestV2to append snap seed info fromseed.yaml. - Refactor classic manifest generation to use helper functions instead of inline
chroot dpkg-querycalls. - Extend tests/mocks to cover the v2 manifest output format.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/statemachine/helper.go | Adds classic manifest generator helpers, including v2 logic and snap seed parsing. |
| internal/statemachine/classic_states.go | Updates generatePackageManifest() to generate v1 and/or v2 manifests based on artifacts. |
| internal/statemachine/classic.go | Adds manifest-v2 artifact support when assembling classic artifact states. |
| internal/statemachine/classic_test.go | Converts manifest testing into table-driven cases and adds v2 expectations (including snaps). |
| internal/statemachine/state_machine_test.go | Extends the exec helper process to return tab-separated dpkg output for the v2 test case. |
| internal/imagedefinition/image_definition.go | Adds ManifestV2 to the artifact schema (manifest-v2). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Fixed Copilot reviews, I still don't understand why short tests are failing? |
upils
left a comment
There was a problem hiding this comment.
Overall it LGTM, thanks @fhost!
See my comments for some suggestions. Can you also:
- fix the linter errors
- add a changelog entry
- update the snapcraft.yaml conf
I was unable to reproduce the test failure locally. Let's see after pushing your fixes if this is still breaking.
| File string `yaml:"file"` | ||
| Name string `yaml:"name"` | ||
| Channel string `yaml:"channel"` |
There was a problem hiding this comment.
suggestion: did you consider using https://github.com/canonical/snapd/blob/master/seed/seed.go#L50 instead of defining a new type here? This might make it easier to maintain over time.
There was a problem hiding this comment.
No I did not, I'll take a look.
There was a problem hiding this comment.
So my modification to use the Seed class is working, but that break the test as just having the seed file is probably not enough now.
There was a problem hiding this comment.
I don't know how to test >.< I don't know what is required, but I guess at least the snaps in the seed file in the snaps/ directory. I'll test with snapd, but I don't think adding snapd into the repository for testing is a good idea... If the file is enough, maybe I should make an empty example snap?
There was a problem hiding this comment.
Okay, I've just mocked the Seed directly!
There was a problem hiding this comment.
I did not expect such a big change when I suggested that, I thought unmarshaling into the Seed type from the snapd lib was going to be enough. It looks fine though and I guess that was the cleaner way to do it.
There was a problem hiding this comment.
It was enough for running, not for testing ^^´
03fa94b to
7b88ce4
Compare
upils
left a comment
There was a problem hiding this comment.
LGTM. I am waiting for the base on main and the rerun of the CI to see if unit tests pass.
Make the classic image manifest follow the same format as the one from livecd-rootfs. Changes: * Use tab instead of space in the manifest * Add snaps to the manifest
Add a new artifact manifest-v2 for classic image with the same format as the one from livecd-rootfs.
Difference with the current manifest: