-
Notifications
You must be signed in to change notification settings - Fork 45
[RSDK-8045] [RSDK-8366] Simultaneous Auth for App and Machine #326
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
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.
It took me a while to look through, but the changes look really good to me! I enjoyed seeing the improved tests.
@@ -571,7 +592,7 @@ export class AppClient { | |||
pb.LocationAuthRequest, | |||
pb.LocationAuthResponse | |||
>(service.locationAuth.bind(service), req); | |||
return response.toObject(); | |||
return response.toObject().auth; |
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.
Following the previous functions, this return should be return response.getAuth()?.toObject()
. The following functions after this one, however, follow this pattern of response.toObject()._
. Wanted to point it out in case we had a preference.
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 had changed some of them to be response.getFoo().toObject()
and kept some as response.toObject().foo
because I had a slight inkling that converting the entire message to a JS object would be slower than getting only the attribute we want and then calling toObject()
on that. However, so many of these responses only had a single attribute that I figured it probably didn't actually make much of a difference.
I perhaps should've done some benchmarking instead of making assumptions...
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.
lgtm
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.
Oh whoops, meant to approve with my comment.
This PR enables getting a machine client from an already-authenticated Viam client.
It also introduces a few breaking changes, all of the same type: in the
AppClient
, there were many instances of methods returningpb.Response.AsObject
rather than the actual type underneath the response, e.g.pb.Robot.AsObject
:This PR also goes through and explicitly adds return types for app client so that we get compile-time errors in future.
Once this is approved, I will make an announcement regarding this breaking change. Specifically the functions that are affected are:
getOrganization
createOrganizationInvite
updateOrganizationInviteAuthorizations
resendOrganizationInvite
createLocation
getLocation
updateLocation
locationAuth
createLocationSecret
getRobot
updateRobotPart
createRobotPartSecret
updateRobot
getFragment
createFragment
updateFragment
getRegistryItem
getModule