-
Notifications
You must be signed in to change notification settings - Fork 55
feat(ws): add workspace pause actions backend API #340
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(ws): add workspace pause actions backend API #340
Conversation
/ok-to-test |
fb2e2fb
to
f22b3d3
Compare
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.
8d5e467
to
3237336
Compare
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.
Some questions inline.
3237336
to
ba9e1bc
Compare
ba9e1bc
to
575e1a8
Compare
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.
@harshad16: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 left a few comments (we were on a call during the review so @andyatmiami should be aware of what to do and we were in agreement on the changes).
575e1a8
to
69ff24a
Compare
69ff24a
to
41f8118
Compare
related: kubeflow#298 - Added PauseActionWorkspaceHandler to handle pausing or unpausing a given workspace - Introduced single new route for starting and pausing workspaces in the API. - `api/v1/workspaces/{namespace}/{name}/actions/pause` - pausing or unpausing operation is specified in the request payload - Created a new WorkspaceActionPauseEnvelope type for successful responses. - Leveraging JSONPatch / client.RawPatch to ensure Workspace in "valid state" before attempting action - for `start`: `spec.paused` must be `true`, and `status.state` must be `Paused` - for `pause`: `spec.paused` must be `false` - note: I would love to have a `status.state` check here of `status.state != Paused`, but that type of comparison is not supported in [JSONPatch](https://datatracker.ietf.org/doc/html/rfc6902#section-4.6) - Added tests for the new API, including success and error cases. - Updated README/OpenAPI documentation to include the new endpoints. --- As an interesting "edge case" worth calling out, the following payload is currently honored by the API: ``` { "data": {} } ``` Given the `WorkspaceActionPause` struct is simply `{"paused": true|false}`, the "empty" Envelope presented above deserializes the JSON using the zero value of `bool` (which is `false`). Our validation today is always performed against the **deserialized** object, and as such impossible to distinguish the following cases: ``` { "data": {} } ``` vs ``` { "data": { "paused": false } } ``` The effort and (relative) complexity to prevent this and return a `422` in this scenario was not deemed "worth it" for the time being. As a result, a test case has been added for this specific scenario to at minimum document this "strange" behavior. - Clients, however, should **NOT** rely on this behavior and always provide a fully defined `WorkspaceActionPause` JSON object to ensure future compatibility. Signed-off-by: Andy Stoneberg <[email protected]>
41f8118
to
e0592d4
Compare
Verifying behaviorCheck existing state
Pause running
Verify
Start (unpause) paused
Verify
Attempt to start (unpause) running
Attempt to start (unpause) running
Verify state
|
@andyatmiami thanks, it took a while but we got there. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thesuperzapper The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
related: #298
api/v1/workspaces/{namespace}/{name}/actions/pause
- pausing or unpausing operation is specified in the request payload
start
:spec.paused
must betrue
, andstatus.state
must bePaused
pause
:spec.paused
must befalse
status.state
check here ofstatus.state != Paused
, but that type of comparison is not supported in JSONPatchAs an interesting "edge case" worth calling out, the following payload is currently honored by the API:
Given the
WorkspaceActionPause
struct is simply{"paused": true|false}
, the "empty" Envelope presented above deserializes the JSON using the zero value ofbool
(which isfalse
).Our validation today is always performed against the deserialized object, and as such impossible to distinguish the following cases w.r.t actual JSON request payload:
vs
The effort and (relative) complexity to prevent this and return a
422
in this scenario was not deemed "worth it" for the time being. As a result, a test case has been added for this specific scenario to at minimum document this "strange" behavior.WorkspaceActionPause
JSON object to ensure future compatibility.Rendered Swagger doc: