many: bootc deployment variants - #2636
Conversation
Add deployment variant support to osinfo config loading. When a variant name is provided we look in `<prefix>/variant.d/<name>` and fall back to the default path if it doesn't exist. See [1]. [1]: osbuild#2566 Signed-off-by: Simon de Vlieger <cmdr@supakeen.com>
Variant parameter for `Container.ResolveInfo()` and `ResolveBootcInfo()`. Also allow for resolving available variants from a given container reference. Signed-off-by: Simon de Vlieger <cmdr@supakeen.com>
Allow passing `--bootc-variant` to `bootc` relevant commands to select a variant. Also add `bootc variants` to list variants available in a given reference. Signed-off-by: Simon de Vlieger <cmdr@supakeen.com>
b5facb1 to
c4a073a
Compare
lucasgarfield
left a comment
lucasgarfield
left a comment
There was a problem hiding this comment.
Pretty easy for me to follow so I suppose that means it's pretty nice. 😅
Only one real concern.
I could imagine a bit more test coverage but am not yet up to speed on the repo's conventions so I'm not asking for extra coverage - I'd be curious for you to comment on it, though, just for my own information.
| } | ||
|
|
||
| func readDiskYaml(fsys fs.FS) (*diskYAML, error) { | ||
| func readDiskYaml(fsys fs.FS, variant string) (*diskYAML, error) { |
There was a problem hiding this comment.
Right now a default disk.yaml in the image-builder prefix takes precedence over a variant disk.yaml in the bootc-image-builder prefix. This feels like a bug, since it means that bootc variants can list name(s) that --bootc-variant won't actually end up using.
e.g. imagine you have
/usr/lib/image-builder/bootc/disk.yaml
and
/usr/lib/bootc-image-builder/variant.d/btrfs/disk.yaml
It would return as soon as it found /usr/lib/image-builder/bootc/disk.yaml, ignoring the btrfs variant.
Should be an easy fix – just invert the nesting so we loop over [variant, default] on the outside and searchPaths on the inside.
There was a problem hiding this comment.
I don't agree with the solution. What you describe is indeed the behavior but I think mixing two different prefixes and trying to merge them could maybe be wrong-er here. That said: we document that the new prefix takes precedence over the old one 1 so it isn't super weird that if a disk.yaml exists in that prefix that it wins over anything in the compatibility prefix.
What we should (probably) start doing is to emit a warning when we detect a container that has files in the old prefix path. I've filed 2 for this.
As for how to solve this with the above in mind I think there are three options:
- Leave as-is.
- Don't use both prefixes for any variant handling only
/usr/lib/image-builder. - Adjust
ListVariantsto not continue to the compatibility prefix when/usr/lib/image-builderexists.
What has your preference? For me it's the the last one and we could probably apply it to all cases where we check both prefixes.
There was a problem hiding this comment.
By the way, I initially thought that merging the variants across prefixes was nice (it even has a test!) but your comment made me reconsider 🙂.
| } | ||
|
|
||
| func readISOYaml(fsys fs.FS) (*isoYAML, error) { | ||
| func readISOYaml(fsys fs.FS, variant string) (*isoYAML, error) { |
There was a problem hiding this comment.
Same issue as readDiskYaml above.
What test coverage would you like to see? It has a bunch of unit tests for the variant YAML finding but no actual build tests. Integration tests on the manifest generation? To /me/ it feels like overkill but I am known to avoid them :) |
2 participants
Implementation of #2566; I've left the path to be under
/usr/lib/image-builder/bootcas I don't want to change the path again and we don't have a use case for package based trees yet.Commits should be self explanatory see the issue for the reasoning.