-
Notifications
You must be signed in to change notification settings - Fork 180
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
feat(robot-server,api): Add the skeleton for a new complete-recovery
run action
#14674
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14674 +/- ##
=======================================
Coverage 67.33% 67.33%
=======================================
Files 2485 2485
Lines 71389 71389
Branches 9032 9032
=======================================
Hits 48071 48071
Misses 21173 21173
Partials 2145 2145
Flags with carried forward coverage won't be shown. Click here to find out more.
|
0c35a8d
to
3b805c9
Compare
complete-recovery
run actioncomplete-recovery
run action
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.
modulo the correction on the state hnadling that Derek pointed out, looks good. I'm interested in your reasoning about two things:
- why call this
complete-recovery
as opposed to someresume
thing? To me it carries the implication that the engine will be doing something to complete the recover, as opposed to relying on the client to have done something and then naively resuming - in fact now that i'm looking at it, why not have this be
PlayAction
? That moves us towards a system where we have fewer, more common keywords that do different things based on state - a state machine where logic is biased to "what state am I in".
Like I said, it looks good to merge either way, but I'm interested in hearing your thoughts.
No reasoning.
A couple of loose reasons:
|
Overview
Closes EXEC-300.
Changelog
POST /runs/{id}/actions
:complete-recovery
.ProtocolEngine
and update unit tests along the way.ProtocolEngine
raise aNotImplementedError
whenever it encounters it. The actual implementation will be done in future PRs.Test Plan
I've tested with Postman that I can post a
complete-recovery
action to a run, and it raises the expectedNotImplementedError
.Review requests
Do we like the name
complete-recovery
for this?Risk assessment
Low.