Repository navigation
Add supported and required blueprint options to RHEL image types [HMS-9248] - #1855
Conversation
lzap
left a comment
There was a problem hiding this comment.
That is a lot of work indeed. I scanned the code, could not tell what is supported or not. LGTM
| } | ||
| if diunSet != 1 { | ||
| return warnings, fmt.Errorf("%s: one of customizations.fdo.diun_pub_key_hash, customizations.fdo.diun_pub_key_insecure, customizations.fdo.diun_pub_key_root_certs: required when using fdo", errPrefix) | ||
| } |
There was a problem hiding this comment.
Oh I haven't realized they all do the same thing. Noted osbuild/blueprint-schema#56
There was a problem hiding this comment.
Thanks for catching this!
mvo5
left a comment
There was a problem hiding this comment.
Thanks for doing this! Some quick feedback/question (I only went to rhel8 because I feel my questions will be the same for rhel9/10 :)
| - "grub2-efi-x64" | ||
| - "shim-x64" | ||
| supported_blueprint_options: &supported_blueprint_options | ||
| - "distro" |
There was a problem hiding this comment.
(not related to this PR) silly question, wouldn't "distro" be an implicit option that is accepted everywhere (just like name)?
There was a problem hiding this comment.
I don't know. I think when we discussed the options we decided that maybe distro could be something we don't want on some image types, but I can't imagine why.
|
|
||
| var warnings []string | ||
|
|
||
| // we do not support embedding containers on ostree-derived images, only on commits themselves |
There was a problem hiding this comment.
Its great to see so much removal here! Will we see even more in a followup? Looking at the checkOptionsRhelX it seems there is potential to move some into checkOptionsCommon (happy to look at this if you are busy!)
There was a problem hiding this comment.
I have some follow-ups planned and merging all the checkOptions<distro> into one is definitely on the list.
| case "edge-raw-image": | ||
| assert.EqualError(t, err, fmt.Sprintf("%q images require specifying a URL from which to retrieve the OSTree commit", imgTypeName)) | ||
| case "edge-installer", "edge-simplified-installer": | ||
| assert.EqualError(t, err, fmt.Sprintf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", imgTypeName)) |
There was a problem hiding this comment.
Did we loose the ostree.url: required" for "edge-installer", "edge-simplified-installer": tests here? I tried to quickly look if this is covered now elsewhere but couldn't
There was a problem hiding this comment.
No, it just happens after the blueprint check now: https://github.com/achilleas-k/images/blob/5e4403cf6fa7e51489ada0adb751ef65f1a58f2d/pkg/distro/generic/options.go#L125-L133
Happy to change the order if you think it's important.
| switch imgTypeName { | ||
| case "edge-commit", "edge-container", "edge-installer", "image-installer", "netinst", "tar", "wsl": | ||
| assert.EqualError(t, err, fmt.Sprintf("blueprint validation failed for image type %q: customizations.filesystem: not supported", imgTypeName)) | ||
| case "edge-simplified-installer": |
There was a problem hiding this comment.
Maybe I'm missing something but the test says "TestRH8_Distro_CustomFileSystemManifestError()" but here we are testing a different error (that the installation_device is required). Would it make sense to either adjust the test so that the bueprint for edge-simplified-installer has a installation_device and hence we get the error about "customizations.filesystem: not supported"" or just skip these image types in this test (if this is tested elsewhere?)
Same question for the other tests below where the testname/parts of the checks mismatch.
There was a problem hiding this comment.
Yeah, I wanted to keep the tests the same (ish), but I also felt weird testing different error messages while doing this. The previous versions of the tests were checking for the specific error message that was relevant to the test and then allowing any error message for every other case, which I felt left it too open to missing error cases that we might not want. I think merging all these errors into one larger test that checks for all sorts of scenarios would be better. But that's another thing for a follow-up, if you agree.
The Fedora minimal-installer creates an OS tree in a tarball that gets embedded in an ISO and configures anaconda to install it. This tarball supports almost all the options of a disk image type (excluding things that are related to partitioning and bootloaders). Fixes osbuild#1849
Define the supported blueprint options for all the image types in RHEL 7. These are almost the same as the ones added to the disk image types in Fedora, minus containers and openscap, which are generally not supported in our RHEL 7 images. The checkOptionsRhel7() function has also been updated to match the Fedora version, but trimmed down to not include checks for ostree and ISOs.
Define the supported and required blueprint options for all the image types in RHEL 8. These are very similar to the ones added to the image types in Fedora, with a few exceptions: - Ignition is not supported for the edge-simplified-installer. - RHEL distros define a "tar" image type that requires its own set of supported options. - RHEL distros support customizations.rhsm. The checkOptionsRhel8() function has also been updated to match the Fedora and RHEL 7 version, with modifications for the RHEL 8 (e.g. swap creation on aarch64).
Define the supported and required blueprint options for all the image types in RHEL 9. These are very similar to the ones added to the image types in RHEL 8, with a few exceptions: - Ignition is supported for the edge-simplified-installer. The checkOptionsRhel9() function has also been updated to match the new functions for Fedora, RHEL 7, and RHEL 8, with appropriate modifications.
Define the supported and required blueprint options for all the image types in RHEL 10. These are very similar to the ones added to the image types in RHEL 9. The checkOptionsRhel10() function has also been updated to match the new functions for Fedora, RHEL 7, RHEL 8, and RHEL 9 with appropriate modifications (such as not checking for options associated with ostree and edge image types).
Only one of the fdo sub-options is supported at a time, no more, no less, so let's make that clear in the error message. Thanks to Michael for the suggestion.
5e4403c to
c18829b
Compare
Thanks for adding the note, @mvo5. I forgot about that :) |
NOTE: contains #1854
Define the supported and required blueprint options for all the image types in RHEL 7, 8, 9, and 10.
The checkOptionsRhelX() functions have also been updated to match the function for Fedora with appropriate modifications.