Skip to content

RSDK-6829 - Add machine part id to ResourceName message #467

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.

Sign up for GitHub

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

Merged
Cheuk (cheukt) merged 6 commits into from
Apr 1, 2024
Merged

RSDK-6829 - Add machine part id to ResourceName message #467

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2091aa6
change
cheukt Mar 8, 2024
1f19da9
robot -> machine
cheukt Mar 29, 2024
211cca3
Merge branch 'main' into robot-part-id
cheukt Mar 29, 2024
a828f61
test
cheukt Mar 29, 2024
8e6cd53
Built new protos from a828f61
github-actions[bot] Mar 29, 2024
211e60b
Merge branch 'main' into robot-part-id
cheukt Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions proto/viam/common/v1/common.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ message ResourceName {
string type = 2;
string subtype = 3;
string name = 4;
optional string machine_part_id = 5;

@maximpertsov Maxim Pertsov (maximpertsov) Mar 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why optional?

@cheukt Cheuk (cheukt) Mar 29, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything else in the resource name should always be filled out, so using optional here to be a bit more clear about the optionality. It doesn't seem like we are super consistent about this though, so could see making it a non-optional field as well

@maximpertsov Maxim Pertsov (maximpertsov) Mar 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that was gonna be my next question - it doesn't seem like we are consistent with using the optional keyword vs relying on implicit optionality in protos (e.g. this field will just be a blank string)

@maximpertsov Maxim Pertsov (maximpertsov) Mar 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't have have a strong opinion on this, so won't block - I'm okay making this explicitly optional

@cheukt Cheuk (cheukt) Mar 29, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to start using optional a bit more, I think it makes it easier to tell what the response shape is like by just reading the proto

@benjirewis Benjamin Rewis (benjirewis) Apr 1, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not also want the machine ID? Or just the part ID?

@cheukt Cheuk (cheukt) Apr 1, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just part id for now, that's what was scoped and most likely to be useful - user can get cloud metadata if they need more of that stuff

}

message BoardStatus {
Expand Down