distro: add OutputFilename option to ImageOptions

[mvo5]

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]

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]

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.

[mvo5]

Hm, if we do this we will have to add validation here too, something like:

        if filepath.Base(s) != s {
		return fmt.Errorf("cannot use %q: only relative filenames supported", s)
	}
	if filepath.Clean(s) != s {
		return fmt.Errorf("cannot use %q: only clean filenames supported", s)
	}

and of course error handling which will make the diff a bit more annoying.

[achilleas-k]

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

Something for the gitlab tests maybe?

[achilleas-k]

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.

Do you really think it's worth moving it all here just for the image name? The output filename should definitely end up in the target, of course, but beyond that, I don't know if it makes sense for the whole Target struct to be defined here.

I could be convinced that moving Target to images makes sense, but I wouldn't do it in the context of exposing an option to name the final exported artifact.

[thozza]

TL;DR; my biggest problem with this approach is that ImageOptions.OutputFilename and Target.ImageName can be used to achieve the same thing - produce an artifact with a given filename. One approach will be used in the image-builder-cli, and another is currently used by build-composer. While both structures will be available and used in osbuild-composer, making the code potentially confusing.

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.

While your point of view is definitely correct, I'm afraid both concepts will eventually be used to solve the same problem—generating an artifact with the user-provided name. The fact that one is a higher-level concept and the other is a lower-level concept becomes just an implementation detail hidden from the user.

On a conceptual level, I don't think ImageOptions is the correct place to encode this type of information. One of the reasons is that it would be functionally almost an identical type of information to the one encoded by Target. In addition, one can export multiple osbuild pipelines as part of a manifest build, and each of these exported pipelines may produce one or more files as an artifact. Being able to export multiple pipelines as part of a single manifest build is something we should keep as a possibility, at least internally. Target conceptually represents exporting a single osbuild pipeline (also nicely represented by OsbuildArtifact).

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.

I don't think we should modify manifests to generate files with the desired filename. If we do, we'll need to make this possible with any tool that generates the manifest (OTK or anything else). Instead, image definitions usually encode metadata about what each pipeline can produce if exported. Renaming the produced artifact afterward seems like a cleaner separation of functionality.

I admit that your proposed change is simple, clean, and an easy way to achieve the functionality that you need in the image-builder-cli. However, I'm afraid that it will add friction and complicate future and more complex use cases, such as potentially exporting multiple artifacts (multiple image files, SBOMs, etc.).

FWIW, I don't think Target is usable as it is implemented in osbuild-composer. But it is suitable as a concept and inspiration. We should probably create an extensible representation for user preferences when exporting any artifact (an image file, a manifest, or anything else). As you've mentioned, we may need something different when generating just the manifest, something different when producing an image file, and something different when importing an image to the target environment and naming it there. Having a single OutputFilename field in the ImageOptions won't solve all of these use cases either.

I may be overthinking this a bit, but I'm sure that we will find some middle ground.

[achilleas-k]

I may be overthinking this a bit, but I'm sure that we will find some middle ground.

I get where you're coming from (I think) and I don't disagree. The way I see it (and please tell me if I'm going in a completely different direction from what you were thinking), we have two major categories of options: Reusable and one-off (or build-specific) options.

Reusable options are the ones we put in blueprints. Package selection, users, kernel args, enabling services, etc, are valid for all sorts of image types and we generally consider that anything that goes in a blueprint can be reused for multiple builds across time, image types, distributions, even architectures.

One-off options are things that are specific to a single build. The target image type, upload target, ostree commit source, ostree ref. Things like these originally came from the composer-cli command line arguments and later as extra properties in the build request in the API.

This distinction was never really formalised (I don't think it was anyway) and we generally had an understanding that some things don't belong in a blueprint, so we'd make it a build option, or target option, or something similar. Maybe we should think about this distinction in these (or similar) terms and reflect it in the code as well. Also, perhaps we need a third category here: Options that have no effect on manifest generation (like the upload target), which would be options that are never passed to Manifest(), but still have some effect on the output after the build.

I don't think we should modify manifests to generate files with the desired filename. If we do, we'll need to make this possible with any tool that generates the manifest (OTK or anything else). Instead, image definitions usually encode metadata about what each pipeline can produce if exported. Renaming the produced artifact afterward seems like a cleaner separation of functionality.

I don't fully agree with this, though I see your point. I don't think it's a bad idea to modify a manifest to output a desired filename. That said, allow me to argue with myself about the two possible ways of thinking about this:

1. The filename generated by a pipeline (especially the exported pipeline) is useful for a tool to know so that it can manage the output once it's done. Since it's useful to know it, it might also be useful to be able to define it, making it simpler to manage. Instead of reading some metadata from the build or the manifest generator, it tells the manifest generator what it wants, the same way it tells it where the output directory should be. This is what this PR is doing.
2. The filename generated by a pipeline is an internal implementation detail. It's parameterised in the code, but it only matters that it's consistent between pipelines and it doesn't matter what the specific filename is and should never be seen by a user of a tool built around a manifest generator for osbuild. Manifest generators know the filename created by each pipeline because they set it themselves as part of the pipeline creation (when it's a single file and not a tree) and so they can expose an option to set a filename, but what they actually do is basically just rename the file before returning it.

No. 2 is basically what osbuild-composer with composer-cli do today. composer-cli compose image --filename images/my-fancy-new-image.qcow2 <UUID> simply copies a file to a user-defined path.
Relating this back to the option categories I was describing above, this means that the output filename is part of the third category: it's specific to a single build and has no effect on the manifest itself, it only defines what the build tool does after the image is finished building.

FWIW, I don't think Target is usable as it is implemented in osbuild-composer. But it is suitable as a concept and inspiration. We should probably create an extensible representation for user preferences when exporting any artifact (an image file, a manifest, or anything else). As you've mentioned, we may need something different when generating just the manifest, something different when producing an image file, and something different when importing an image to the target environment and naming it there. Having a single OutputFilename field in the ImageOptions won't solve all of these use cases either.

I think I'm coming around to the same conclusion. I certainly agree that "Target is [not] usable as it is implemented in osbuild-composer". But we should definitely think about this more generally and design something that will be useful for all the use cases we can expect going forward.