-
Notifications
You must be signed in to change notification settings - Fork 72
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
RSDK-6058 Deprecate sensors service api #424
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.
LGTM. It's exciting to get rid of this!
} | ||
|
||
message GetSensorsResponse { | ||
repeated common.v1.ResourceName sensor_names = 1; |
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 that in addition to giving RPCs the deprecated option, you can do that to entire messages, too. Worth doing here? I dunno; what you've got is already pretty thorough.
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.
nice! I am not a proto expert, had to google what to actually do here. I don't see the harm in just adding deprecated signifiers everywhere we can.
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's:
message GetSensorsResponse {
option deprecated = true;
repeated common.v1.ResourceName sensor_names = 1 [deprecated = true];
...
}
but I've never done this myself and could be wrong.
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.
Okay I'm satisfied. I will check with a local version of this API with rdk tomorrow to make sure things work as expected and won't break anything juts by adding a deprecated signifier to proto.
Finally taste with a local build of the API following instructions here, in case anyone was wondering about the method I used to test. Tested with the python SDK using the following code: service = SensorsClient.from_robot(robot, "builtin")
sensors = await service.get_sensors()
print(f"b get_readings return value: {sensors}")
vals = await service.get_readings(sensors)
print(f"b get_readings return value: {vals}") Got back:
RC Card is still functional (three fake sensors configured): I'm happy, will merge in. |
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.
Rand thinks the PR is stuck in some weird CI edge case, and needs another LGTM to get unstuck...
This pull request marks the sensors service API as deprecated. This will be removed once the RC card for sensors no longer has a dependency on the GetAllReadings Function.