Add hierarchical support for storage fields #504
Conversation
| } | ||
|
|
||
| if len(rc.Config.Storage.Verity) > 0 || len(im.baseImageVerityMetadata) > 0 { | ||
| if len(rc.Storage.Verity) > 0 || len(im.baseImageVerityMetadata) > 0 { |
There was a problem hiding this comment.
There are still a lot of references to Config.Storage in the codebase. These need to be redirected to ResolvedConfig.Storage instead. In VS Code, you can right click a field and select Find All References.
Also, you'll probably want to delete the Config.CustomizePartitions() function, since that contains a hidden reference to Config.Storage.
Also, the fact that you missed so much means that you need to expand your test coverage.
| return nil | ||
| } | ||
|
|
||
| func (s *Storage) CustomizePartitions() bool { |
There was a problem hiding this comment.
It would be good to keep the Storage.CustomizePartitions function.
| if baseHasDisks { | ||
| if hasResetUUID { | ||
| return fmt.Errorf( | ||
| "cannot specify 'resetPartitionsUuidsType' in config at layer %d when a base config specifies '.storage.disks'", i) |
There was a problem hiding this comment.
I don't think at layer %d is particularly meaningful to the user, since they see the config tree and not the list. If possible, it might be nice to give the config file names. If not, I would just remove at layer %d.
| resolvedStorage.Disks = storage.Disks | ||
| resolvedStorage.BootType = storage.BootType | ||
| resolvedStorage.FileSystems = storage.FileSystems | ||
| resolvedStorage.Verity = storage.Verity |
There was a problem hiding this comment.
resolvedStorage,VerityPartitionsType needs to also be set.
FYI: storage.VerityPartitionsType is populated by Storage.IsValid(). So, you can use that value instead of recalculating it.
|
|
||
| // .storage.resetPartitionsUuidsType - override | ||
| if storage.ResetPartitionsUuidsType != imagecustomizerapi.ResetPartitionsUuidsTypeDefault { | ||
| resolvedStorage.ResetPartitionsUuidsType = storage.ResetPartitionsUuidsType |
There was a problem hiding this comment.
If you are iterating backwards through the chain, you need to use early return. Otherwise, you will pick the first value in the chain instead of the last.
That being said, I think the storage merging logic is complex enough that it would probably be better to merge storage using recursion on the config tree. (The other fields have simple merging logic. So, going backwards through the linear chain works fine.)
There was a problem hiding this comment.
I updated func resolveStorage to use early return. I think merge storage using recursion might add complexity since it will duplicate work of loading and flattening the config tree (we obtained configChain already). Let me know if I misunderstood your comment
| hasResetUUID := storage.ResetPartitionsUuidsType != imagecustomizerapi.ResetPartitionsUuidsTypeDefault | ||
| hasReinitVerity := storage.ReinitializeVerity != imagecustomizerapi.ReinitializeVerityTypeDefault | ||
|
|
||
| if baseHasDisks { |
There was a problem hiding this comment.
Is it okay to have .storage.disks settings when baseHasDisks is true?
| storage := configWithBase.Config.Storage | ||
|
|
||
| hasDisks := storage.CustomizePartitions() | ||
| hasResetUUID := storage.ResetPartitionsUuidsType != imagecustomizerapi.ResetPartitionsUuidsTypeDefault |
There was a problem hiding this comment.
nit: these local variables are redundant to have, can change it to something like:
if storage.ResetPartitionsUuidsType != imagecustomizerapi.ResetPartitionsUuidsTypeDefault {
Checklist