From b9efe82bd76cc66d3d192df71ba565cc14ef1e44 Mon Sep 17 00:00:00 2001 From: Tomas Hozza Date: Wed, 2 Feb 2022 14:25:08 +0100 Subject: [PATCH] distro/fedora: implementation cleanups for newer releases Clean up some implementation aspects of the Fedora distro definition: - Do not have default Fedora distro version and use `fedora` as the package name in all places that use it, instead of `fedora33`. - Fix bugs when wrong (Fedora 33) values were returned by `OSTreeRef()` and `Releasever()` for newer Fedora releases. - Test Fedora 35 in package unit tests. - Add unit test for `OSTreeRef()` method. - Use architecture name constants from `distro` package, instead of string literals. Fix #1802 Signed-off-by: Tomas Hozza --- cmd/osbuild-store-dump/main.go | 10 +- internal/distro/fedora33/distro.go | 139 ++++++++++++---------- internal/distro/fedora33/distro_test.go | 92 +++++++------- internal/distroregistry/distroregistry.go | 10 +- internal/store/json_test.go | 4 +- 5 files changed, 138 insertions(+), 117 deletions(-) diff --git a/cmd/osbuild-store-dump/main.go b/cmd/osbuild-store-dump/main.go index bf23daece..f4912fb31 100644 --- a/cmd/osbuild-store-dump/main.go +++ b/cmd/osbuild-store-dump/main.go @@ -11,7 +11,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/distro" - "github.com/osbuild/osbuild-composer/internal/distro/fedora33" + fedora "github.com/osbuild/osbuild-composer/internal/distro/fedora33" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/store" "github.com/osbuild/osbuild-composer/internal/target" @@ -116,8 +116,8 @@ func main() { awsTarget.ImageName = "My Image" awsTarget.Created = time.Now() - d := fedora33.New() - a, err := d.GetArch("x86_64") + d := fedora.NewF35() + a, err := d.GetArch(distro.X86_64ArchName) if err != nil { panic(err) } @@ -129,11 +129,11 @@ func main() { if err != nil { panic(err) } - allRepos, err := rpmmd.LoadRepositories([]string{cwd}, "fedora-33") + allRepos, err := rpmmd.LoadRepositories([]string{cwd}, "fedora-35") if err != nil { panic(err) } - repos := allRepos["x86_64"] + repos := allRepos[distro.X86_64ArchName] homeDir, err := os.UserHomeDir() if err != nil { panic("os.UserHomeDir(): " + err.Error()) diff --git a/internal/distro/fedora33/distro.go b/internal/distro/fedora33/distro.go index c9eaf66e5..942b16544 100644 --- a/internal/distro/fedora33/distro.go +++ b/internal/distro/fedora33/distro.go @@ -16,27 +16,25 @@ import ( "github.com/osbuild/osbuild-composer/internal/rpmmd" ) -const defaultName = "fedora-33" -const releaseVersion = "33" -const modulePlatformID = "platform:f33" -const ostreeRef = "fedora/33/%s/iot" +const nameTmpl = "fedora-%s" +const modulePlatformIDTmpl = "platform:f%s" -const f34Name = "fedora-34" -const f34modulePlatformID = "platform:f34" -const f34ostreeRef = "fedora/34/%s/iot" +// The second format value is intentionally escaped, because the +// format string is substituted in two steps: +// 1. on distribution level when being created +// 2. on image level +const ostreeRefTmpl = "fedora/%s/%%s/iot" -const f35Name = "fedora-35" -const f35modulePlatformID = "platform:f35" -const f35ostreeRef = "fedora/35/%s/iot" - -const f36Name = "fedora-36" -const f36modulePlatformID = "platform:f36" -const f36ostreeRef = "fedora/36/%s/iot" +const f33ReleaseVersion = "33" +const f34ReleaseVersion = "34" +const f35ReleaseVersion = "35" +const f36ReleaseVersion = "36" type distribution struct { name string + releaseVersion string modulePlatformID string - ostreeRef string + ostreeRefTmpl string arches map[string]architecture buildPackages []string } @@ -177,7 +175,7 @@ func (t *imageType) MIMEType() string { func (t *imageType) OSTreeRef() string { if t.rpmOstree { - return fmt.Sprintf(ostreeRef, t.arch.name) + return fmt.Sprintf(t.Arch().Distro().OSTreeRef(), t.Arch().Name()) } return "" } @@ -278,7 +276,7 @@ func (d *distribution) Name() string { } func (d *distribution) Releasever() string { - return releaseVersion + return d.releaseVersion } func (d *distribution) ModulePlatformID() string { @@ -286,7 +284,7 @@ func (d *distribution) ModulePlatformID() string { } func (d *distribution) OSTreeRef() string { - return d.ostreeRef + return d.ostreeRefTmpl } func sources(packages []rpmmd.PackageSpec) *osbuild.Sources { @@ -642,29 +640,80 @@ func ostreeCommitAssembler(options distro.ImageOptions, arch distro.Arch) *osbui } // New creates a new distro object, defining the supported architectures and image types -func New() distro.Distro { - return newDistro(defaultName, modulePlatformID, ostreeRef) +func NewF33() distro.Distro { + return newDefaultDistro(f33ReleaseVersion) } func NewF34() distro.Distro { - return newDistro(f34Name, f34modulePlatformID, f34ostreeRef) + return newDefaultDistro(f34ReleaseVersion) } func NewF35() distro.Distro { - return newDistro(f35Name, f35modulePlatformID, f35ostreeRef) + return newDefaultDistro(f35ReleaseVersion) } func NewF36() distro.Distro { - return newDistro(f36Name, f36modulePlatformID, f36ostreeRef) + return newDefaultDistro(f36ReleaseVersion) } func NewHostDistro(name, modulePlatformID, ostreeRef string) distro.Distro { return newDistro(name, modulePlatformID, ostreeRef) } +func newDefaultDistro(releaseVersion string) distro.Distro { + return newDistro( + fmt.Sprintf(nameTmpl, releaseVersion), + fmt.Sprintf(modulePlatformIDTmpl, releaseVersion), + fmt.Sprintf(ostreeRefTmpl, releaseVersion), + ) +} + func newDistro(name, modulePlatformID, ostreeRef string) distro.Distro { const GigaByte = 1024 * 1024 * 1024 + r := distribution{ + buildPackages: []string{ + "dnf", + "dosfstools", + "e2fsprogs", + "policycoreutils", + "qemu-img", + "selinux-policy-targeted", + "systemd", + "tar", + "xz", + }, + name: name, + modulePlatformID: modulePlatformID, + ostreeRefTmpl: ostreeRef, + } + + x86_64 := architecture{ + distro: &r, + name: distro.X86_64ArchName, + bootloaderPackages: []string{ + "dracut-config-generic", + "grub2-pc", + }, + buildPackages: []string{ + "grub2-pc", + }, + legacy: "i386-pc", + } + + aarch64 := architecture{ + distro: &r, + name: distro.Aarch64ArchName, + bootloaderPackages: []string{ + "dracut-config-generic", + "efibootmgr", + "grub2-efi-aa64", + "grub2-tools", + "shim-aa64", + }, + uefi: true, + } + iotImgType := imageType{ name: "fedora-iot-commit", filename: "commit.tar", @@ -934,35 +983,7 @@ func newDistro(name, modulePlatformID, ostreeRef string) distro.Distro { }, } - r := distribution{ - buildPackages: []string{ - "dnf", - "dosfstools", - "e2fsprogs", - "policycoreutils", - "qemu-img", - "selinux-policy-targeted", - "systemd", - "tar", - "xz", - }, - name: name, - modulePlatformID: modulePlatformID, - ostreeRef: ostreeRef, - } - x8664 := architecture{ - distro: &r, - name: "x86_64", - bootloaderPackages: []string{ - "dracut-config-generic", - "grub2-pc", - }, - buildPackages: []string{ - "grub2-pc", - }, - legacy: "i386-pc", - } - x8664.setImageTypes( + x86_64.setImageTypes( iotImgType, amiImgType, qcow2ImageType, @@ -972,18 +993,6 @@ func newDistro(name, modulePlatformID, ostreeRef string) distro.Distro { ociImageType, ) - aarch64 := architecture{ - distro: &r, - name: "aarch64", - bootloaderPackages: []string{ - "dracut-config-generic", - "efibootmgr", - "grub2-efi-aa64", - "grub2-tools", - "shim-aa64", - }, - uefi: true, - } aarch64.setImageTypes( amiImgType, qcow2ImageType, @@ -991,7 +1000,7 @@ func newDistro(name, modulePlatformID, ostreeRef string) distro.Distro { ociImageType, ) - r.setArches(x8664, aarch64) + r.setArches(x86_64, aarch64) return &r } diff --git a/internal/distro/fedora33/distro_test.go b/internal/distro/fedora33/distro_test.go index e3b20f3b2..389af014d 100644 --- a/internal/distro/fedora33/distro_test.go +++ b/internal/distro/fedora33/distro_test.go @@ -8,7 +8,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/distro/distro_test_common" - "github.com/osbuild/osbuild-composer/internal/distro/fedora33" + fedora "github.com/osbuild/osbuild-composer/internal/distro/fedora33" ) func TestFilenameFromType(t *testing.T) { @@ -60,8 +60,8 @@ func TestFilenameFromType(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - dist := fedora33.New() - arch, _ := dist.GetArch("x86_64") + dist := fedora.NewF35() + arch, _ := dist.GetArch(distro.X86_64ArchName) imgType, err := arch.GetImageType(tt.args.outputFormat) if (err != nil) != tt.wantErr { t.Errorf("Arch.GetImageType() error = %v, wantErr %v", err, tt.wantErr) @@ -106,10 +106,10 @@ func TestImageType_BuildPackages(t *testing.T) { "xz", } buildPackages := map[string][]string{ - "x86_64": x8664BuildPackages, - "aarch64": aarch64BuildPackages, + distro.X86_64ArchName: x8664BuildPackages, + distro.Aarch64ArchName: aarch64BuildPackages, } - d := fedora33.New() + d := fedora.NewF35() for _, archLabel := range d.ListArches() { archStruct, err := d.GetArch(archLabel) if err != nil { @@ -137,13 +137,13 @@ func TestImageType_BuildPackages(t *testing.T) { } func TestImageType_Name(t *testing.T) { - distro := fedora33.New() + f35 := fedora.NewF35() imgMap := []struct { arch string imgNames []string }{ { - arch: "x86_64", + arch: distro.X86_64ArchName, imgNames: []string{ "ami", "qcow2", @@ -153,7 +153,7 @@ func TestImageType_Name(t *testing.T) { }, }, { - arch: "aarch64", + arch: distro.Aarch64ArchName, imgNames: []string{ "ami", "qcow2", @@ -162,7 +162,7 @@ func TestImageType_Name(t *testing.T) { }, } for _, mapping := range imgMap { - arch, err := distro.GetArch(mapping.arch) + arch, err := f35.GetArch(mapping.arch) if assert.NoError(t, err) { for _, imgName := range mapping.imgNames { imgType, err := arch.GetImageType(imgName) @@ -203,8 +203,8 @@ func TestImageType_Size(t *testing.T) { }, } - distro := fedora33.New() - arch, err := distro.GetArch("x86_64") + f35 := fedora.NewF35() + arch, err := f35.GetArch(distro.X86_64ArchName) if assert.NoError(t, err) { for _, mapping := range sizeMap { imgType, err := arch.GetImageType(mapping.name) @@ -282,8 +282,8 @@ func TestImageType_BasePackages(t *testing.T) { bootable: true, }, } - distro := fedora33.New() - arch, err := distro.GetArch("x86_64") + f35 := fedora.NewF35() + arch, err := f35.GetArch(distro.X86_64ArchName) assert.NoError(t, err) for _, pkgMap := range pkgMaps { @@ -307,7 +307,7 @@ func TestImageType_BasePackages(t *testing.T) { func TestDistro_ManifestError(t *testing.T) { // Currently, the only unsupported configuration is OSTree commit types // with Kernel boot options - f33distro := fedora33.New() + f35distro := fedora.NewF35() bp := blueprint.Blueprint{ Customizations: &blueprint.Customizations{ Kernel: &blueprint.KernelCustomization{ @@ -316,8 +316,8 @@ func TestDistro_ManifestError(t *testing.T) { }, } - for _, archName := range f33distro.ListArches() { - arch, _ := f33distro.GetArch(archName) + for _, archName := range f35distro.ListArches() { + arch, _ := f35distro.GetArch(archName) for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, err := imgType.Manifest(bp.Customizations, distro.ImageOptions{}, nil, nil, 0) @@ -330,23 +330,23 @@ func TestDistro_ManifestError(t *testing.T) { } } -func TestFedora33_ListArches(t *testing.T) { - distro := fedora33.New() - arches := distro.ListArches() - assert.Equal(t, []string{"aarch64", "x86_64"}, arches) +func TestFedora35_ListArches(t *testing.T) { + f35 := fedora.NewF35() + arches := f35.ListArches() + assert.Equal(t, []string{distro.Aarch64ArchName, distro.X86_64ArchName}, arches) } -func TestFedora33_GetArch(t *testing.T) { - distro := fedora33.New() +func TestFedora35_GetArch(t *testing.T) { + f35 := fedora.NewF35() arches := []struct { name string errorExpected bool }{ { - name: "x86_64", + name: distro.X86_64ArchName, }, { - name: "aarch64", + name: distro.Aarch64ArchName, }, { name: "foo-arch", @@ -355,7 +355,7 @@ func TestFedora33_GetArch(t *testing.T) { } for _, a := range arches { - actualArch, err := distro.GetArch(a.name) + actualArch, err := f35.GetArch(a.name) if !a.errorExpected { assert.Equal(t, a.name, actualArch.Name()) assert.NoError(t, err) @@ -366,22 +366,34 @@ func TestFedora33_GetArch(t *testing.T) { } } -func TestFedora33_Name(t *testing.T) { - distro := fedora33.New() - assert.Equal(t, "fedora-33", distro.Name()) +func TestFedora35_Name(t *testing.T) { + distro := fedora.NewF35() + assert.Equal(t, "fedora-35", distro.Name()) } -func TestFedora33_ModulePlatformID(t *testing.T) { - distro := fedora33.New() - assert.Equal(t, "platform:f33", distro.ModulePlatformID()) +func TestFedora35_ModulePlatformID(t *testing.T) { + distro := fedora.NewF35() + assert.Equal(t, "platform:f35", distro.ModulePlatformID()) } -func TestFedora33_KernelOption(t *testing.T) { - distro_test_common.TestDistro_KernelOption(t, fedora33.New()) +func TestFedora35_OSTreeRef(t *testing.T) { + f35 := fedora.NewF35() + assert.Equal(t, "fedora/35/%s/iot", f35.OSTreeRef()) + + x86_64, err := f35.GetArch(distro.X86_64ArchName) + assert.Nilf(t, err, "failed to get %q architecture of %q distribution", distro.X86_64ArchName, f35.Name()) + ostreeImgName := "fedora-iot-commit" + ostreeImg, err := x86_64.GetImageType(ostreeImgName) + assert.Nilf(t, err, "failed to get %q image type for %q architecture of %q distribution", ostreeImgName, distro.X86_64ArchName, f35.Name()) + assert.Equal(t, "fedora/35/x86_64/iot", ostreeImg.OSTreeRef()) +} + +func TestFedora35_KernelOption(t *testing.T) { + distro_test_common.TestDistro_KernelOption(t, fedora.NewF35()) } func TestDistro_CustomFileSystemManifestError(t *testing.T) { - f33distro := fedora33.New() + f35distro := fedora.NewF35() bp := blueprint.Blueprint{ Customizations: &blueprint.Customizations{ Filesystem: []blueprint.FilesystemCustomization{ @@ -392,8 +404,8 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { }, }, } - for _, archName := range f33distro.ListArches() { - arch, _ := f33distro.GetArch(archName) + for _, archName := range f35distro.ListArches() { + arch, _ := f35distro.GetArch(archName) for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, err := imgType.Manifest(bp.Customizations, distro.ImageOptions{}, nil, nil, 0) @@ -407,7 +419,7 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { } func TestDistro_TestRootMountPoint(t *testing.T) { - f33distro := fedora33.New() + f35distro := fedora.NewF35() bp := blueprint.Blueprint{ Customizations: &blueprint.Customizations{ Filesystem: []blueprint.FilesystemCustomization{ @@ -418,8 +430,8 @@ func TestDistro_TestRootMountPoint(t *testing.T) { }, }, } - for _, archName := range f33distro.ListArches() { - arch, _ := f33distro.GetArch(archName) + for _, archName := range f35distro.ListArches() { + arch, _ := f35distro.GetArch(archName) for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, err := imgType.Manifest(bp.Customizations, distro.ImageOptions{}, nil, nil, 0) diff --git a/internal/distroregistry/distroregistry.go b/internal/distroregistry/distroregistry.go index 41b3fe27e..993023f5e 100644 --- a/internal/distroregistry/distroregistry.go +++ b/internal/distroregistry/distroregistry.go @@ -6,7 +6,7 @@ import ( "strings" "github.com/osbuild/osbuild-composer/internal/distro" - "github.com/osbuild/osbuild-composer/internal/distro/fedora33" + fedora "github.com/osbuild/osbuild-composer/internal/distro/fedora33" "github.com/osbuild/osbuild-composer/internal/distro/rhel8" "github.com/osbuild/osbuild-composer/internal/distro/rhel84" "github.com/osbuild/osbuild-composer/internal/distro/rhel85" @@ -18,10 +18,10 @@ import ( // When adding support for a new distribution, add it here. // Note that this is a constant, do not write to this array. var supportedDistros = []supportedDistro{ - {fedora33.New, fedora33.NewHostDistro}, - {fedora33.NewF34, fedora33.NewHostDistro}, - {fedora33.NewF35, fedora33.NewHostDistro}, - {fedora33.NewF36, fedora33.NewHostDistro}, + {fedora.NewF33, fedora.NewHostDistro}, + {fedora.NewF34, fedora.NewHostDistro}, + {fedora.NewF35, fedora.NewHostDistro}, + {fedora.NewF36, fedora.NewHostDistro}, {rhel8.New, rhel8.NewHostDistro}, {rhel84.New, rhel84.NewHostDistro}, {rhel85.New, rhel85.NewHostDistro}, diff --git a/internal/store/json_test.go b/internal/store/json_test.go index be6fe24f2..9f2c0c308 100644 --- a/internal/store/json_test.go +++ b/internal/store/json_test.go @@ -13,7 +13,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/common" "github.com/osbuild/osbuild-composer/internal/distro" - "github.com/osbuild/osbuild-composer/internal/distro/fedora33" + fedora "github.com/osbuild/osbuild-composer/internal/distro/fedora33" "github.com/osbuild/osbuild-composer/internal/distro/test_distro" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/target" @@ -303,7 +303,7 @@ func Test_upgrade(t *testing.T) { assert.NoErrorf(err, "Could not read test-store '%s': %v", fileName, err) err = json.Unmarshal([]byte(file), &storeStruct) assert.NoErrorf(err, "Could not parse test-store '%s': %v", fileName, err) - arch, err := fedora33.New().GetArch("x86_64") + arch, err := fedora.NewF35().GetArch(distro.X86_64ArchName) assert.NoError(err) store := newStoreFromV0(storeStruct, arch, nil) assert.Equal(1, len(store.blueprints))