many: drop the workload.Workload type entirely#1770
Conversation
supakeen
left a comment
There was a problem hiding this comment.
Sure, though I feel this could have dropped the idiom entirely and just apply them to img.OSCustomizations directly. Let's do that as a follow up since you already mentioned it.
Workloads are no longer used in the images library and there
is no appetite to use them. So this commit removes the last
use of them: the workload.Custom{} that was used to pass
image type configuration to the images functions.
The workload.Custom is replaces with the regular
manifest.OSCustomizations that already contains (almost)
anything that is needed. So instead of having two concepts
to configure the image we consolidate it to one.
The only thing that had to be added to the OSCustomizations
is the `Modules`.
Now that the workload concept is removed and everything the initial image type configuration is done via OSCustomization we need to rename "workload" as it will be confusing without the historic context (and arguably it was always a bit confusing). The new name needs to capture that the OSCustomizations applied here come from the initial imagetype. I picked `imgTypeCustomizations` as it seems to capture best what this is doing. But its long and I'm open for more ideas, I initially used `seed` as its short but its probably too abstract and it captures the concept poorly. Its also unclear how long-lived this name will be, ideally it all will be merged into a single OSCustomizations variable inside the various images but that needs to happen in followups or the churn is too big.
624b293 to
083859d
Compare
I agree, I would like to merge it into a single thing. One reason why its not currently is https://github.com/osbuild/images/blob/main/pkg/manifest/os.go#L338 - the workload packages are resolved in their own transaction so we need something new in OSCustomizations (ImageTypePackages?). I will look into it. |
These are now effectively the "custom packages" requested by the user in the Blueprint. These are not related to the Image Type. Something along the lines of "custom packages", "additional packages", "user packages", or "blueprint packages" would, in my opinion, be more descriptive of what the package set represents. |
thozza
left a comment
There was a problem hiding this comment.
Thanks for doing this first step.
All in all, I think that this is a reasonable first step. However, there are some parts that are IMO not accurate (mainly comments or variable names). But more importantly, I think that we should get rid of imgTypeCustomizations completely, since it is duplicating existing functionality. In my opinion, the OSCustomizations struct should contain the "final" image type customizations after processing user-provided input (mainly extra repos and the Blueprint). Keeping imgTypeCustomizations overlaps with this functionality and makes the code more complex.
I'm OK with merging this as is and iterating on the implementation.
pkg/distro/generic/images.go
Outdated
| // IMAGES | ||
|
|
||
| func diskImage(workload workload.Workload, | ||
| func diskImage(workload manifest.OSCustomizations, |
There was a problem hiding this comment.
In practice, we should not need to pass workload at all to any ImageKind generator. The custom workload used to carry user-specified packages, custom repos, and systemd unit config. Besides custom repos, all of these are already obtainable from the Blueprint, which is already passed as an argument to the generator.
| // Modules to install in addition to the ones required by the pipeline. | ||
| // These are the statically defined packages for the image type. | ||
| BaseModules []string |
There was a problem hiding this comment.
This is incorrect AFAICT. BaseModules aims to replace Workload.GetEnabledModules(), but enabling a module does not necessarily mean installing it. It must still be specified in the package list to be installed. Enabling modules means "making the module stream available for installation", so it is more like enabling a repository.
This commit fixes an incorrect comment in the description of OSCustomizations.BaseModules. Thanks to Thozza, c.f. osbuild#1770 (comment)
|
Fwiw, I agree to all points, working on a followup that will be a bit more involved (unfortunately) |
In osbuild#1770 we did the first step to remove the workload and replaced them with the (badly named) `imgTypeCustomizations`. This commit now removes this too and folds it into the existing OSCustomizations for clarity and to have one less thing to worry about. Thanks to Thozza and Simon for suggesting this.
This commit fixes an incorrect comment in the description of OSCustomizations.BaseModules. Thanks to Thozza, c.f. osbuild#1770 (comment)
In osbuild#1770 we did the first step to remove the workload and replaced them with the (badly named) `imgTypeCustomizations`. This commit now removes this too and folds it into the existing OSCustomizations for clarity and to have one less thing to worry about. The payload repositories are now passed to the image functions and get added to the OSCustomizations there as a new `PayloadRepos`. They will be used when resolving blueprint packages.
|
[..]
Thanks for the excellent feedback. I was indeed meant as a first step and I opened a followup now (and the naming was indeed pretty bad, sorry for that!), c.f. #1780 |
In osbuild#1770 we did the first step to remove the workload and replaced them with the (badly named) `imgTypeCustomizations`. This commit now removes this too and folds it into the existing OSCustomizations for clarity and to have one less thing to worry about. The payload repositories are now passed to the image functions and get added to the OSCustomizations there as a new `PayloadRepos`. They will be used when resolving blueprint packages.
Changes with 0.178.0 ---------------- - Update osbuild dependency commit ID to latest (osbuild/images#1763) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - many: drop `ISORootKickstart` (osbuild/images#1769) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Tomáš Hozza - many: drop the workload.Workload type entirely (osbuild/images#1770) - Author: Michael Vogt, Reviewers: Simon de Vlieger, Tomáš Hozza - platform: drop hardcoded platforms and rename PlatformConf (osbuild/images#1739) - Author: Michael Vogt, Reviewers: Brian C. Lane, Simon de Vlieger - rhel: fix openscap profile allowlists (HMS-9095) (osbuild/images#1778) - Author: Sanne Raymaekers, Reviewers: Michael Vogt, Simon de Vlieger — Somewhere on the Internet, 2025-08-21
This commit fixes an incorrect comment in the description of OSCustomizations.BaseModules. Thanks to Thozza, c.f. #1770 (comment)
Changes with 0.178.0 ---------------- - Update osbuild dependency commit ID to latest (osbuild/images#1763) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - many: drop `ISORootKickstart` (osbuild/images#1769) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Tomáš Hozza - many: drop the workload.Workload type entirely (osbuild/images#1770) - Author: Michael Vogt, Reviewers: Simon de Vlieger, Tomáš Hozza - platform: drop hardcoded platforms and rename PlatformConf (osbuild/images#1739) - Author: Michael Vogt, Reviewers: Brian C. Lane, Simon de Vlieger - rhel: fix openscap profile allowlists (HMS-9095) (osbuild/images#1778) - Author: Sanne Raymaekers, Reviewers: Michael Vogt, Simon de Vlieger — Somewhere on the Internet, 2025-08-21
In osbuild#1770 we did the first step to remove the workload and replaced them with the (badly named) `imgTypeCustomizations`. This commit now removes this too and folds it into the existing OSCustomizations for clarity and to have one less thing to worry about. The payload repositories are now passed to the image functions and get added to the OSCustomizations there as a new `PayloadRepos`. They will be used when resolving blueprint packages.
In #1770 we did the first step to remove the workload and replaced them with the (badly named) `imgTypeCustomizations`. This commit now removes this too and folds it into the existing OSCustomizations for clarity and to have one less thing to worry about. The payload repositories are now passed to the image functions and get added to the OSCustomizations there as a new `PayloadRepos`. They will be used when resolving blueprint packages.
Note that this is only a first step, we can do more here
(and probably should) but for now this is a really
low-hanging fruit. It ties into the whole discussion
of unifying workload/oscustomization/imageconfig
and seems like a step in the right direction to me.
But I'm open for discussion of course, maybe it
should be part of a bigger refactor, idk.
many: drop the workload.Workload type entirely
Workloads are no longer used in the images library and there
is no appetite to use them. So this commit removes the last
use of them: the workload.Custom{} that was used to pass
image type configuration to the images functions.
The workload.Custom is replaces with the regular
manifest.OSCustomizations that already contains (almost)
anything that is needed. So instead of having two concepts
to configure the image we consolidate it to one.
many: rename
workloadusage toimgTypeCustomizationsNow that the workload concept is removed and everything the
initial image type configuration is done via OSCustomization
we need to rename "workload" as it will be confusing without
the historic context (and arguably it was always a bit
confusing).
The new name needs to capture that the OSCustomizations
applied here come from the initial imagetype. I picked
imgTypeCustomizationsas it seems to capture bestwhat this is doing. But its long and I'm open for more
ideas, I initially used
seedas its short but itsprobably too abstract and it captures the concept poorly.
Its also unclear how long-lived this name will be,
ideally it all will be merged into a single OSCustomizations
variable inside the various images but that needs to
happen in followups or the churn is too big.