[4/n] [sled-agent] add code to read mupdate overrides #8081
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
sunshowers
changed the title
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1 [skip ci]
|
some review notes:
|
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
There was a problem hiding this comment.
LGTM, just a couple small nits/questions.
I feel like we should extract this out into a common module at some point -- it feels like a somewhat different use case from Ledgerable though still related to it
I wonder if Ledgerable could/should use logic like this PR does instead of its current behavior. Ledgerable kinda throws caution to the wind in terms of "what if something weird is happening with the internal disks", and I think it would be fine for it treat the boot disk as authoritative in such a situation?
I think we should not actually do that work any time soon, but "make this usable by both this use case and all the Ledgerable use cases" might be a reasonable driver for extracting something generic.
| // Log the results. | ||
| mupdate_overrides.log_results(&log); |
There was a problem hiding this comment.
Tiny nit - why not do this implicitly inside read_all?
There was a problem hiding this comment.
Seems reasonable, fixed.
| dataset_dir: &Utf8Path, | ||
| override_path: &Utf8Path, | ||
| ) -> Result<Option<MupdateOverrideInfo>, MupdateOverrideReadError> { | ||
| // First check that the dataset directory exists. It would be nice if we |
There was a problem hiding this comment.
I was going to ask why we want to check the directory exists instead of just trying to read the file. I think this is to distinguish "the install dataset isn't here at all" (an error) from "the install dataset exists but doesn't have the override file" (not an error)? If that's right, it might be worth noting in this comment.
There was a problem hiding this comment.
Expanded the comment.
| }, | ||
|
|
||
| #[error( | ||
| "error deserializing into MupdateOverrideInfo, contents: {contents:?}" |
There was a problem hiding this comment.
Do we want path in this error string?
There was a problem hiding this comment.
Yep, fixed, thanks.
sunshowers
changed the base branch from
sunshowers/spr/main.sled-agent-add-code-to-read-mupdate-overrides
to
main
Created using spr 1.3.6-beta.1
sunshowers
deleted the
sunshowers/spr/sled-agent-add-code-to-read-mupdate-overrides
branch
Don't make any decisions based on this at the moment -- simply read it for now. Decisions will happen in upcoming commits.
The logic here became quite complex, including tests, and I'm wondering if it would make sense to extract this into something more generic now or in the future.
Depends on: