osbuild / images

Image builder image definition library
Apache License 2.0
23 stars 52 forks source link

distro: add `OutputFilename` option to ImageOptions #1039

Open mvo5 opened 6 days ago

mvo5 commented 6 days ago

This commit allows to change the filename when creating a manifest from an image type. This is useful when doing a UI for users, e.g. this will allow us to support:

$ image-builder build rhel-9 type:qcow2 --output foo.img

(this will also be useful for bootc-image-builder).

Well, osbuild itself will still put it under a directory when doing main_cli.py:exports() but that is something orthogonal.

I couldn't find a good way to test this end-to-end though (without doing a full manifest with depsolve) because the various DiskImage(), EdgeCommitImage() functions are assigned to ImageType.method so iterating over the image types will only give me the ability to run Manifest() which creates a manifest.Manifest but from there I cannot access any pipelines without producing a full osbuild manifest. Ideas welcome, I did test it via the "image-builder-cli" branch. Maybe I'm missing something but this area of the code might need some tweaks to make it more testable.

P.S. I also explored an alternative approach that would allow to set the filename on the image type in (in https://github.com/osbuild/images/compare/main...mvo5:make-target-filename-setable?expand=1) but this one here feels slightly cleaner.

thozza commented 6 days ago

My suggestion is to instead move internal/target pkg from osbuild-composer to images and reuse what is there. Please take a look at https://github.com/osbuild/osbuild-composer/blob/d1bf0a77f046c909f6fc99783310b645c37b312f/internal/target/target.go#L23-L35

EDIT: Probably not reuse as is, but I would hate us having the same thing defined in multiple places and multiple structures. Maybe some parts can be extracted / refactored or reused. What I didn't realize at the moment of making this initial comment was that the Target structure contains status-related items, which we probably don't need in images.

mvo5 commented 6 days ago

My suggestion is to instead move internal/target pkg from osbuild-composer to images and reuse what is there. Please take a look at https://github.com/osbuild/osbuild-composer/blob/d1bf0a77f046c909f6fc99783310b645c37b312f/internal/target/target.go#L23-L35

EDIT: Probably not reuse as is, but I would hate us having the same thing defined in multiple places and multiple structures. Maybe some parts can be extracted / refactored or reused. What I didn't realize at the moment of making this initial comment was that the Target structure contains status-related items, which we probably don't need in images.

Thanks for the suggestion - maybe I'm misunderstanding and/or I need to look more at this but it seems to me that the "target.Target" is a higher level concept on a different layer that is applied after the manifest creation (c.f. https://github.com/osbuild/osbuild-composer/blob/main/internal/cloudapi/v2/server.go#L163) and is more an "after-fact" concept (i.e. do something to the image after it was created with the fixed filename). I'm looking for a way to already encode the target filename in the generated manifest.

It seems to me to me this and the target.ImageName are (mostly) orthogonal, i.e. this is strictly about the local filename that is encoded in the generated manifest. I also looked at the options in target.Target and it seems only the "ImageName" (OutputFilename in this PR) is useful, the others can be derived (like "Created") or have no meaning in the "generate-a-manifest" use-case (like "Uuid"). But maybe I'm missing something, happy to have a chat about how you envision this.

achilleas-k commented 5 days ago

I couldn't find a good way to test this end-to-end though

Something for the gitlab tests maybe?