From f829d8d623f218ac9fd08f9783bff00d949ec1b1 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 11:32:14 +0200 Subject: [PATCH 1/7] distro: drop imageType.{default{Image,Installer}Config}} This commit drops the redundant and confusing copies of `defaultImageConfig` and `defaultInstallerConfig` in the generic distro imageType. The information is available inside `ImageTypeYAML` so just take it from there. Also the imageTypes `defaultImageConfig` needs to inherit the config from the parent distro so makeing the plain values available was actually a source for errors (thanks to Thozza for spotting this). --- pkg/distro/generic/imagedefs.go | 3 --- pkg/distro/generic/imagetype.go | 20 +++++++------------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/pkg/distro/generic/imagedefs.go b/pkg/distro/generic/imagedefs.go index 3a436ed584..9820cf2466 100644 --- a/pkg/distro/generic/imagedefs.go +++ b/pkg/distro/generic/imagedefs.go @@ -3,7 +3,6 @@ package generic import ( "fmt" - "github.com/osbuild/images/internal/common" "github.com/osbuild/images/pkg/distro/defs" ) @@ -12,8 +11,6 @@ func newImageTypeFrom(d *distribution, ar *architecture, imgYAML defs.ImageTypeY ImageTypeYAML: imgYAML, isoLabel: d.getISOLabelFunc(imgYAML.ISOLabel), } - it.defaultImageConfig = common.Must(it.ImageConfig(d.Name(), ar.name)) - it.defaultInstallerConfig = common.Must(it.InstallerConfig(d.Name(), ar.name)) switch imgYAML.Image { case "disk": diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index a3b1a2ca22..07fd710e7d 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -35,10 +35,8 @@ type imageType struct { arch *architecture platform platform.Platform - // XXX: make definable via YAML - workload workload.Workload - defaultImageConfig *distro.ImageConfig - defaultInstallerConfig *distro.InstallerConfig + // TODO: workload is never set + workload workload.Workload image imageFunc isoLabel isoLabelFunc @@ -160,8 +158,8 @@ func (t *imageType) getPartitionTable(customizations *blueprint.Customizations, } func (t *imageType) getDefaultImageConfig() *distro.ImageConfig { + imageConfig := common.Must(t.ImageConfig(t.arch.distro.Name(), t.arch.name)) // ensure that image always returns non-nil default config - imageConfig := t.defaultImageConfig if imageConfig == nil { imageConfig = &distro.ImageConfig{} } @@ -173,8 +171,7 @@ func (t *imageType) getDefaultInstallerConfig() (*distro.InstallerConfig, error) if !t.ImageTypeYAML.BootISO { return nil, fmt.Errorf("image type %q is not an ISO", t.Name()) } - - return t.defaultInstallerConfig, nil + return t.InstallerConfig(t.arch.distro.Name(), t.arch.name) } func (t *imageType) PartitionType() disk.PartitionTableType { @@ -259,13 +256,10 @@ func (t *imageType) Manifest(bp *blueprint.Blueprint, } if experimentalflags.Bool("no-fstab") { - if t.defaultImageConfig == nil { - t.defaultImageConfig = &distro.ImageConfig{ - MountUnits: common.ToPtr(true), - } - } else { - t.defaultImageConfig.MountUnits = common.ToPtr(true) + if t.ImageConfigYAML.ImageConfig != nil { + t.ImageConfigYAML.ImageConfig = &distro.ImageConfig{} } + t.ImageConfigYAML.ImageConfig.MountUnits = common.ToPtr(true) } containerSources := make([]container.SourceSpec, len(bp.Containers)) From 1f84765ac8bec1df06122783378861cd2eecf564 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 11:54:04 +0200 Subject: [PATCH 2/7] distro: drop `workload` from imageType We no longer support image types that set a workload directly, see https://github.com/osbuild/images/pull/1660 and also there is no concept (intentionally) for a workload to be YAML. With this we can drop `workload` from the generic distro `imageType` as this is always unset and we always create a `workload.Custom{}` based on the blueprint config. As a further step we might consider removing the `workload` package entirely and just replace it with `ImageConfig` (this ties into the idea of Achilleas to unify most configs into inheritable ImageConfigs). But that is for later :) --- pkg/distro/generic/imagetype.go | 48 +++++++++++++-------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index 07fd710e7d..1fb390621e 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -35,9 +35,6 @@ type imageType struct { arch *architecture platform platform.Platform - // TODO: workload is never set - workload workload.Workload - image imageFunc isoLabel isoLabelFunc } @@ -228,31 +225,24 @@ func (t *imageType) Manifest(bp *blueprint.Blueprint, } } - w := t.workload - if w == nil { - // XXX: this needs to get duplicaed in exactly the same - // way in rhel/imagetype.go - workloadRepos := payloadRepos - customRepos, err := bp.Customizations.GetRepositories() - if err != nil { - return nil, nil, err - } - installFromRepos := blueprint.RepoCustomizationsInstallFromOnly(customRepos) - workloadRepos = append(workloadRepos, installFromRepos...) - - cw := &workload.Custom{ - BaseWorkload: workload.BaseWorkload{ - Repos: workloadRepos, - }, - Packages: bp.GetPackagesEx(false), - EnabledModules: bp.GetEnabledModules(), - } - if services := bp.Customizations.GetServices(); services != nil { - cw.Services = services.Enabled - cw.DisabledServices = services.Disabled - cw.MaskedServices = services.Masked - } - w = cw + customRepos, err := bp.Customizations.GetRepositories() + if err != nil { + return nil, nil, err + } + installFromRepos := blueprint.RepoCustomizationsInstallFromOnly(customRepos) + payloadRepos = append(payloadRepos, installFromRepos...) + + cw := &workload.Custom{ + BaseWorkload: workload.BaseWorkload{ + Repos: payloadRepos, + }, + Packages: bp.GetPackagesEx(false), + EnabledModules: bp.GetEnabledModules(), + } + if services := bp.Customizations.GetServices(); services != nil { + cw.Services = services.Enabled + cw.DisabledServices = services.Disabled + cw.MaskedServices = services.Masked } if experimentalflags.Bool("no-fstab") { @@ -277,7 +267,7 @@ func (t *imageType) Manifest(bp *blueprint.Blueprint, /* #nosec G404 */ rng := rand.New(source) - img, err := t.image(w, t, bp, options, staticPackageSets, containerSources, rng) + img, err := t.image(cw, t, bp, options, staticPackageSets, containerSources, rng) if err != nil { return nil, nil, err } From 9d7cbd0e48d1e847de65250aed69022102156b91 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 12:00:02 +0200 Subject: [PATCH 3/7] generic: make missing ISOLabel an error Tiny cleanup, instead of returning an empty isolabel string require that ISOs have a `iso_label` set. --- pkg/distro/generic/imagetype.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index 1fb390621e..93c2a99935 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -67,12 +67,11 @@ func (t *imageType) ISOLabel() (string, error) { if !t.ImageTypeYAML.BootISO { return "", fmt.Errorf("image type %q is not an ISO", t.Name()) } - - if t.isoLabel != nil { - return t.isoLabel(t), nil + if t.isoLabel == nil { + return "", fmt.Errorf("no iso label function for %q", t.Name()) } - return "", nil + return t.isoLabel(t), nil } func (t *imageType) Size(size uint64) uint64 { From 906f4b280d35a6c112f8d688c7aeb981430f30ef Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 12:12:31 +0200 Subject: [PATCH 4/7] distro: drop `ImageType.BasePartitionTable()` The `ImageType.BasePartitionTable()` was important so that the various "images.go" functions for the various go based distros could generate a partition table. However now we only have a single "generic" distro and it has access to the base partition table without the need for the interface (and there are only two callers). So drop it from the `distro.ImageType` interface. --- pkg/distro/distro.go | 4 ---- pkg/distro/generic/imagetype.go | 8 ++------ 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/pkg/distro/distro.go b/pkg/distro/distro.go index e5bedeec9c..c9669591be 100644 --- a/pkg/distro/distro.go +++ b/pkg/distro/distro.go @@ -102,10 +102,6 @@ type ImageType interface { // has no partition table. Only support for RHEL 8.5+ PartitionType() disk.PartitionTableType - // Return the base partition tabe for the given image type, will - // return `nil` if there is none - BasePartitionTable() (*disk.PartitionTable, error) - // Returns the corresponding boot mode ("legacy", "uefi", "hybrid") or "none" BootMode() platform.BootMode diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index 93c2a99935..677e4a8a1d 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -115,12 +115,8 @@ func (t *imageType) BootMode() platform.BootMode { return platform.BOOT_NONE } -func (t *imageType) BasePartitionTable() (*disk.PartitionTable, error) { - return t.ImageTypeYAML.PartitionTable(t.arch.distro.Name(), t.arch.name) -} - func (t *imageType) getPartitionTable(customizations *blueprint.Customizations, options distro.ImageOptions, rng *rand.Rand) (*disk.PartitionTable, error) { - basePartitionTable, err := t.BasePartitionTable() + basePartitionTable, err := t.ImageTypeYAML.PartitionTable(t.arch.distro.Name(), t.arch.name) if err != nil { return nil, err } @@ -171,7 +167,7 @@ func (t *imageType) getDefaultInstallerConfig() (*distro.InstallerConfig, error) } func (t *imageType) PartitionType() disk.PartitionTableType { - basePartitionTable, err := t.BasePartitionTable() + basePartitionTable, err := t.ImageTypeYAML.PartitionTable(t.arch.distro.Name(), t.arch.name) if errors.Is(err, defs.ErrNoPartitionTableForImgType) { return disk.PT_NONE } From 5c76aa1560e227247eb6ef004cf099cf6d095629 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 12:20:15 +0200 Subject: [PATCH 5/7] distro: move `newImageTypeFrom()` close to `imageType` This commit just moves the existing `newImageTypeFrom()` close to the `imageType` definition. The old place was a separate file called `imagedefs.go` that (kinda) made sense at some point but now the new*() should really be close to the type definition. --- pkg/distro/generic/imagedefs.go | 44 --------------------------------- pkg/distro/generic/imagetype.go | 37 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 44 deletions(-) delete mode 100644 pkg/distro/generic/imagedefs.go diff --git a/pkg/distro/generic/imagedefs.go b/pkg/distro/generic/imagedefs.go deleted file mode 100644 index 9820cf2466..0000000000 --- a/pkg/distro/generic/imagedefs.go +++ /dev/null @@ -1,44 +0,0 @@ -package generic - -import ( - "fmt" - - "github.com/osbuild/images/pkg/distro/defs" -) - -func newImageTypeFrom(d *distribution, ar *architecture, imgYAML defs.ImageTypeYAML) imageType { - it := imageType{ - ImageTypeYAML: imgYAML, - isoLabel: d.getISOLabelFunc(imgYAML.ISOLabel), - } - - switch imgYAML.Image { - case "disk": - it.image = diskImage - case "container": - it.image = containerImage - case "image_installer": - it.image = imageInstallerImage - case "live_installer": - it.image = liveInstallerImage - case "bootable_container": - it.image = bootableContainerImage - case "iot": - it.image = iotImage - case "iot_commit": - it.image = iotCommitImage - case "iot_container": - it.image = iotContainerImage - case "iot_installer": - it.image = iotInstallerImage - case "iot_simplified_installer": - it.image = iotSimplifiedInstallerImage - case "tar": - it.image = tarImage - default: - err := fmt.Errorf("unknown image func: %v for %v", imgYAML.Image, imgYAML.Name()) - panic(err) - } - - return it -} diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index 677e4a8a1d..61565b57fb 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -39,6 +39,43 @@ type imageType struct { isoLabel isoLabelFunc } +func newImageTypeFrom(d *distribution, ar *architecture, imgYAML defs.ImageTypeYAML) imageType { + it := imageType{ + ImageTypeYAML: imgYAML, + isoLabel: d.getISOLabelFunc(imgYAML.ISOLabel), + } + + switch imgYAML.Image { + case "disk": + it.image = diskImage + case "container": + it.image = containerImage + case "image_installer": + it.image = imageInstallerImage + case "live_installer": + it.image = liveInstallerImage + case "bootable_container": + it.image = bootableContainerImage + case "iot": + it.image = iotImage + case "iot_commit": + it.image = iotCommitImage + case "iot_container": + it.image = iotContainerImage + case "iot_installer": + it.image = iotInstallerImage + case "iot_simplified_installer": + it.image = iotSimplifiedInstallerImage + case "tar": + it.image = tarImage + default: + err := fmt.Errorf("unknown image func: %v for %v", imgYAML.Image, imgYAML.Name()) + panic(err) + } + + return it +} + func (t *imageType) Name() string { return t.ImageTypeYAML.Name() } From a86fbee2c5a22de2c70256431b7e2d1c9867f802 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 12:23:22 +0200 Subject: [PATCH 6/7] distro: drop distro/generic.distribution.defaultImageConfig Similar to the previous commit for imageType we drop the copy of the distro default image config from the generic `distribution` struct here. This struct is trivially available via t.arch.distro.ImageConfig so no need to duplicate it. --- pkg/distro/generic/distro.go | 10 +--------- pkg/distro/generic/imagetype.go | 2 +- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/pkg/distro/generic/distro.go b/pkg/distro/generic/distro.go index 93f4c7d100..d5cba182de 100644 --- a/pkg/distro/generic/distro.go +++ b/pkg/distro/generic/distro.go @@ -40,12 +40,6 @@ type distribution struct { defs.DistroYAML arches map[string]*architecture - // XXX: move into defs.DistroYAML? the downside of doing this - // is that we would have to duplicate the default image config - // accross the centos/alma/rhel distros.yaml, otherwise we - // just load it from the imagetypes file/dir and it is natually - // "in-sync" - defaultImageConfig *distro.ImageConfig } func (d *distribution) getISOLabelFunc(isoLabel string) isoLabelFunc { @@ -85,9 +79,7 @@ func newDistro(nameVer string) (distro.Distro, error) { rd := &distribution{ DistroYAML: *distroYAML, - - defaultImageConfig: distroYAML.ImageConfig(), - arches: make(map[string]*architecture), + arches: make(map[string]*architecture), } for _, imgTypeYAML := range distroYAML.ImageTypes() { diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index 61565b57fb..cee2ed7053 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -192,7 +192,7 @@ func (t *imageType) getDefaultImageConfig() *distro.ImageConfig { if imageConfig == nil { imageConfig = &distro.ImageConfig{} } - return imageConfig.InheritFrom(t.arch.distro.defaultImageConfig) + return imageConfig.InheritFrom(t.arch.distro.ImageConfig()) } From c69762337823a99e0c6f57a84d9ac453c5ffe515 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 12:30:49 +0200 Subject: [PATCH 7/7] distro: make `ImageConfig.InheritFrom()` robust against `nil` This commit makes `ImageConfig.InheritFrom()` work with `nil` ImageConfigs. Starting from an empty ImageConfig seems fine and this way we can simplify `imageType.getDefaultImageConfig()`. Tests are added that empty ImageConfig or empty ParentConfig will result in the expected result. --- pkg/distro/generic/imagetype.go | 4 ---- pkg/distro/image_config.go | 3 +++ pkg/distro/image_config_test.go | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/distro/generic/imagetype.go b/pkg/distro/generic/imagetype.go index cee2ed7053..efa1d52bf0 100644 --- a/pkg/distro/generic/imagetype.go +++ b/pkg/distro/generic/imagetype.go @@ -188,10 +188,6 @@ func (t *imageType) getPartitionTable(customizations *blueprint.Customizations, func (t *imageType) getDefaultImageConfig() *distro.ImageConfig { imageConfig := common.Must(t.ImageConfig(t.arch.distro.Name(), t.arch.name)) - // ensure that image always returns non-nil default config - if imageConfig == nil { - imageConfig = &distro.ImageConfig{} - } return imageConfig.InheritFrom(t.arch.distro.ImageConfig()) } diff --git a/pkg/distro/image_config.go b/pkg/distro/image_config.go index 58b1c24ee8..6a646fc30a 100644 --- a/pkg/distro/image_config.go +++ b/pkg/distro/image_config.go @@ -186,6 +186,9 @@ type DNFConfig struct { // InheritFrom inherits unset values from the provided parent configuration and // returns a new structure instance, which is a result of the inheritance. func (c *ImageConfig) InheritFrom(parentConfig *ImageConfig) *ImageConfig { + if c == nil { + c = &ImageConfig{} + } return shallowMerge(c, parentConfig) } diff --git a/pkg/distro/image_config_test.go b/pkg/distro/image_config_test.go index 432b503165..129a675ba3 100644 --- a/pkg/distro/image_config_test.go +++ b/pkg/distro/image_config_test.go @@ -158,6 +158,20 @@ func TestImageConfigInheritFrom(t *testing.T) { DisabledServices: []string{"named"}, DefaultTarget: common.ToPtr("multi-user.target"), }, + }, { + name: "inheritance with nil imageConfig", + distroConfig: &ImageConfig{ + Timezone: common.ToPtr("America/New_York"), + }, + imageConfig: nil, + expectedConfig: &ImageConfig{ + Timezone: common.ToPtr("America/New_York"), + }, + }, { + name: "inheritance with nil imageConfig and distroConfig", + distroConfig: nil, + imageConfig: nil, + expectedConfig: &ImageConfig{}, }, } for idx, tt := range tests {