feat(ws): Implement DELETE endpoint for WorkspaceKind #480
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
andyatmiami
force-pushed
the
feat/workspacekind-delete
branch
2 times, most recently
from
3613aaf to
4d9e835
Compare
|
@paulovmr we'll need a frontend counterpart for this new endpoint. |
- Added the ability to delete a specific WorkspaceKind via a new DELETE API endpoint.
- `DELETE /workspacekinds/{name}`
- Updated the README to include the new endpoint details and added corresponding tests to ensure proper functionality and error handling for various scenarios, including validation and authorization checks.
- Swagger files regenerated
The implementation is more or less a "carbon copy" of how it is implemented for Workspaces _with the exception_ of the need to handle the `Conflict` that can be returned by the admission webhook. While there is basic handling of this condition in the PR - I'm not necessarily happy with it.
First off, there is an error message prefix inserted by kubernetes itself:
```
curl -X 'DELETE' 'http://localhost:4001/api/v1/workspacekinds/jupyterlab' -H 'accept: application/json'
{
"error": {
"code": "409",
"message": "admission webhook \"vworkspacekind.kb.io\" denied the request: Operation cannot be fulfilled on WorkspaceKind.kubeflow.org \"jupyterlab\": WorkspaceKind is used by 2 workspace(s)"
}
}
```
The `admission webhook \"vworkspacekind.kb.io\" denied the request:` is appended on the error message returned from `workspacekind_webhook.go` and not easily able to be omitted (without falling back to brittle string parsing). I personally don't think that is appropriate details to put in an error message that could be surfaced in the UI. And while a `Conflict` error today is the only surfaced from the workspacekind webhook code if a workspace is using it - I'm not confident that condition would hold in the future... and there is no other information I can reliably act on to distinguish this particular cause (to simply provide a more succinct error message in `repo.go`)
Furthermore, our testing framework does not currently run with webhooks - so I have deliberately **NOT** added a test case for this behavior.
Signed-off-by: Andy Stoneberg <astonebe@redhat.com>
andyatmiami
force-pushed
the
feat/workspacekind-delete
branch
from
4d9e835 to
1123a45
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. Members may comment |
|
/lifecycle frozen |
|
@andyatmiami: The 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. |
2 participants
DELETE /workspacekinds/{name}The implementation is more or less a "carbon copy" of how it is implemented for Workspaces with the exception of the need to handle the
Conflictthat can be returned by the admission webhook. While there is basic handling of this condition in the PR - I'm not necessarily happy with it.First off, there is an error message prefix inserted by kubernetes itself:
The
admission webhook \"vworkspacekind.kb.io\" denied the request:is appended on the error message returned fromworkspacekind_webhook.goand not easily able to be omitted (without falling back to brittle string parsing). I personally don't think that is appropriate details to put in an error message that could be surfaced in the UI. And while aConflicterror today is the only surfaced from the workspacekind webhook code if a workspace is using it - I'm not confident that condition would hold in the future... and there is no other information I can reliably act on to distinguish this particular cause (to simply provide a more succinct error message inrepo.go)Furthermore, our testing framework does not currently run with webhooks - so I have deliberately NOT added a test case for this behavior.