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.