-
Notifications
You must be signed in to change notification settings - Fork 8
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
Yield more information about services to client #30
Conversation
7fe893e
to
8d2d472
Compare
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Signed-off-by: Alexander V. Nikolaev <[email protected]>
b5ee0d7
to
399bc28
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.
The changes seem reasonable; added some stylistic comments.
pub fn vm_name(&self) -> Option<&str> { | ||
match &self.placement { | ||
Placement::Endpoint { vm, .. } => Some(vm), | ||
Placement::Managed { vm, .. } => Some(vm), |
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.
These branches could be combined with |
, either way is fine though.
@@ -103,11 +129,17 @@ impl From<RegistryEntry> for QueryResult { | |||
} else { | |||
VMStatus::PoweredOff | |||
}; | |||
let vm_name = val.vm_name().map(|s| s.to_owned()); |
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 use ToOwned::to_owned
or str::to_owned
instead of closure.
@@ -51,6 +56,12 @@ impl TryFrom<pb::QueryListItem> for QueryResult { | |||
.with_context(|| format!("While parsing vm_status {}", item.vm_status))?, | |||
trust_level: TrustLevel::from_str(item.trust_level.as_str()) | |||
.with_context(|| format!("While parsing trust_level {}", item.trust_level))?, | |||
vm_type: VmType::from_str(item.vm_type.as_str()) |
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 item.vm_type.parse().with_context(...)
would be more readable.
.await | ||
.with_context(|| format!("handing error, by restart VM {}", entry.name))?; | ||
if let Placement::Managed { vm: vm_name, .. } = entry.placement { | ||
self.start_vm(&vm_name) |
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.
Not sure if it makes a difference, but could move the &
to above: &entry.placement
to ensure no move occurs.
Placement::Managed(within) if within == name => Some(re.name.clone()), | ||
_ => None, | ||
.filter_map(|re| { | ||
if re.agent_name() == Some(name) || re.vm_name() == Some(name) { |
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.
Maybe (re.agent_name() == Some(name) || re.vm_name() == Some(name)).then(|| re.name.clone())
Tried to make it a one-liner, but the shortest I got was: .filter_map(|e| [e.agent_name(), e.vm_name()].contains(&Some(name)).then(|| e.name.clone()))
which rustfmt still splits onto multiple lines :(
Signed-off-by: Alexander V. Nikolaev <[email protected]>
Description
Yield more information about services to client
Checklist
nix flake check --accept-flake-config
and it passesTesting