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.

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

feature(api, robot-server): add get command errors to runs router #15883

Merged
merged 12 commits into from
Aug 5, 2024

Conversation

TamarZanzouri
Copy link
Contributor

@TamarZanzouri TamarZanzouri commented Aug 2, 2024

Overview

closes https://opentrons.atlassian.net/browse/EXEC-504.
add an end point in runs router to fetch commands error list.

Test Plan and Hands on Testing

  • Post a protocol that will cause a recovery workflow.
  • Start the run and let the command fail.
  • Resume from recovery.
  • Let the run end.
  • GET /runs/{runId}/commandErrors and make sure you are getting the full command error list.

Changelog

  • added an end point to commands router /runs/{runId}/commandErrors.
  • added logic in CommandsState to get_error_slice.

Review requests

naming makes sense? changes make sense? location for the new route makes sense? (under commands router)

Risk assessment

low. adding new logic

@TamarZanzouri TamarZanzouri changed the title Exec 504 get error list route feature(api, robot-server): add get command errors to commands router Aug 2, 2024
@TamarZanzouri TamarZanzouri marked this pull request as ready for review August 2, 2024 20:31
@TamarZanzouri TamarZanzouri requested a review from a team as a code owner August 2, 2024 20:31
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

I think we should change the route from /commands/errors to /commandErrors, but other than that, this looks good to me. Thank you!

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Some implementation notes, but mostly request-changes because of the route - I agree with @SyntaxColoring, I think this should be a child of /runs/:runid directly. I support /runs/:runid/errors rather than /runs/:runid/commandErrors just because it's shorter and I hate camelcase but obviously that's a matter of taste more than anything else.

@TamarZanzouri TamarZanzouri changed the title feature(api, robot-server): add get command errors to commands router feature(api, robot-server): add get command errors to runs router Aug 5, 2024
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Much nicer, love the new API presentation. I think we probably still want to hoist the cursor defaulting to the robot server but other than that I'm happy with it.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

I think the return type needs adjusting, but other than that this looks great, thanks.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@TamarZanzouri TamarZanzouri merged commit ee65689 into chore_release-8.0.0 Aug 5, 2024
21 checks passed
@TamarZanzouri TamarZanzouri deleted the EXEC-504-get-error-list-route branch August 5, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants