Conversation
Johan-Liebert1
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements. The addition of structured logging across the composefs backend will significantly enhance observability. The refactoring to support both sealed and unsealed UKIs, along with storing the full UKI command line, adds important flexibility for testing and future use cases. Furthermore, the updates to the Justfile and xtask infrastructure to accommodate these new configurations are well-executed, consolidating multiple test recipes into a more maintainable, parameterized approach. My review includes one suggestion to improve code clarity in the xtask logic.
Johan-Liebert1
force-pushed
the
pr-followups
branch
5 times, most recently
from
e89e9e6 to
9b31c66
Compare
|
Something's wrong with unsealed UKI case. Need to look into it |
|
Also, we don't need unsealed + ext4 as the xfs case covers it. I'll remove that from matrix |
|
Perhaps split out the first two commits separate from the larger test changes? |
|
Split out the first two commits to #2032 |
Johan-Liebert1
force-pushed
the
pr-followups
branch
2 times, most recently
from
cdae3da to
cc25035
Compare
seal-state: Required to switch between secure/insecure firmware options boot-type: Required to send kargs to only bls installs Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Update the CI matrix to include `seal_state` and `boot_type`. This does not increase our matrix, but only rearranges it to be a bit more meaningful. Earlier even when testing "insecure UKI", it still showed up as "composefs-sealeduki-sdboot" which is incorrect. This also allows us flexibility to, in future, test grub + UKI which is disabled currently. Update Justfile and the Dockerfile to make use of these new arguments. Now we only sign the UKI, if `seal_state == sealed`, and in the Justfile we disallow combinations that don't make sense, like BLS boot + sealed, allowing missing verity (xfs) + sealed, etc. Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
pr-followups
branch
from
cc25035 to
c38284f
Compare
Remove "composefs-sealeduki-sdboot" variant Only exclude ext4-unsealed-uki as we still want to run tests on unsealed bls ext4 systems Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
pr-followups
branch
from
c38284f to
b2bc375
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Awesome!
I have to say as we keep growing the matrix here it sure is easy to spend Other People's Money in CI 😄 💸
It's mainly painful for us with flakes.
But...it shouldn't be too bad in theory in the future to also limit which variants get run on which PRs e.g. opt-in via label or other mechanism.
| test_os: [fedora-43, centos-9, centos-10] | ||
| variant: [ostree, composefs-sealeduki-sdboot, composefs-sdboot, composefs-grub] | ||
| variant: [ostree, composefs] | ||
| filesystem: ["ext4", "xfs"] |
There was a problem hiding this comment.
as an aside we should probably have btrfs in this list too
There was a problem hiding this comment.
yes, but that will add a bunch more combinations to the matrix
| if let Some(variant) = std::env::var("BOOTC_variant").ok() { | ||
| match variant.as_str() { | ||
| v @ "ostree" | v @ "composefs" => { | ||
| match (variant.as_str(), is_uki) { |
There was a problem hiding this comment.
Nonblocking but for followup: we could also validate the sealed/unsealed state here right?
2 participants
xtask: Add
seal-stateandboot-typeoptionsseal-state: Required to switch between secure/insecure firmware options
boot-type: Required to send kargs to only bls installs
composefs/tests: More flexibility for insecure UKI testing
Update the CI matrix to include
seal_stateandboot_type. This doesnot increase our matrix, but only rearranges it to be a bit more
meaningful. Earlier even when testing "insecure UKI", it still showed up
as "composefs-sealeduki-sdboot" which is incorrect.
This also allows us flexibility to, in future, test grub + UKI which is
disabled currently.
Update Justfile and the Dockerfile to make use of these new arguments.
Now we only sign the UKI, if
seal_state == sealed, and in the Justfilewe disallow combinations that don't make sense, like BLS boot + sealed,
allowing missing verity (xfs) + sealed, etc.