Skip to content
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

Jump to bottom

add last updated to fragment proto #540

Merged
merged 5 commits into from
Aug 14, 2024
Merged

add last updated to fragment proto #540

merged 5 commits into from
Aug 14, 2024

Conversation

puranamp
Copy link
Contributor

@puranamp puranamp commented Aug 9, 2024 •
edited
Loading

needed for this

@github-actions github-actions bot added the safe to test committer is a member of this org label Aug 9, 2024
@puranamp puranamp marked this pull request as ready for review August 9, 2024 16:22
@puranamp puranamp added the ready-for-protos add this when you want protos to compile on every commit label Aug 9, 2024
@puranamp puranamp removed the ready-for-protos add this when you want protos to compile on every commit label Aug 9, 2024
@puranamp puranamp requested review from michaellee1019 and removed request for piokasar August 9, 2024 19:33
michaellee1019
michaellee1019 approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@michaellee1019 michaellee1019 left a comment

Choose a reason for hiding this comment

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

Fine with this change. Technically you could achieve what you need to do in your project without any API changes. Internally can store the last_updated field, and return that field as part of the config response to the robot as the config version. Doing this API work makes the APIs match what is stored on the Fragment, so this is good practice.

I can send out this API change as an email to the scope, any API change made is supposed to be in a scope/email.

@michaellee1019
Copy link
Member

I just looked at other protos. Don't we have the same issue for RobotPart and Location as well? Wondering if we want to expand this PR to handle those and do it all at once.

I came to realize this when reviewing the naming of other fields (one usage named last_updated and another updated_at).

@puranamp puranamp added the ready-for-protos add this when you want protos to compile on every commit label Aug 12, 2024
@puranamp puranamp removed the ready-for-protos add this when you want protos to compile on every commit label Aug 12, 2024
@puranamp puranamp added the ready-for-protos add this when you want protos to compile on every commit label Aug 12, 2024
@puranamp puranamp removed the ready-for-protos add this when you want protos to compile on every commit label Aug 12, 2024
@puranamp puranamp added the ready-for-protos add this when you want protos to compile on every commit label Aug 13, 2024
@puranamp puranamp added ready-for-protos add this when you want protos to compile on every commit and removed ready-for-protos add this when you want protos to compile on every commit labels Aug 13, 2024
@puranamp puranamp removed the ready-for-protos add this when you want protos to compile on every commit label Aug 13, 2024
michaellee1019
michaellee1019 approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@michaellee1019 michaellee1019 left a comment

Choose a reason for hiding this comment

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

LGTM! I suggest adding a comment for the robot part field. Comments show up in editors when you are interfacing with the proto objects.

I sent an email mentioning the proto change to scope reviewers.

proto/viam/app/v1/app.proto
@@ -254,6 +254,7 @@ message RobotPart {

// List of secrets allowed for authentication.
repeated SharedSecret secrets = 14 [(tagger.v1.tags) = "bson:\"secrets\""];
google.protobuf.Timestamp last_updated = 15 [(tagger.v1.tags) = "bson:\"last_updated_at\""];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
google.protobuf.Timestamp last_updated = 15 [(tagger.v1.tags) = "bson:\"last_updated_at\""];
// latest timestamp when robot part was updated
google.protobuf.Timestamp last_updated = 15 [(tagger.v1.tags) = "bson:\"last_updated_at\""];

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Pranav Puranam seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@puranamp puranamp added the ready-for-protos add this when you want protos to compile on every commit label Aug 14, 2024
@puranamp puranamp removed the ready-for-protos add this when you want protos to compile on every commit label Aug 14, 2024
@puranamp puranamp merged commit abe5a39 into viamrobotics:main Aug 14, 2024
2 of 3 checks passed
abe-winter pushed a commit that referenced this pull request Aug 14, 2024
@puranamp @github-actions @abe-winter
add last updated to fragment proto (#540)
4c89137 
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Pranav Puranam <pranav.puranam@gmail.com>
maxhorowitz added a commit that referenced this pull request Sep 16, 2024
@maxhorowitz
Merge branch 'main' into app-6230
32e5e34 
* main: (22 commits)
  DATA-3094: Add CLI args to SubmitCustomTrainingJobRequest (#550)
  bump min golang version (#548)
  DATA-3054: Added UpdateBoundingBox (#547)
  RSDK-8595 update docs with new behavior (#546)
  APP-3604 build endpoints for external oauth and repos (#544)
  add last updated to fragment proto (#540)
  APP-5851: Add optional visibility field to `CreateFragmentRequest` (#543)
  APP-5314 - add dates and limit to GetRobotPartLogsRequest (#538)
  RSDK-7903 Add unhealthy state and error field (#539)
  RSDK-8381 add viam server version to API (#537)
  [APP-5417]: Undo APP-5417 (#536)
  [APP-5417]: Accommodate pagination for GetRobotPartHistory in api (#535)
  Data 2816/get training job logs (#534)
  DATA-2792 - Add URL to UpdateRegistryItemRequest, but make it optional (#532)
  RSDK-8228 Add config status to machine status API (#531)
  Add revision field to config (#530)
  [APP-5420] Add ListMachineFragments request and response (#525)
  [APP-5400]: Add GetFragmentHistory proto (#524)
  RSDK-7899 Add MachineStatus to RobotService (#527)
  RSDK-8083: add LogPatternConfig field to RobotConfig API (#526)
  ...
maxhorowitz added a commit that referenced this pull request Sep 16, 2024
@maxhorowitz
Merge branch 'main' into app-6230
ac28de1 
* main: (22 commits)
  DATA-3094: Add CLI args to SubmitCustomTrainingJobRequest (#550)
  bump min golang version (#548)
  DATA-3054: Added UpdateBoundingBox (#547)
  RSDK-8595 update docs with new behavior (#546)
  APP-3604 build endpoints for external oauth and repos (#544)
  add last updated to fragment proto (#540)
  APP-5851: Add optional visibility field to `CreateFragmentRequest` (#543)
  APP-5314 - add dates and limit to GetRobotPartLogsRequest (#538)
  RSDK-7903 Add unhealthy state and error field (#539)
  RSDK-8381 add viam server version to API (#537)
  [APP-5417]: Undo APP-5417 (#536)
  [APP-5417]: Accommodate pagination for GetRobotPartHistory in api (#535)
  Data 2816/get training job logs (#534)
  DATA-2792 - Add URL to UpdateRegistryItemRequest, but make it optional (#532)
  RSDK-8228 Add config status to machine status API (#531)
  Add revision field to config (#530)
  [APP-5420] Add ListMachineFragments request and response (#525)
  [APP-5400]: Add GetFragmentHistory proto (#524)
  RSDK-7899 Add MachineStatus to RobotService (#527)
  RSDK-8083: add LogPatternConfig field to RobotConfig API (#526)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaellee1019 michaellee1019 michaellee1019 approved these changes

@maximpertsov maximpertsov maximpertsov approved these changes

@piokasar piokasar piokasar approved these changes

Assignees
No one assigned
Labels
protos-compiled safe to test committer is a member of this org
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@puranamp @michaellee1019 @CLAassistant @maximpertsov @piokasar