Skip to content

Commit 5db9d71

Browse files
kacpersawkacpersaw
committed
Apply review suggestions
1 parent 42170ab commit 5db9d71

File tree

6 files changed

+31
-26
lines changed

6 files changed

+31
-26
lines changed

‎coderd/workspacebuilds.go

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -598,8 +598,8 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
598598

599599
code := http.StatusOK
600600
resp := codersdk.Response{}
601-
err = api.Database.InTx(func(store database.Store) error {
602-
valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID)
601+
err = api.Database.InTx(func(db database.Store) error {
602+
valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, db, httpmw.APIKey(r).UserID, workspace.TemplateID, expectStatus)
603603
if err != nil {
604604
code = http.StatusInternalServerError
605605
resp.Message = "Internal error verifying permission to cancel workspace build."
@@ -611,10 +611,10 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
611611
code = http.StatusForbidden
612612
resp.Message = "User is not allowed to cancel workspace builds. Owner role is required."
613613

614-
return xerrors.Errorf("user is not allowed to cancel workspace builds")
614+
return xerrors.New("user is not allowed to cancel workspace builds")
615615
}
616616

617-
job, err := store.GetProvisionerJobByID(ctx, workspaceBuild.JobID)
617+
job, err := db.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID)
618618
if err != nil {
619619
code = http.StatusInternalServerError
620620
resp.Message = "Internal error fetching provisioner job."
@@ -626,32 +626,32 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
626626
code = http.StatusBadRequest
627627
resp.Message = "Job has already completed!"
628628

629-
return xerrors.Errorf("job has already completed")
629+
return xerrors.New("job has already completed")
630630
}
631631
if job.CanceledAt.Valid {
632632
code = http.StatusBadRequest
633633
resp.Message = "Job has already been marked as canceled!"
634634

635-
return xerrors.Errorf("job has already been marked as canceled")
635+
return xerrors.New("job has already been marked as canceled")
636636
}
637637

638638
if expectStatus != "" {
639639
if expectStatus != "running" && expectStatus != "pending" {
640640
code = http.StatusBadRequest
641-
resp.Message = "Invalid expect_status. Only 'running' or 'pending' are allowed."
641+
resp.Message = fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.", expectStatus)
642642

643-
return xerrors.Errorf("invalid expect_status")
643+
return xerrors.Errorf("invalid expect_status %q", expectStatus)
644644
}
645645

646646
if job.JobStatus != database.ProvisionerJobStatus(expectStatus) {
647647
code = http.StatusPreconditionFailed
648648
resp.Message = "Job is not in the expected state."
649649

650-
return xerrors.Errorf("job is not in the expected state")
650+
return xerrors.Errorf("job is not in the expected state: expected: %q, got %q", expectStatus, job.JobStatus)
651651
}
652652
}
653653

654-
err = store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
654+
err = db.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
655655
ID: job.ID,
656656
CanceledAt: sql.NullTime{
657657
Time: dbtime.Now(),
@@ -688,7 +688,12 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
688688
})
689689
}
690690

691-
func 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, expectStatus string) (bool, error) {
692+
// If the expectStatus is pending, we can cancel it.
693+
if expectStatus == "pending" {
694+
return true, nil
695+
}
696+
692697
template, err := store.GetTemplateByID(ctx, templateID)
693698
if err != nil {
694699
return false, xerrors.New("no template exists for this workspace")

‎coderd/workspacebuilds_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
573573
build, err = client.WorkspaceBuild(ctx, workspace.LatestBuild.ID)
574574
return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning
575575
}, testutil.WaitShort, testutil.IntervalFast)
576-
err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{})
576+
err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{})
577577
require.NoError(t, err)
578578
require.Eventually(t, func() bool {
579579
var err error
@@ -618,7 +618,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
618618
build, err = userClient.WorkspaceBuild(ctx, workspace.LatestBuild.ID)
619619
return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning
620620
}, testutil.WaitShort, testutil.IntervalFast)
621-
err := userClient.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{})
621+
err := userClient.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{})
622622
var apiErr *codersdk.Error
623623
require.ErrorAs(t, err, &apiErr)
624624
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
@@ -671,7 +671,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
671671
}
672672

673673
// When: the workspace build is canceled
674-
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{
674+
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{
675675
ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending,
676676
})
677677
require.NoError(t, err)
@@ -711,7 +711,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
711711
}, testutil.WaitShort, testutil.IntervalFast)
712712

713713
// When: a cancel request is made with expect_state=pending
714-
err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{
714+
err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{
715715
ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending,
716716
})
717717
// Then: the request should fail with 412.
@@ -744,7 +744,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
744744
ctx := testutil.Context(t, testutil.WaitLong)
745745

746746
// When: a cancel request is made with invalid expect_state
747-
err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildRequest{
747+
err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildParams{
748748
ExpectStatus: "invalid_status",
749749
})
750750
// Then: the request should fail with 400.
@@ -1098,7 +1098,7 @@ func TestWorkspaceBuildStatus(t *testing.T) {
10981098
_ = closeDaemon.Close()
10991099
// after successful cancel is "canceled"
11001100
build = coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStart)
1101-
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{})
1101+
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{})
11021102
require.NoError(t, err)
11031103

11041104
workspace, err = client.Workspace(ctx, workspace.ID)

‎coderd/workspaces_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3245,7 +3245,7 @@ func TestWorkspaceWatcher(t *testing.T) {
32453245
closeFunc.Close()
32463246
build := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStart)
32473247
wait("first is for the workspace build itself", nil)
3248-
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{})
3248+
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{})
32493249
require.NoError(t, err)
32503250
wait("second is for the build cancel", nil)
32513251
}

‎codersdk/toolsdk/toolsdk_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ func TestTools(t *testing.T) {
164164

165165
// Important: cancel the build. We don't run any provisioners, so this
166166
// will remain in the 'pending' state indefinitely.
167-
require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildRequest{}))
167+
require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildParams{}))
168168
})
169169

170170
t.Run("Start", func(t *testing.T) {
@@ -184,7 +184,7 @@ func TestTools(t *testing.T) {
184184

185185
// Important: cancel the build. We don't run any provisioners, so this
186186
// will remain in the 'pending' state indefinitely.
187-
require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildRequest{}))
187+
require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildParams{}))
188188
})
189189

190190
t.Run("TemplateVersionChange", func(t *testing.T) {
@@ -216,7 +216,7 @@ func TestTools(t *testing.T) {
216216
require.Equal(t, r.Workspace.ID.String(), updateBuild.WorkspaceID.String())
217217
require.Equal(t, newVersion.TemplateVersion.ID.String(), updateBuild.TemplateVersionID.String())
218218
// Cancel the build so it doesn't remain in the 'pending' state indefinitely.
219-
require.NoError(t, client.CancelWorkspaceBuild(ctx, updateBuild.ID, codersdk.CancelWorkspaceBuildRequest{}))
219+
require.NoError(t, client.CancelWorkspaceBuild(ctx, updateBuild.ID, codersdk.CancelWorkspaceBuildParams{}))
220220

221221
// Roll back to the original version
222222
rollbackBuild, err := testTool(t, toolsdk.CreateWorkspaceBuild, tb, toolsdk.CreateWorkspaceBuildArgs{
@@ -229,7 +229,7 @@ func TestTools(t *testing.T) {
229229
require.Equal(t, r.Workspace.ID.String(), rollbackBuild.WorkspaceID.String())
230230
require.Equal(t, originalVersionID.String(), rollbackBuild.TemplateVersionID.String())
231231
// Cancel the build so it doesn't remain in the 'pending' state indefinitely.
232-
require.NoError(t, client.CancelWorkspaceBuild(ctx, rollbackBuild.ID, codersdk.CancelWorkspaceBuildRequest{}))
232+
require.NoError(t, client.CancelWorkspaceBuild(ctx, rollbackBuild.ID, codersdk.CancelWorkspaceBuildParams{}))
233233
})
234234
})
235235

‎codersdk/workspacebuilds.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,11 +130,11 @@ const (
130130
CancelWorkspaceBuildStatusPending CancelWorkspaceBuildStatus = "pending"
131131
)
132132

133-
type CancelWorkspaceBuildRequest struct {
133+
type CancelWorkspaceBuildParams struct {
134134
ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"`
135135
}
136136

137-
func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption {
137+
func (c *CancelWorkspaceBuildParams) asRequestOption() RequestOption {
138138
return func(r *http.Request) {
139139
q := r.URL.Query()
140140
q.Set("expect_status", string(c.ExpectStatus))
@@ -143,7 +143,7 @@ func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption {
143143
}
144144

145145
// CancelWorkspaceBuild marks a workspace build job as canceled.
146-
func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID, req CancelWorkspaceBuildRequest) error {
146+
func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID, req CancelWorkspaceBuildParams) error {
147147
res, err := c.Request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/workspacebuilds/%s/cancel", id), nil, req.asRequestOption())
148148
if err != nil {
149149
return err

‎scaletest/workspacebuild/run.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ func (r *CleanupRunner) Run(ctx context.Context, _ string, logs io.Writer) error
150150
if err == nil && build.Job.Status.Active() {
151151
// mark the build as canceled
152152
logger.Info(ctx, "canceling workspace build", slog.F("build_id", build.ID), slog.F("workspace_id", r.workspaceID))
153-
if err = r.client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{}); err == nil {
153+
if err = r.client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}); err == nil {
154154
// Wait for the job to cancel before we delete it
155155
_ = waitForBuild(ctx, logs, r.client, build.ID) // it will return a "build canceled" error
156156
} else {

0 commit comments

Comments
 (0)