-
Notifications
You must be signed in to change notification settings - Fork 85
ListMachineSummaries - Add DNS names to Part summaries #745
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
Conversation
proto/viam/app/v1/app.proto
Outdated
@@ -1160,6 +1160,7 @@ message LocationSummary { | |||
message MachineSummary { | |||
string machine_id = 1; | |||
string machine_name = 2; | |||
optional string main_part_dns_name = 4; |
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 makes sense to surface here, but then we should also add main_part_name here as well
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.
Does main_part_name
mean the thing one would use to dial? Is main_part_dial_name
perhaps better? I feel that a "name" could be anything and we want to be clear if the string is meant to plug into something specific.
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.
Adding the main part name feels wrong. We could add the main part ID if needed
Or we can just add a is_main_part
boolean on the PartSummary
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.
@dgottlieb main_part_dns_name
was intended to be the string you use to dial. We have a dns_name
on the Part
proto struct.
I am open to alternative names:
main_part_hostname
main_part_url
main_part_dial_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.
with is_main_part
I don't think we need to add main_part_dns_name
. tho not sure how users are most likely to want this data
https://viam.atlassian.net/browse/APP-9174