-
Notifications
You must be signed in to change notification settings - Fork 45
Add sp version info to omdb nexus update-status
#8517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Some local testing with
|
Ok(()) | ||
} | ||
|
||
fn print_zones(zones: impl Iterator<Item = (SledUuid, Vec<ZoneStatus>)>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was all just moved but didn't change.
println!("{}", table); | ||
} | ||
|
||
fn print_sps(sps: impl Iterator<Item = (String, SpStatus)>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new.
slot0_version: String, | ||
slot0_git_commit: String, | ||
slot1_version: String, | ||
slot1_git_commit: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also include the system version associated with each of these SP versions (if we know it)?
IIRC for zones, the "version" we report is based on mapping the hash reported in inventory that is then mapped back to a system version (from a TUF repo). In the SP case, these versions are reported by the SP, but they're hubris release versions, and have no direct tie to a system version, right? So to know whether an update is complete or in progress, the consumer of this would have to know or find what hubris versions of different artifacts are present in the new repo. But I think Nexus already knows this and could include it for us to show here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 Hmm, Should I just return the actual version if known? No real need for the other versions now that I think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the change to only include the TUF repo version in 3601261
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - thanks!
); | ||
} | ||
} | ||
if let Some(new) = new.tuf_repo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible (albeit unlikely) SP versions don't change between releases - maybe put this one before checking old
, so we report the newer if it matches both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! One other thing, is that testing this with omicron-dev does not seem to be doing what I expect. I also changed to parse the ArtifactVersion
in case to_string
was not symmetric with parse
. Didn't make a difference. I think this is likely just an artifact of my test environment, but can't be sure without testing on a4x2 or a racklette.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in dce506f
f4187f8
to
dce506f
Compare
Fixes #8485