Extract Fedora package sets into a single YAML file #1205

achilleas-k · 2025-02-10T15:35:53Z

This is a simpler version of #1104 but only does the absolute minimum to pull out package sets into embeddable yaml files.

Why?

I think the conversation on #1104 stalled because we all (rightly) have too many opinions about every design decision made in that PR. I hope that with a simpler PR that only makes one or two decisions, we might manage to get an initial version through and start working with it and iterating on it.

What?

This is not the final version of what the full extraction of image definitions will look like. This PR only pulls out the package sets for Fedora into a single yaml file. The file contains the package sets for all image types, keyed by the image type name (with s/-/_/g).

Discuss

With this PR, I think there are two design decisions that we can discuss:

the way we handle conditions,
the way we handle special version names (like rawhide).

I decided to handle conditions in code without any special syntax in the yaml file, just a special condition: block with subkeys representing the three types of conditions we have now: architecture, version upper limit (version_less_than), and lower limit (version_greater_or_equal). I wanted to make something that is the smallest change from what we have now, something that maps directly from the existing code. It's probably not the best solution, but it is the smallest step towards something new and I hope we can all agree that it's a "good first step".

The PR does make some other decisions, like the use of a single file for the whole distribution and the decision to avoid any mechanism for including or merging package sets.
Splitting the package sets into multiple files goes more in the direction of the original PR, which requires making extra decisions about how one could or should split the files. The same goes for any form of inheritance or inclusion. These are things we should discuss and design eventually, but I'd like to avoid bringing them up here, so we can focus on one thing at a time.

Notes

This PR affects only one image type's package sets: the iot-bootable-container was not properly appending the arch-specific packages to its set, and that's now fixed. The extraction of package sets itself, so anything after the second commit, has no effect on any image type's package sets.

Currently, we support Fedora 40+. Remove conditions for older Fedora versions: - Delete all VersionLessThan() conditions for Fedora 40 or older. - Unconditionally add packages with a VersionGreaterThanOrEqual() condition for Fedora 40 or older.

PackageSet.Append() does not append in place. It returns a copy.

Create a yaml file under pkg/distro/packagesets/fedora/ that lists all the package sets defined in pkg/distro/fedora/package_sets.go. Package sets are flattened, so there is no merge/append logic right now. Package sets that were previously constructed by appending another set have comments referring to the original definition (e.g. qcow2 refers to cloud_base). Package set lookups will be tied to the image type name, so image types that used another type's sets and definitions are added as aliases (e.g. ami aliases qcow2). Conditions are added in a rather straightforward way. Conditional package sets are added under 'condition:' which now should support three subkeys: 1. architecture: for extending a package set for a specific architecture 2. version_less_than: for extending a package set for a specific range of versions smaller than the specified one 3. version_greater_or_equal: for extending a package set for a specific range of versions greater or equal to the specified one Version numbers should be defined as strings to RHEL which has major.minor versions. VERSION_RAWHIDE will remain as a special value defined in code for now.

mvo5

Fwiw, I think this is a nice start, its all internal only so we can tweak the yaml as needed and I think this already makes it more approachable for cases like #1202 where external contributors can just edit yaml now

achilleas-k · 2025-02-10T15:54:11Z

Missed a go.mod update for the yaml import. Fixed and squashed.

ondrejbudai · 2025-02-10T16:17:56Z

pkg/distro/packagesets/loader.go

+type packageSet struct {
+	Include   []string   `yaml:"include"`
+	Exclude   []string   `yaml:"exclude"`
+	Condition conditions `yaml:"condition,omitempty"`


Shouldn't this be a pointer? I think it's quite common to have optional properties as pointers.

supakeen · 2025-02-10T20:30:44Z

pkg/distro/fedora/version.go

@@ -6,3 +6,11 @@ const VERSION_RAWHIDE = "42"
 // Fedora version 41 and later use a plain squashfs rootfs on the iso instead of
 // compressing an ext4 filesystem.
 const VERSION_ROOTFS_SQUASHFS = "41"
+
+func VersionReplacements() map[string]string {


The only minor question I have is why this is a function instead of a var ... :)

I tend to avoid mutable package globals which could be modified at runtime and cause issues that are difficult to troubleshoot. Most times it's okay, I don't foresee anyone modifying this map accidentally or otherwise, but I did recently have an issue where a package global struct used for testing was being modified by one test, so the test run order would affect outcomes. I tend not to think about it too much now and default to making them functions. Maybe that's a bad pattern, I'm not sure yet.

supakeen

Nice and manageable, great to start with and expand later. Thanks for pushing this forward.

bcl

I really like this. I think you've boiled down the previous attempts and discussions to their essential core here.

miabbott · 2025-02-11T06:51:57Z

pkg/distro/packagesets/fedora/package_sets.yaml

+ova:
+  <<: *vmdk
+
+iot_commit: &iot_commit


Could you add a comment here about keeping this list in sync with the package list in pagure over here - https://pagure.io/fedora-iot/ostree/blob/main/f/fedora-iot-base.yaml

We have a comment over there about keeping osbuild/images up-to-date (https://pagure.io/fedora-iot/ostree/blob/main/f/fedora-iot-base.yaml#_76) but never made a similar comment in osbuild/images

I wonder how much work it would be to have an integration test that compares the two and fail if they start to diverge. But obviously followup material!

Added comment.

ondrejbudai

LGTM! :)

@thozza when you have a minute, can you take a look? I would love to know your opinion. :)

thozza

I like it.

I think that using the image type as the key in YAML will and is getting tricky for image types that install packages in multiple pipelines (e.g. some installers). We could consider eventually adding the pipeline names to the YAML pkg set, so that an image type can have pkg sets for all pipelines defined under single key (the image type name). YAML anchors make this quite nice. Anyway, something for the future... 🚀

thozza · 2025-02-11T14:06:22Z

pkg/distro/packagesets/loader.go

-	Condition conditions `yaml:"condition,omitempty"`
+	Include   []string    `yaml:"include"`
+	Exclude   []string    `yaml:"exclude"`
+	Condition *conditions `yaml:"condition,omitempty"`


Nitpick: this change is in commit 172e42b984047a8f8ec2ad40560bc7887c0e1079, but based on the commit message, it does not belong here.

uh, messed up the rebase again. I'd like to fix this before merging. I don't like a messy history.

The Load() function reads the package sets for a given distribution and selects the specific set for the image type based on the image type name. Conditions are processed if any are defined. The 'replacements' argument defines a set of string replacements, which currently only apply to version strings. This will be used to replace version names like VERSION_RAWHIDE with the value defined by the distribution.

The VersionReplacements() function returns a map of the constants defined in pkg/distro/fedora/version.go.

Add a comment as a reminder to keep the fedora iot-commit package set in sync with the official definitions on pagure.

achilleas-k · 2025-02-11T14:47:31Z

Had to dismiss the reviews because the git history was a bit messy. Fixed up now.

achilleas-k · 2025-02-11T14:50:12Z

We could consider eventually adding the pipeline names to the YAML pkg set, so that an image type can have pkg sets for all pipelines defined under single key (the image type name).

Yes, this is probably the best idea. This workaround is bad and hopefully temporary:
https://github.com/achilleas-k/images/blob/1197ca05c50ee3430fef2d6811da579f0ea05172/pkg/distro/fedora/distro.go#L94-L99

Since the packages were extracted into yaml files in osbuild#1205, the package list order has become unstable, in particular when packages are added conditionally. I'm not entirely sure where the instability comes from, but generating manifests with the mock depsolver creates lists with different order for the OS pipeline for the image types that conditionally add "dnsmasq". Sorting the package lists ensures that manifests don't change from this instability. It also makes the manifests reproducible when the package lists are reordered internally, which we would prefer to have, since the package order pre-depsolve shouldn't affect the final manifest.

Since the packages were extracted into yaml files in #1205, the package list order has become unstable, in particular when packages are added conditionally. I'm not entirely sure where the instability comes from, but generating manifests with the mock depsolver creates lists with different order for the OS pipeline for the image types that conditionally add "dnsmasq". Sorting the package lists ensures that manifests don't change from this instability. It also makes the manifests reproducible when the package lists are reordered internally, which we would prefer to have, since the package order pre-depsolve shouldn't affect the final manifest.

achilleas-k added 3 commits February 10, 2025 16:10

distro/fedora: fix append calls for bootable container package set

c6efd12

PackageSet.Append() does not append in place. It returns a copy.

achilleas-k requested a review from a team as a code owner February 10, 2025 15:35

achilleas-k requested review from mvo5, thozza and schuellerf February 10, 2025 15:35

achilleas-k changed the title ~~Yamlize/package sets~~ Extract Fedora package sets into a single YAML file Feb 10, 2025

achilleas-k requested review from ondrejbudai, supakeen and bcl February 10, 2025 15:36

mvo5 previously approved these changes Feb 10, 2025

View reviewed changes

achilleas-k dismissed mvo5’s stale review via b5ee142 February 10, 2025 15:53

achilleas-k force-pushed the yamlize/package-sets branch from d9acd87 to b5ee142 Compare February 10, 2025 15:53

ondrejbudai reviewed Feb 10, 2025

View reviewed changes

mvo5 previously approved these changes Feb 10, 2025

View reviewed changes

achilleas-k dismissed mvo5’s stale review via 84444e5 February 10, 2025 17:09

achilleas-k force-pushed the yamlize/package-sets branch from b5ee142 to 84444e5 Compare February 10, 2025 17:09

mvo5 previously approved these changes Feb 10, 2025

View reviewed changes

supakeen reviewed Feb 10, 2025

View reviewed changes

supakeen previously approved these changes Feb 10, 2025

View reviewed changes

bcl previously approved these changes Feb 11, 2025

View reviewed changes

miabbott reviewed Feb 11, 2025

View reviewed changes

ondrejbudai previously approved these changes Feb 11, 2025

View reviewed changes

supakeen mentioned this pull request Feb 11, 2025

imagefilter: adjust short output format #1206

Merged

thozza previously approved these changes Feb 11, 2025

View reviewed changes

supakeen added this pull request to the merge queue Feb 11, 2025

achilleas-k removed this pull request from the merge queue due to a manual request Feb 11, 2025

achilleas-k added 3 commits February 11, 2025 15:45

distro/fedora: function that returns a replacement map

993e9b8

The VersionReplacements() function returns a map of the constants defined in pkg/distro/fedora/version.go.

distro/fedora: replace all package sets with packagesets.Load()

a23f7d7

packagesets/fedora: comment to keep iot-commit in sync with official

1197ca0

Add a comment as a reminder to keep the fedora iot-commit package set in sync with the official definitions on pagure.

achilleas-k dismissed stale reviews from thozza, ondrejbudai, bcl, supakeen, and mvo5 via 1197ca0 February 11, 2025 14:46

achilleas-k force-pushed the yamlize/package-sets branch from 84444e5 to 1197ca0 Compare February 11, 2025 14:46

ondrejbudai approved these changes Feb 11, 2025

View reviewed changes

ondrejbudai enabled auto-merge February 11, 2025 14:53

mvo5 approved these changes Feb 11, 2025

View reviewed changes

ondrejbudai added this pull request to the merge queue Feb 11, 2025

thozza approved these changes Feb 11, 2025

View reviewed changes

Merged via the queue into osbuild:main with commit db8c46d Feb 11, 2025
18 checks passed

achilleas-k deleted the yamlize/package-sets branch February 12, 2025 11:42

ondrejbudai mentioned this pull request Feb 13, 2025

rhel10: move os package sets to yaml files #1104

Closed

achilleas-k mentioned this pull request Feb 18, 2025

Externalise image definitions #1234

Open

7 tasks

achilleas-k mentioned this pull request Feb 26, 2025

cmd/gen-manifests: sort package sets in mock depsolver #1266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Fedora package sets into a single YAML file #1205

Extract Fedora package sets into a single YAML file #1205

achilleas-k commented Feb 10, 2025

mvo5 left a comment

achilleas-k commented Feb 10, 2025

ondrejbudai Feb 10, 2025

achilleas-k Feb 10, 2025

supakeen Feb 10, 2025

achilleas-k Feb 11, 2025

supakeen left a comment

bcl left a comment

miabbott Feb 11, 2025

mvo5 Feb 11, 2025

achilleas-k Feb 11, 2025

ondrejbudai left a comment

thozza left a comment

thozza Feb 11, 2025

achilleas-k Feb 11, 2025

achilleas-k Feb 11, 2025

achilleas-k commented Feb 11, 2025

achilleas-k commented Feb 11, 2025

Extract Fedora package sets into a single YAML file #1205

Extract Fedora package sets into a single YAML file #1205

Conversation

achilleas-k commented Feb 10, 2025

Why?

What?

Discuss

Notes

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supakeen left a comment

Choose a reason for hiding this comment

bcl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejbudai left a comment

Choose a reason for hiding this comment

thozza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilleas-k commented Feb 11, 2025

achilleas-k commented Feb 11, 2025