refactor(robot-server): Use last run command for /runs/:runId/currentState links
#16557
Conversation
mjhuff
force-pushed
the
robot-server_update-curren-state-links
branch
4 times, most recently
from
363018f
to
d2778af
Compare
| @@ -132,9 +132,9 @@ class AllRunsLinks(BaseModel): | |||
| class CurrentStateLinks(BaseModel): | |||
| """Links returned with the current state of a run.""" | |||
|
|
|||
| current: Optional[CommandLinkNoMeta] = Field( | |||
| last: Optional[CommandLinkNoMeta] = Field( | |||
There was a problem hiding this comment.
I would consider naming this lastCompleted instead of just last, in order to avoid confusion with the last queued command, which hasn't necessarily executed yet, and so is a different thing.
| command_id=command.id, | ||
| command_key=command.key, | ||
| created_at=command.createdAt, | ||
| index=command_slice.total_length - 1, |
There was a problem hiding this comment.
It should be equivalent, but you might find it clearer to do index=command_slice.cursor instead of index=command_slice.total_length - 1.
There was a problem hiding this comment.
Ahhh yep, that is cleaner, thanks
| @@ -554,3 +564,22 @@ def _get_run_time_parameters(self, run_id: str) -> List[RunTimeParameter]: | |||
| return self._run_orchestrator_store.get_run_time_parameters() | |||
| else: | |||
| return self._run_store.get_run_time_parameters(run_id=run_id) | |||
|
|
|||
| def _get_historical_run_last_command(self, run_id: str) -> Optional[CommandPointer]: | |||
| command_slice = self._run_store.get_commands_slice( | |||
There was a problem hiding this comment.
Heads up that for older historical JSON runs, which enqueued all of their commands up-front instead of one-by-one as the run proceeded, this will return the last command in the whole protocol, not the last command that actually ran.
I think this is close enough that we shouldn't worry about it.
mjhuff
force-pushed
the
robot-server_update-curren-state-links
branch
2 times, most recently
from
7f53850
to
db5dda4
Compare
Overview
In #16417, it was noted that
linksshould include a reference to the last completed command as opposed to the "current" command in order to prevent torn state. This PR updates the link to do just that.Test Plan and Hands on Testing
Changelog
linksproperty within theruns/runId/currentStateresponse.Review requests
c8dce60 - Updates the behavior for
get_current_commandfor historical runs with the behavior that I think we want given the comment, but is this actually the case? CC: @SyntaxColoringRisk assessment
low