engineering: Improve COSI Metadata Parsing #394
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves COSI metadata parsing by enhancing type safety and forward compatibility. The changes introduce strongly-typed enums for bootloader types and versions, replace string comparisons with enum matching, and remove deny_unknown_fields to allow graceful handling of future metadata fields.
Key changes:
- Introduced
KnownMetadataVersionenum to track supported COSI specification versions - Replaced string-based bootloader type checks with
BootloaderTypeandSystemdBootLoaderTypeenums - Removed
deny_unknown_fieldsfrom all structs for forward compatibility
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Unknown bootloader type is invalid | ||
| (BootloaderType::Unknown(bootloader_type), _) => { | ||
| bail!("Unsupported bootloader type: {}", bootloader_type); |
There was a problem hiding this comment.
This should only bail if the version is equal to 1.1. Otherwise we can't add new bootloader types.
There was a problem hiding this comment.
This is more about this version of trident not recognizing this bootloader type. If we see something we don't recognize then we have no idea what to do with it?
There was a problem hiding this comment.
I'd feel better about running this once we know the specific operation we're doing with the COSI. Perhaps directly in the validate() method of the subsystem that is using the bootloader field?
For something like trident stream-image it might not matter and would present a forward compatibility concern
There was a problem hiding this comment.
Fair. This is the existing logic that was implemented before, but happy to revise for forward compat.
There was a problem hiding this comment.
I did some extra future proofing, basically we will always validate the metadata as long as it's technically valid, regardless of the support offered by the version of Trident.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // First entry MUST be of type 'uki-standalone' | ||
| [entry, ..] if !entry.boot_type.eq(&SystemdBootloaderType::UkiStandalone) => warn!( | ||
| "First entry of bootloader type 'systemd-boot' is not of type 'uki-standalone'" | ||
| ), | ||
|
|
||
| // Having more than one entry is warned about, only the first is used in this version of Trident. | ||
| [_, rest @..] if !rest.is_empty() => warn!( | ||
| "Bootloader type 'systemd-boot' has more than one entry, only the first entry will be used" | ||
| ), | ||
|
|
||
| // Everything else is OK |
There was a problem hiding this comment.
The pattern matching logic has overlapping cases that prevent both warnings from being emitted when both conditions are true. When there are multiple entries AND the first entry is not uki-standalone, only the warning about the first entry type is issued, but the warning about multiple entries is silently skipped.
For example, if entries are [config, uki-standalone], the user will only see a warning about the first entry not being uki-standalone, but won't be warned that only the first entry will be used.
Consider restructuring the logic to check both conditions independently, or reordering the patterns so that the multiple entries warning is checked first, followed by a separate check for the first entry type.
| // First entry MUST be of type 'uki-standalone' | |
| [entry, ..] if !entry.boot_type.eq(&SystemdBootloaderType::UkiStandalone) => warn!( | |
| "First entry of bootloader type 'systemd-boot' is not of type 'uki-standalone'" | |
| ), | |
| // Having more than one entry is warned about, only the first is used in this version of Trident. | |
| [_, rest @..] if !rest.is_empty() => warn!( | |
| "Bootloader type 'systemd-boot' has more than one entry, only the first entry will be used" | |
| ), | |
| // Everything else is OK | |
| // For one or more entries, validate the first entry type and the number of entries independently. | |
| [first, rest @ ..] => { | |
| // First entry MUST be of type 'uki-standalone' | |
| if !first | |
| .boot_type | |
| .eq(&SystemdBootloaderType::UkiStandalone) | |
| { | |
| warn!( | |
| "First entry of bootloader type 'systemd-boot' is not of type 'uki-standalone'" | |
| ); | |
| } | |
| // Having more than one entry is warned about, only the first is used in this version of Trident. | |
| if !rest.is_empty() { | |
| warn!( | |
| "Bootloader type 'systemd-boot' has more than one entry, only the first entry will be used" | |
| ); | |
| } | |
| } | |
| // Everything else is OK (should be unreachable due to the above patterns) |
|
|
||
| // Unknown bootloader type is warned about, it may cause issues down the line | ||
| (BootloaderType::Unknown(bootloader_type), _) => { | ||
| log::warn!("Unknown bootloader type: {}", bootloader_type) |
There was a problem hiding this comment.
Inconsistent use of the warn macro. Line 74 uses log::warn! while the warn macro is imported at the top of the file (line 3) and used without the prefix on lines 58 and 63. For consistency, this should be changed to just warn! to match the import and other usages in the file.
| log::warn!("Unknown bootloader type: {}", bootloader_type) | |
| warn!("Unknown bootloader type: {}", bootloader_type) |
🔍 Description