feat: Allow user to cancel workspace jobs #5115
Conversation
There was a problem hiding this comment.
The code looks good. Just a few things:
- I think you could use the field helper
{...getFieldHelpers("name")}if it makes sense and works just to keep consistency - Would be nice to add this field to the form schema in line
47
I just commented on a few things but it is up to you.
|
Thank you for the review, Bruno.
Checkbox doesn't have property For reference:
Good catch! Fixed. |
coderd/database/migrations/000081_template_allow_user_cancel_workspace_jobs.up.sql
Show resolved
Hide resolved
| @@ -599,6 +604,21 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques | |||
| return | |||
| } | |||
|
|
|||
| valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, httpmw.APIKey(r).UserID, workspace.TemplateID) | |||
| if err != nil { | |||
| httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | |||
There was a problem hiding this comment.
Suggestion: Both errors returned by verify seem like known states and caused by a user doing a nonsense request (vs. server making a mistake), maybe unauthorized would be a good response?
There was a problem hiding this comment.
Unauthorized might be misleading while debugging potential customer issues. In theory, we don't expect an invalid user or an invalid template, and it doesn't look possible that the user passes it, hence an internal error.
There was a problem hiding this comment.
Good points. After a second look I agree, didn't look closely enough at the inputs at first. 👍🏻
codersdk/templates.go
Outdated
| Description string `json:"description,omitempty"` | ||
| Icon string `json:"icon,omitempty"` | ||
| DefaultTTLMillis int64 `json:"default_ttl_ms,omitempty"` | ||
| AllowUserCancelWorkspaceJobs bool `json:"allow_user_cancel_workspace_jobs"` |
There was a problem hiding this comment.
Should this be *bool as well so the value doesn’t need to be provided every request?
There was a problem hiding this comment.
Frankly speaking, I would leave it as is and fix it as part of #5066. This is the outcome of API design. If I fix it here, it will be confusing for customers (optional params vs "").
Fixes: #5035
This PR adds a condition to
workspacebuildsto allow/block cancel requests to workspace jobs. If the flagallow_user_cancel_workspace_jobsis disabled, only users with the "owner" role can cancel jobs.Detailed changes:
allow_user_cancel_workspace_jobs(default: true)workspacebuildsScreenshots: