Skip to content

Commit 86a34df

Browse files
kacpersawkacpersaw
committed
Apply review suggestions
1 parent b672d76 commit 86a34df

File tree

2 files changed

+41
-43
lines changed

2 files changed

+41
-43
lines changed

‎coderd/workspacebuilds.go

Lines changed: 40 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -596,56 +596,58 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
596596
return
597597
}
598598

599+
code := http.StatusOK
600+
resp := codersdk.Response{}
599601
err = api.Database.InTx(func(store database.Store) error {
600-
valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID)
602+
valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID)
601603
if err != nil {
602-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
603-
Message: "Internal error verifying permission to cancel workspace build.",
604-
Detail: err.Error(),
605-
})
606-
return nil
604+
code = http.StatusInternalServerError
605+
resp.Message = "Internal error verifying permission to cancel workspace build."
606+
resp.Detail = err.Error()
607+
608+
return xerrors.Errorf("verify user can cancel workspace builds: %w", err)
607609
}
608610
if !valid {
609-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
610-
Message: "User is not allowed to cancel workspace builds. Owner role is required.",
611-
})
612-
return nil
611+
code = http.StatusForbidden
612+
resp.Message = "User is not allowed to cancel workspace builds. Owner role is required."
613+
614+
return xerrors.Errorf("user is not allowed to cancel workspace builds")
613615
}
614616

615617
job, err := store.GetProvisionerJobByID(ctx, workspaceBuild.JobID)
616618
if err != nil {
617-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
618-
Message: "Internal error fetching provisioner job.",
619-
Detail: err.Error(),
620-
})
621-
return nil
619+
code = http.StatusInternalServerError
620+
resp.Message = "Internal error fetching provisioner job."
621+
resp.Detail = err.Error()
622+
623+
return xerrors.Errorf("get provisioner job: %w", err)
622624
}
623625
if job.CompletedAt.Valid {
624-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
625-
Message: "Job has already completed!",
626-
})
627-
return nil
626+
code = http.StatusBadRequest
627+
resp.Message = "Job has already completed!"
628+
629+
return xerrors.Errorf("job has already completed")
628630
}
629631
if job.CanceledAt.Valid {
630-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
631-
Message: "Job has already been marked as canceled!",
632-
})
633-
return nil
632+
code = http.StatusBadRequest
633+
resp.Message = "Job has already been marked as canceled!"
634+
635+
return xerrors.Errorf("job has already been marked as canceled")
634636
}
635637

636638
if expectStatus != "" {
637639
if expectStatus != "running" && expectStatus != "pending" {
638-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
639-
Message: "Invalid expect_status. Only 'running' or 'pending' are allowed.",
640-
})
641-
return nil
640+
code = http.StatusBadRequest
641+
resp.Message = "Invalid expect_status. Only 'running' or 'pending' are allowed."
642+
643+
return xerrors.Errorf("invalid expect_status")
642644
}
643645

644646
if job.JobStatus != database.ProvisionerJobStatus(expectStatus) {
645-
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
646-
Message: "Job is not in the expected state.",
647-
})
648-
return nil
647+
code = http.StatusPreconditionFailed
648+
resp.Message = "Job is not in the expected state."
649+
650+
return xerrors.Errorf("job is not in the expected state")
649651
}
650652
}
651653

@@ -662,20 +664,17 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
662664
},
663665
})
664666
if err != nil {
665-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
666-
Message: "Internal error updating provisioner job.",
667-
Detail: err.Error(),
668-
})
669-
return nil
667+
code = http.StatusInternalServerError
668+
resp.Message = "Internal error updating provisioner job."
669+
resp.Detail = err.Error()
670+
671+
return xerrors.Errorf("update provisioner job: %w", err)
670672
}
671673

672674
return nil
673675
}, nil)
674676
if err != nil {
675-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
676-
Message: "Internal error updating provisioner job.",
677-
Detail: err.Error(),
678-
})
677+
httpapi.Write(ctx, rw, code, resp)
679678
return
680679
}
681680

@@ -689,7 +688,7 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
689688
})
690689
}
691690

692-
func (*API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
691+
func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
693692
template, err := store.GetTemplateByID(ctx, templateID)
694693
if err != nil {
695694
return false, xerrors.New("no template exists for this workspace")

‎coderd/workspacebuilds_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -763,8 +763,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
763763
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
764764
workspace := coderdtest.CreateWorkspace(t, client, template.ID)
765765

766-
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
767-
defer cancel()
766+
ctx := testutil.Context(t, testutil.WaitLong)
768767

769768
// When: a cancel request is made with invalid expect_state
770769
err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildRequest{

0 commit comments

Comments
 (0)