cmd: implement manifest command #6

mvo5 · 2024-12-02T12:49:23Z

This commit implements the manifest command for image-builder.
It will generate an osbuild manifest based on the given inputs,
e.g.:

$ ./image-builder manifest --blueprint /path/to/blueprint.toml centos-9 qcow2
{"version":"2","pipelines":[{"name":"build","runner":",...

Note that there is an integration test but because of the depsolve
it will be slow. It will be skipped when doing go test -short.

Integration tests for ostree commit, this is because ostree commit integration tests will need "build" first because the way this is tested today is that first an edge-commit is created, then an edge-ami based on this commit is generated. But "build" is currently missing

Note that this has no README.md update for manifest because manifest is considered an internal command, once "build" is added (which is easy once manifest is there) "build" will be documented.

HACKING.md

internal/manifestgen/manifestgen.go

achilleas-k · 2024-12-03T17:10:56Z

cmd/image-builder/main.go

@@ -69,6 +104,17 @@ operating sytsems like centos and RHEL with easy customizations support.`,
 	listImagesCmd.Flags().String("output", "", "Output in a specific format (text, json)")
 	rootCmd.AddCommand(listImagesCmd)

+	manifestCmd := &cobra.Command{
+		Use:          "manifest <distro> <image-type> [<arch>]",


Would it be nicer if the architecture was a flag option (--arch)?
That way we could make blueprint an optional positional argument, which I think would be a more common usage. Switching the arch would probably be less common.

Thanks, that is an interesting idea. The reason for current layout is that it's designed to be copy/paste friendly from list-iamges. I.e.

$ image-builder list-images ... rhel-10.0 type:oci arch:x86_64 $ image-builder manifest rhel-10.0 type:oci arch:x86_64 ...

so we could tweak the list output to

$ image-builder list-images ... rhel-10.0 type:vhd --arch x86_64 rhel-10.0 type:vmdk --arch x86_64 rhel-10.0 type:oci --arch x86_64 $ image-builder manifest rhel-10.0 type:oci --arch x86_64 ...

(which looks a bit uneven but not too bad and for scripts we provide --json output).

Or we could drop this property (copy/pastable). Or we make the third argument fuzzy, i.e. if it starts with "arch:" we consider it an architecture and otherwise its a blueprint. (or something else I forgot?)

WDYT?

so we could tweak the list output to

$ image-builder list-images ... rhel-10.0 type:vhd --arch x86_64 rhel-10.0 type:vmdk --arch x86_64 rhel-10.0 type:oci --arch x86_64 $ image-builder manifest rhel-10.0 type:oci --arch x86_64 ...

Eeeeeeeh... no. That's too weird.

Or we could drop this property (copy/pastable).

It's a nice property but yes I don't think it's important enough to make the output of another command so unusual.

Or we make the third argument fuzzy, i.e. if it starts with "arch:" we consider it an architecture and otherwise its a blueprint.

Not a bad idea.

(or something else I forgot?)

Perhaps something more uniform?

$ image-builder list-images ... distro:rhel-10.0 type:vhd arch:x86_64 distro:rhel-10.0 type:vmdk arch:x86_64 distro:rhel-10.0 type:oci arch:x86_64 $ image-builder manifest distro:rhel-10.0 type:oci arch:x86_64

While that makes typing out a manifest or build command by hand a tiny bit more tedious, it also makes it nice that the args aren't positional anymore and opens up the way to make distro optional too.

So you could do:

$ image-builder manifest type:qcow2

which would imply distro:$hostdistro,arch:$hostarch, and blueprint:/dev/null (empty).

While that makes typing out a manifest or build command by hand a tiny bit more tedious, it also makes it nice that the args aren't positional anymore and opens up the way to make distro optional too.

One issue with this is that we're inventing a new convention for non-positional arguments with option:value instead of the standard --option=value ...

[..]

Or we could drop this property (copy/pastable).

It's a nice property but yes I don't think it's important enough to make the output of another command so unusual.
[..]

Let's do that then, it's the most simple and straightforward change. I am a little bit sad about it because now:

$ image-builder manifest distro:rhel-10.0 type:oci arch:x86_64 error: cannot find blueprint "arch:x86_64" $ image-builder build distro:rhel-10.0 type:oci arch:x86_64 error: cannot find blueprint "arch:x86_64"

We could change the default for image-builder-cli list-image to only show same arch images by default and hint at something like "--all" to see all, then we would have preserved the copy/paste property again (at least by default) and it's nicer for the "build" case. Open for ideas of course but that seems a reasonable path forward(?).

I pushed this tweak now in a "fixup!" commit for easier review and squash them all at the end:

$ image-builder manifest centos-9 qcow2 ... $ image-builder manifest centos-9 qcow2 ./path/to/blueprint ... image-builder manifest --arch s390x centos-9 qcow2 /path/to/blueprint ...

Would it be nicer if the architecture was a flag option (--arch)? That way we could make blueprint an optional positional argument, which I think would be a more common usage. Switching the arch would probably be less common.

I'd agree that this would be the more common usage but for the build command, not the manifest. My reasoning is that we will most probably end up generating manifests on a host that may not have the desired architecture. For "internal" usage, I'd be fine with BP being the an option instead.

There's no perfect solution here, as I can make an argument for any combination of BP and arch (not) being a positional argument.

Fwiw, I have no strong opinion, I like the copy/paste property of the original design and "bib" also rquires an argument (currently) to pass a blueprint. Going back to the previous way is as easy as removing the last commit :) But I'm more interested in a way forward right now than which way it is :) (I would love to get this in before EOY)

This had some more (offline) discussion with Ondrej and Achilleas and the latest agreement is:

This commit tweaks the argument handling based on the suggestion by Achilleas and Ondrej (thanks!). Now it takes $ image-builder manifest qcow2 ./path/to/blueprint ... $ image-builder manifest --arch s390x --distro centos-9 qcow2 ... If no arch is specified the hostname arch is used. If no distro is specified in either the blueprint or the commandline it is auto-detected based on the host. Note that if the distro from the comandline and the blueprint diverge an error is raised. We can relax this rule and just add precedence, e.g. commandline always overrides the blueprint but ideally we would have a clear use-case here so we start conservative and can always relax this rule later (the inverse is much harder). This means it is no longer copy/paste friendly from `list-images` by default but instead we can provide a new `list-images --output=shell` option that outputs in exactly the format that `image-builder [manifest|build]` need.

cmd/image-builder/main.go

thozza

Nice work. I've added a few comments.

thozza · 2024-12-12T10:21:32Z

HACKING.md

+The go source code of image-builder-cli is under
+`./cmd/image-builder`. It uses the
+[images](https://github.com/osbuild/images) library internally to
+generate the imagess. Unit tests (and integration tests where it makes


typo: imagess

thozza · 2024-12-12T10:23:23Z

internal/manifesttest/manifesttest.go

+// PipelineNamesFrom will return all pipeline names from an osbuild
+// json manifest. It will error on missing pipelines.
+//
+// XXX: move to images:pkg/manifesttest? or something


I added this as a TODO

thozza · 2024-12-12T10:29:18Z

internal/manifestgen/manifestgen.go

+		depsolvedSets[name] = res.Packages
+		repoSets[name] = res.Repos


It may be good to note here that the code discards the SBOM part of res.

internal/manifestgen/manifestgen.go

thozza · 2024-12-12T10:46:07Z

cmd/image-builder/main.go

@@ -69,6 +104,17 @@ operating sytsems like centos and RHEL with easy customizations support.`,
 	listImagesCmd.Flags().String("output", "", "Output in a specific format (text, json)")
 	rootCmd.AddCommand(listImagesCmd)

+	manifestCmd := &cobra.Command{
+		Use:          "manifest <distro> <image-type> [<arch>]",


Would it be nicer if the architecture was a flag option (--arch)? That way we could make blueprint an optional positional argument, which I think would be a more common usage. Switching the arch would probably be less common.

I'd agree that this would be the more common usage but for the build command, not the manifest. My reasoning is that we will most probably end up generating manifests on a host that may not have the desired architecture. For "internal" usage, I'd be fine with BP being the an option instead.

There's no perfect solution here, as I can make an argument for any combination of BP and arch (not) being a positional argument.

internal/blueprintload/blueprintload.go

ondrejbudai

Looks great. I would like to have an answer for the seeding, other comments are nitpicks. :)

ondrejbudai · 2024-12-12T19:32:25Z

cmd/image-builder/filters.go

+	if err != nil {
+		return nil, err
+	}
+	// strip prefixes to make ib copy/paste friendly when pastring output


pastring -> pasting

ondrejbudai · 2024-12-12T19:58:35Z

internal/manifestgen/manifestgen.go

+	for name, pkgSet := range packageSets {
+		res, err := solver.Depsolve(pkgSet, sbom.StandardTypeNone)
+		if err != nil {
+			return nil, nil, err


Not sure how much this is worth since we really need a shared helper, but still: I think this error should be wrapper with name.

Good point, I wrapped all errors from the default*Resolver now

ondrejbudai · 2024-12-12T20:00:43Z

internal/manifestgen/manifestgen.go

+	if err != nil {
+		return err
+	}
+	preManifest, warnings, err := imgType.Manifest(bp, *imgOpts, repos, 0)


What about the seed here? I think the we should make it random by default. A fixed seed should be opt-in.

Thanks, (very) nice catch and urg for missing this.

Thinking a bit about it I wonder^Wthink we should look into images and tweak the API here, it seems in almost all "real" use-cases one would want a non-zero seed here so maybe this could become either a pointer (and on nil we just pick a random seed) or keep as is but assign a random value if we get "0" in images. Obviously followup material and I fixed this instance of the issue here but thinking about the future/a followup, wdyt?

I opened osbuild/images#1107 as an RFC

This commit adds a helper to find exactly a single image from a given disto/type/arch description. This will be used in `manifest`, `build` and potentially more. It is meant to support copy/paste from the `image-builder list-images` output, so: "distro:centos-9" is support just like "centos-9" (same for distro/imgType/arch).

This commit adds a basic HACKING.md that explains the bare mimimum to get started, including `go test -short` to skip the expensive tests. Contains also a drive-by rename of the test dependency install for the GH action.

This commit adds a new `manifesttest` that helps sharing code when testing generated osbuild manifests.

This commit adds a new generic `manifestgen` package that can be used to generate osbuild manifests. It works on a higher level then the low-level `manifest` package from `images` and provides plugable resolvers and a streamlined API. This package is meant to be moved to the `images` library eventually.

This commit implements the `manifest` command for `image-builder`. It will generate an osbuild manifest based on the given inputs, e.g.: ``` $ ./image-builder manifest centos-9 qcow2 {"version":"2","pipelines":[{"name":"build","runner":",... ``` Note that there is an integration test but because of the depsolve it will be slow. It will be skipped when doing `go test -short`.

ondrejbudai

I like this very much. Some minor issues inline.

ondrejbudai · 2024-12-13T14:52:43Z

cmd/image-builder/main.go

 		Short:        "Build manifest for the given distro/image-type, e.g. centos-9 qcow2",
 		RunE:         cmdManifest,
 		SilenceUsage: true,
-		Args:         cobra.MinimumNArgs(2),
+		Args:         cobra.MinimumNArgs(1),


Doesn't cobra have RangeArgs(1, 2)?

internal/blueprintload/blueprintload.go

cmd/image-builder/main.go

cmd/image-builder/distro.go

This commit provides a `blueprintload` package that can be used to load blueprints from json/toml from a path. This will be used in `bootc-image-builder` and `image-builder-cli` and should eventually be merged into `images`.

This commit adds support to pass a blueprint to the manifest generation.

This commit adds an integration test for the container resolving when generating a manifest. This is sadly quite indirect right now because `manifestgen` needs some more support from `images` and the `distro_test` code there (that will hopefully come soon(ish)).

This commit tweaks the argument handling based on the suggestion by Achilleas and Ondrej (thanks!). Now it takes ```console $ image-builder manifest qcow2 ./path/to/blueprint ... $ image-builder manifest --arch s390x --distro centos-9 qcow2 ... ``` If no arch is specified the hostname arch is used. If no distro is specified in either the blueprint or the commandline it is auto-detected based on the host. Note that if the distro from the comandline and the blueprint diverge an error is raised. We can relax this rule and just add precedence, e.g. commandline always overrides the blueprint but ideally we would have a clear use-case here so we start conservative and can always relax this rule later (the inverse is much harder). This means it is no longer copy/paste friendly from `list-images` by default but instead we can provide a new `list-images --output=shell` option that outputs in exactly the format that `image-builder [manifest|build]` need.

ondrejbudai

Looks good from my PoV

mvo5 requested a review from achilleas-k December 2, 2024 12:49

mvo5 force-pushed the add-manifest-2 branch from 3dc94a3 to 75ffe06 Compare December 2, 2024 14:25

mvo5 marked this pull request as ready for review December 2, 2024 14:28

mvo5 requested a review from ondrejbudai December 2, 2024 15:37

mvo5 mentioned this pull request Dec 3, 2024

cmd: add new build command #8

Merged

achilleas-k reviewed Dec 3, 2024

View reviewed changes

mvo5 force-pushed the add-manifest-2 branch 2 times, most recently from df44603 to 82e057f Compare December 4, 2024 09:34

mvo5 requested a review from achilleas-k December 4, 2024 09:35

mvo5 force-pushed the add-manifest-2 branch 2 times, most recently from d4101ef to d46d430 Compare December 4, 2024 09:54

thozza self-requested a review December 6, 2024 11:59

mvo5 mentioned this pull request Dec 12, 2024

cmd: add new describe-image command #9

Merged

thozza reviewed Dec 12, 2024

View reviewed changes

mvo5 force-pushed the add-manifest-2 branch from d8b8d4e to 0079e3d Compare December 12, 2024 17:59

ondrejbudai reviewed Dec 12, 2024

View reviewed changes

mvo5 added 5 commits December 13, 2024 09:22

HACKING.md: add basic documentation for the development

0b9b601

This commit adds a basic HACKING.md that explains the bare mimimum to get started, including `go test -short` to skip the expensive tests. Contains also a drive-by rename of the test dependency install for the GH action.

pkg: add new manifesttest package with test helpers

d44bc16

This commit adds a new `manifesttest` that helps sharing code when testing generated osbuild manifests.

mvo5 force-pushed the add-manifest-2 branch from 0079e3d to c147fa8 Compare December 13, 2024 08:35

mvo5 mentioned this pull request Dec 13, 2024

pkg: make seed in ImageType.Manifest() random by default osbuild/images#1107

Merged

mvo5 force-pushed the add-manifest-2 branch from c147fa8 to 2d0c92c Compare December 13, 2024 12:04

mvo5 requested review from ondrejbudai and thozza December 13, 2024 14:14

ondrejbudai requested changes Dec 13, 2024

View reviewed changes

mvo5 added 3 commits December 13, 2024 16:33

internal: add new blueprintload package

3401220

This commit provides a `blueprintload` package that can be used to load blueprints from json/toml from a path. This will be used in `bootc-image-builder` and `image-builder-cli` and should eventually be merged into `images`.

cmd: add blueprint support to manifest

fb8fbe6

This commit adds support to pass a blueprint to the manifest generation.

mvo5 force-pushed the add-manifest-2 branch from 2d0c92c to 2d4b03d Compare December 13, 2024 15:36

mvo5 requested a review from ondrejbudai December 13, 2024 15:37

ondrejbudai approved these changes Dec 13, 2024

View reviewed changes

supakeen approved these changes Dec 16, 2024

View reviewed changes

supakeen added this pull request to the merge queue Dec 16, 2024

Merged via the queue into osbuild:main with commit 4b8bff8 Dec 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: implement manifest command #6

cmd: implement manifest command #6

mvo5 commented Dec 2, 2024 •

edited

Loading

achilleas-k Dec 3, 2024

mvo5 Dec 4, 2024

achilleas-k Dec 4, 2024 •

edited

Loading

achilleas-k Dec 4, 2024

mvo5 Dec 4, 2024

mvo5 Dec 4, 2024 •

edited

Loading

thozza Dec 12, 2024

mvo5 Dec 13, 2024

mvo5 Dec 13, 2024

thozza left a comment

thozza Dec 12, 2024

thozza Dec 12, 2024

mvo5 Dec 12, 2024

thozza Dec 12, 2024

thozza Dec 12, 2024

ondrejbudai left a comment

ondrejbudai Dec 12, 2024

ondrejbudai Dec 12, 2024

mvo5 Dec 13, 2024

ondrejbudai Dec 12, 2024

mvo5 Dec 13, 2024

mvo5 Dec 13, 2024

ondrejbudai left a comment

ondrejbudai Dec 13, 2024

mvo5 Dec 13, 2024

ondrejbudai left a comment

		depsolvedSets[name] = res.Packages
		repoSets[name] = res.Repos

cmd: implement manifest command #6

cmd: implement manifest command #6

Conversation

mvo5 commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilleas-k Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 Dec 4, 2024 • edited Loading

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

thozza 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

ondrejbudai 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

ondrejbudai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejbudai left a comment

Choose a reason for hiding this comment

mvo5 commented Dec 2, 2024 •

edited

Loading

achilleas-k Dec 4, 2024 •

edited

Loading

mvo5 Dec 4, 2024 •

edited

Loading