-
Notifications
You must be signed in to change notification settings - Fork 54
Add measurement corpus for installation via wicket #8852
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
06aab1e
to
a557323
Compare
fd54c92
to
78974d0
Compare
78974d0
to
02578df
Compare
02578df
to
b2b01bf
Compare
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.
(reviewed on my phone, full review later)
@@ -160,6 +189,9 @@ async fn fetch_hash( | |||
struct Manifest { | |||
#[serde(rename = "artifact")] | |||
artifacts: HashMap<KnownArtifactKind, Vec<Artifact>>, | |||
// Add a default for backwards compatibility | |||
#[serde(rename = "measurement_corpus")] | |||
corpus: Option<Source>, |
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 can this be part of the artifacts map above?
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 could. This may be a hold over from very early drafts of this work where I was trying to be backwards compatible. I'll give this a test on the SP/RoT end.
/// A component that means "all possible components (host phase 2, control | ||
/// plane, and measurements)", used for writes for now. It is possible | ||
/// that this component will go away in the future. | ||
All, |
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 this might run into issues if the variant is unknown to the old Wicketd version. I'd serialize this as "both" for now.
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.
ahhh yeah I think you're right. I was trying to improve the clarity but I'll keep it here for now.
@@ -205,6 +211,7 @@ impl fmt::Display for WriteComponent { | |||
Self::HostPhase2 => f.write_str("host phase 2"), | |||
Self::ControlPlane => f.write_str("control plane"), | |||
Self::Unknown => f.write_str("unknown"), | |||
Self::MeasurementCorpus => f.write_str("measurement corpus"), |
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.
Could you move MeasurementCorpus to above Unknown?
@@ -320,13 +329,16 @@ impl InstallOpts { | |||
StepHandle::ready(ArtifactsToDownload { | |||
host_phase_2, | |||
control_plane, | |||
// We only support measurement corpus in the installinator document |
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.
We should get rid of this path I think — could you file an issue for that?
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 will be installed for wicket via the installinator document. For now, the measurements are treated as a subset of the control plane installation procedure. This may change as we add reconfigurator support.
No description provided.