many: HMS-10300 prepare splitting of imagetypes.yaml by avitova · Pull Request #2257 · osbuild/images

avitova · 2026-03-09T15:47:25Z

This PR contains changes in the functional part of the code, there is also a draft #2266 that shows what these changes allow us. We wanted to keep changes in the .yaml separately in order to prevent merge conflicts. :)

supakeen

Thank you this is already shaping up to be quite nice. I've put some comments in the files. Let me know what you think, some of them are pretty straightforward left-overs and some of them are maybe up for discussion (e.g. the double-loop).

I'd really prefer to drop the idea of any image config key existing within a file containing image types so if we can I think we should.

I'll check out your branch when I find some time to play around with how I'd do that/figure out the approach that makes sense to me 🙂.

pkg/distro/defs/loader.go

avitova · 2026-03-12T10:35:11Z

There's workstation-live-installer, pxe-tar-xz and bootc-rpm-installer. We can put those in a separate file as well, and that should make it! I assume we cannot really deduplicate, since cross-file anchor sharing is not possible. I moved image_config to distro definitions.

supakeen

Starting to look great functionally; can you please drop the commits that actually do the YAML splitting from this PR? We'll open a new PR based on this one after you do so where we can work on the splitting part separately.

Note: this also means that your last commit that introduces the common file should drop all the changes that are being done to the YAML.

The YAML changes causes a lot of conflicts with any other PRs while the code changes do not, and it'll also put the code commits in logical ordering 🙂

supakeen · 2026-03-14T08:33:03Z

data/distrodefs/fedora/_common.yaml

@@ -0,0 +1,1243 @@
+---
+.common:


Perhaps we should just allow arbitrary top-level keys in the decoding, or introduce a second common section in the ImageTypesYAML so we can have two areas for people to define shared things. Let's start with a .global in imagetypes.yaml and rename the _common.yaml to _global.yaml.

Some things are shared 'globally' across all imagetype yaml's but some here clearly belong inside the variant YAMLs (such as the server package sets; they'd live inside server.yaml, etc).

avitova · 2026-03-16T11:37:35Z

Great, I dropped everything that seems to create conflicts.

Undrafted.

supakeen · 2026-03-16T11:40:07Z

This PR is ready for review, I'll be creating a PR based on this one to show what split imagetypes would actually look like in practice. We decided to do that separately since it will almost continuously conflict with anything.

Some questions we can ask ourselves:

Do we want to prepend distros.yaml instead of a special file? My hunch is no; it'd require allowing specifying imagetypes within distros.yaml as well and probably a bunch of special casing for the defs path?
Do we want to expand so that we prepend all files starting with _ in the defspath? Probably YAGNI for now.

achilleas-k

The PR title suggests this will in fact split the Fedora image types. I understand we want to do this separately. But it'd be nice to see if this change is functional. Maybe we don't need to split everything all at once, but one image type can be moved to a separate file to see that this actually works.

Regardless, the PR title should be updated.

As for the test failures in GitLab, it seems everything's running correctly but the execution fails during cleanup (or during cache updates)? Not our own cache though, the gitlab one. Is this related to the recent changes in the terraform executor?

supakeen · 2026-03-17T06:28:11Z

The PR title suggests this will in fact split the Fedora image types. I understand we want to do this separately. But it'd be nice to see if this change is functional. Maybe we don't need to split everything all at once, but one image type can be moved to a separate file to see that this actually works.

Regardless, the PR title should be updated.

There's #2266 that actually implements the split so you can see what it looks like; we wanted to keep it out of this PR because it constantly conflicts (as you can see; that PR conflicts right now as well and it's only half a day old :>).

As for the test failures in GitLab, it seems everything's running correctly but the execution fails during cleanup (or during cache updates)? Not our own cache though, the gitlab one. Is this related to the recent changes in the terraform executor?

I've rebased on main where I think these issues are now fixed :)

thozza

Nice work!

I had a big comment in the first commit, that the distro-level ImageConfig should be moved out from image types YAML to the distro YAML with this split, only to find out that you did it in the next commit. So KUDOS for that 👏

On a general note, I've added a few nitpicks that I'd like to see addressed or at least discussed.

Also WRT commit messages, we tend to use imperative speech in general.

thozza · 2026-03-17T12:03:22Z

pkg/distro/defs/loader.go

+	if d.DistroImageConfig != nil {
+		return d.DistroImageConfig.For(d.ID)
+	}
+	return &distro.ImageConfig{}


Nitpick: the function would previously return nil, not a pointer to an empty struct.,

oh, good catch, ty
fixed

thozza · 2026-03-17T12:46:28Z

pkg/distro/defs/loader.go

+	count := 0
+	for _, cfg := range configs {
+		count += len(cfg.ImageTypes)
+	}
+
+	if count > 0 {
+		d.imageTypes = make(map[string]ImageTypeYAML, count)


Nitpick: this is unrelated to the commit it appears in, but it also changes the behavior. Previously, the d.imageTypes would be initialized only when it was nil, now it will be overwritten by a new value regardless of its original value if count > 0.

Moreover, you could simply return if count == 0.

Lastly, I'm not convinced that counting the image types from configs in a separate loop, just to allocate the map precisely is worth it. It adds complexity for marginal benefit and Go will keep some buffer and it is quite efficient when growing the map. It is unlikely that we will have thousands of image types defined in a distribution, so not specifying the size of the map should be acceptable.

Well, this helps us not allocate a pointer to an empty map into imageTypes which is what happened with the previous version.
We can also do
d.imageTypes = make(map[string]ImageTypeYAML) as we previously did, and then if it is empty by the end of the function, we can set it back to nil - maybe even in a defer, but I don't like that either.

I think it would be a lot cleaner and avoid pre-counting if we loaded everything into a separate map and skipped assigning it in the end if it's empty (to preserve nil).

imageTypes := make(map[string]ImageTypeYAML) for _, cfg := range configs { for name, v := cfg.ImageTypes { ... load etc ... } } if len(imageTypes) > 0 { d.imageTypes = imageTypes }

We're going to run through configs and image types anyway, no need to do it twice. The total count (len(imageTypes)) is right there in the end.

achilleas-k · 2026-03-18T10:16:45Z

pkg/distro/defs/loader.go

 	imageTypes map[string]ImageTypeYAML
 	// distro wide default image config
-	imageConfig *distro.ImageConfig `yaml:"default"`
+	DistroImageConfig *distroImageConfig `yaml:"image_config,omitempty"`


Why the name change? This is a property of DistroYAML, so I think it's clear that it's an ImageConfig for the distro already.

We need to have the field public, but there is already a method called ImageCongig, so there is a collision. 🥲

I don't see any public use of the field, but maybe that's part of the follow-up?

The reason is that private fields aren't deserialized (and the rest is the name clash).

achilleas-k · 2026-03-18T10:31:41Z

pkg/distro/defs/loader.go

+	count := 0
+	for _, cfg := range configs {
+		count += len(cfg.ImageTypes)
+	}
+
+	if count > 0 {
+		d.imageTypes = make(map[string]ImageTypeYAML, count)


I think it would be a lot cleaner and avoid pre-counting if we loaded everything into a separate map and skipped assigning it in the end if it's empty (to preserve nil).

imageTypes := make(map[string]ImageTypeYAML) for _, cfg := range configs { for name, v := cfg.ImageTypes { ... load etc ... } } if len(imageTypes) > 0 { d.imageTypes = imageTypes }

We're going to run through configs and image types anyway, no need to do it twice. The total count (len(imageTypes)) is right there in the end.

avitova · 2026-03-18T12:38:57Z

Thank you everyone for your reviews. I tried to address everyone's comments, I did keep the name of the field though, I am open to renaming it back if we find a nice way to solve this. :)

Update LoadImageTypes to search for all relevant files in the folder, add image type from each of those. The function allows for each image type to appear in only one file.

Move the image config field from the image types yaml into distro definitions. Allow image types to contain only definitions of image types.

Remove anchors defined in imagetypes.yaml that were not used anywhere in the yaml.

Remove the default_kernel_options which was unused for fedora, and had also a typo.

Add a logic for prepending a file _common.yaml in front of other yaml image types. Allow splitting yaml files while keeping common anchors.

supakeen

Thank you. With all the comments addressed this is a great start to offering more options for organizing image types and our YAML in general (and moving image_config into the distro YAML is something I've wanted for a while).

I'm sure (some) things will change in the future in how all of this works but there's nothing that's actively bad in this PR 🙂.

I'll continue on #2266 so we can discuss there what the final form would/could look like (as soon as I get it non-conflicting again).

supakeen requested changes Mar 9, 2026

View reviewed changes

avitova force-pushed the split-imgtypes branch from eb5b594 to 11eaff8 Compare March 10, 2026 14:54

supakeen self-requested a review March 10, 2026 15:31

avitova force-pushed the split-imgtypes branch from 11eaff8 to f83ee8e Compare March 12, 2026 10:31

avitova force-pushed the split-imgtypes branch from f83ee8e to 29f6874 Compare March 13, 2026 18:32

avitova changed the title ~~multiple: HMS-10300 split imagetypes.yaml~~ many: HMS-10300 split imagetypes.yaml Mar 13, 2026

supakeen previously requested changes Mar 14, 2026

View reviewed changes

supakeen reviewed Mar 14, 2026

View reviewed changes

avitova force-pushed the split-imgtypes branch 2 times, most recently from a093674 to bd48457 Compare March 16, 2026 11:28

avitova marked this pull request as ready for review March 16, 2026 11:37

avitova requested a review from a team as a code owner March 16, 2026 11:37

avitova requested review from bcl, lzap and supakeen March 16, 2026 11:37

supakeen requested review from achilleas-k and thozza March 16, 2026 11:40

supakeen mentioned this pull request Mar 16, 2026

fedora: split yamls #2266

Draft

achilleas-k reviewed Mar 16, 2026

View reviewed changes

supakeen force-pushed the split-imgtypes branch from c03def3 to 1d486d6 Compare March 17, 2026 06:28

avitova changed the title ~~many: HMS-10300 split imagetypes.yaml~~ many: HMS-10300 prepare splitting of imagetypes.yaml Mar 17, 2026

thozza reviewed Mar 17, 2026

View reviewed changes

achilleas-k reviewed Mar 18, 2026

View reviewed changes

avitova force-pushed the split-imgtypes branch 2 times, most recently from f27b3f0 to e85fbe6 Compare March 18, 2026 12:37

avitova requested review from achilleas-k and thozza March 18, 2026 12:37

avitova added 5 commits March 18, 2026 13:55

pkg/distro: search for image types in all files

e1bcbc2

Update LoadImageTypes to search for all relevant files in the folder, add image type from each of those. The function allows for each image type to appear in only one file.

distrodefs: move image_config to distro YAML

d28eae2

Move the image config field from the image types yaml into distro definitions. Allow image types to contain only definitions of image types.

data/distrodefs: remove unused anchor blocks

fbe5553

Remove anchors defined in imagetypes.yaml that were not used anywhere in the yaml.

distrodefs: remove unused default_kernel_options

5b63773

Remove the default_kernel_options which was unused for fedora, and had also a typo.

distrodefs: prepend a common file to all img types

d30485e

Add a logic for prepending a file _common.yaml in front of other yaml image types. Allow splitting yaml files while keeping common anchors.

avitova force-pushed the split-imgtypes branch from e85fbe6 to d30485e Compare March 18, 2026 12:56

supakeen approved these changes Mar 18, 2026

View reviewed changes

		@@ -0,0 +1,1243 @@
		---
		.common:

Conversation

avitova commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supakeen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avitova commented Mar 12, 2026

Uh oh!

supakeen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supakeen Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

avitova commented Mar 16, 2026

Uh oh!

supakeen commented Mar 16, 2026

Uh oh!

achilleas-k left a comment

Choose a reason for hiding this comment

Uh oh!

supakeen commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thozza left a comment

Choose a reason for hiding this comment

Uh oh!

thozza Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

avitova Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thozza Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

avitova Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

achilleas-k Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achilleas-k Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

avitova Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

achilleas-k Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

supakeen Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achilleas-k Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avitova commented Mar 18, 2026

Uh oh!

supakeen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

avitova commented Mar 9, 2026 •

edited

Loading

supakeen left a comment •

edited

Loading

supakeen commented Mar 17, 2026 •

edited

Loading

avitova Mar 18, 2026 •

edited

Loading

achilleas-k Mar 18, 2026 •

edited

Loading

supakeen Mar 18, 2026 •

edited

Loading

achilleas-k Mar 18, 2026 •

edited

Loading

supakeen left a comment •

edited

Loading