From da29766239b24823a3d9dd71b02a8a9136de9e6a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 29 Jul 2022 09:21:53 +0000 Subject: [PATCH 1/5] refactor: coderd: extract error messages to variables --- coderd/workspaces.go | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 1469807b8f6c7..7cc318e4c1ae0 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -34,6 +34,17 @@ import ( const workspaceDefaultTTL = 2 * time.Hour +var ( + ttlMin = time.Minute + ttlMax = 7 * 24 * time.Hour + + errTTLMin = xerrors.New("time until shutdown must be at least one minute") + errTTLMax = xerrors.New("time until shutdown must be less than 7 days") + errDeadlineTooSoon = xerrors.New("new deadline must be at least 30 minutes in the future") + errDeadlineBeforeStart = xerrors.New("new deadline must be before workspace start time") + errDeadlineOverTemplateMax = xerrors.New("new deadline is greater than template allows") +) + func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionRead, workspace) { @@ -894,12 +905,12 @@ func validWorkspaceTTLMillis(millis *int64, max time.Duration) (sql.NullInt64, e dur := time.Duration(*millis) * time.Millisecond truncated := dur.Truncate(time.Minute) - if truncated < time.Minute { - return sql.NullInt64{}, xerrors.New("time until shutdown must be at least one minute") + if truncated < ttlMin { + return sql.NullInt64{}, errTTLMin } - if truncated > 24*7*time.Hour { - return sql.NullInt64{}, xerrors.New("time until shutdown must be less than 7 days") + if truncated > ttlMax { + return sql.NullInt64{}, errTTLMax } if truncated > max { @@ -915,17 +926,17 @@ func validWorkspaceTTLMillis(millis *int64, max time.Duration) (sql.NullInt64, e func validWorkspaceDeadline(startedAt, newDeadline time.Time, max time.Duration) error { soon := time.Now().Add(29 * time.Minute) if newDeadline.Before(soon) { - return xerrors.New("new deadline must be at least 30 minutes in the future") + return errDeadlineTooSoon } // No idea how this could happen. if newDeadline.Before(startedAt) { - return xerrors.Errorf("new deadline must be before workspace start time") + return errDeadlineBeforeStart } delta := newDeadline.Sub(startedAt) if delta > max { - return xerrors.New("new deadline is greater than template allows") + return errDeadlineOverTemplateMax } return nil From c46e8f2ed9879771ec7029ecce1f5c2116c2ad58 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 29 Jul 2022 10:25:37 +0000 Subject: [PATCH 2/5] fix: putExtendWorkspace: return validation error in message field --- coderd/workspaces.go | 6 ++++-- coderd/workspaces_test.go | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 7cc318e4c1ae0..bdb8e2dd82b81 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -634,9 +634,11 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { newDeadline := req.Deadline.UTC() if err := validWorkspaceDeadline(job.CompletedAt.Time, newDeadline, time.Duration(template.MaxTtl)); err != nil { + // NOTE(Cian): Putting the error in the Message field on request from the FE folks. + // Normally, we would put the validation error in Validations, but this endpoint is + // not tied to a form or specific named user input on the FE. code = http.StatusBadRequest - resp.Message = "Bad extend workspace request." - resp.Validations = append(resp.Validations, codersdk.ValidationError{Field: "deadline", Detail: err.Error()}) + resp.Message = err.Error() return err } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index eb275e0f8a0bc..c1bbf401fa017 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1063,14 +1063,15 @@ func TestWorkspaceExtend(t *testing.T) { err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ Deadline: deadlineTooSoon, }) - require.ErrorContains(t, err, "new deadline must be at least 30 minutes in the future", "setting a deadline less than 30 minutes in the future should fail") + require.ErrorContains(t, err, "unexpected status code 400: new deadline must be at least 30 minutes in the future", "setting a deadline less than 30 minutes in the future should fail") // And with a deadline greater than the template max_ttl should also fail deadlineExceedsMaxTTL := time.Now().Add(time.Duration(template.MaxTTLMillis) * time.Millisecond).Add(time.Minute) err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ Deadline: deadlineExceedsMaxTTL, }) - require.ErrorContains(t, err, "new deadline is greater than template allows", "setting a deadline greater than that allowed by the template should fail") + + require.ErrorContains(t, err, "unexpected status code 400: new deadline is greater than template allows", "setting a deadline greater than that allowed by the template should fail") // Updating with a deadline 30 minutes in the future should succeed deadlineJustSoonEnough := time.Now().Add(30 * time.Minute) From ac80426efb1d40c11b8e47d8ca7557a940315e52 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 29 Jul 2022 11:46:53 +0100 Subject: [PATCH 3/5] adjust error message --- coderd/workspaces.go | 2 +- coderd/workspaces_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index bdb8e2dd82b81..ae79adf1e48b2 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -638,7 +638,7 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { // Normally, we would put the validation error in Validations, but this endpoint is // not tied to a form or specific named user input on the FE. code = http.StatusBadRequest - resp.Message = err.Error() + resp.Message = "Cannot extend workspace: " + err.Error() return err } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index c1bbf401fa017..a958c260422e6 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1063,7 +1063,7 @@ func TestWorkspaceExtend(t *testing.T) { err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ Deadline: deadlineTooSoon, }) - require.ErrorContains(t, err, "unexpected status code 400: new deadline must be at least 30 minutes in the future", "setting a deadline less than 30 minutes in the future should fail") + require.ErrorContains(t, err, "unexpected status code 400: Cannot extend workspace: new deadline must be at least 30 minutes in the future", "setting a deadline less than 30 minutes in the future should fail") // And with a deadline greater than the template max_ttl should also fail deadlineExceedsMaxTTL := time.Now().Add(time.Duration(template.MaxTTLMillis) * time.Millisecond).Add(time.Minute) @@ -1071,7 +1071,7 @@ func TestWorkspaceExtend(t *testing.T) { Deadline: deadlineExceedsMaxTTL, }) - require.ErrorContains(t, err, "unexpected status code 400: new deadline is greater than template allows", "setting a deadline greater than that allowed by the template should fail") + require.ErrorContains(t, err, "unexpected status code 400: Cannot extend workspace: new deadline is greater than template allows", "setting a deadline greater than that allowed by the template should fail") // Updating with a deadline 30 minutes in the future should succeed deadlineJustSoonEnough := time.Now().Add(30 * time.Minute) From f0019e6fdf36b612808cee2cef1748f898c9f0b0 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 29 Jul 2022 12:16:45 +0100 Subject: [PATCH 4/5] nolint --- coderd/workspaces.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index ae79adf1e48b2..027baa31b3063 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -35,7 +35,7 @@ import ( const workspaceDefaultTTL = 2 * time.Hour var ( - ttlMin = time.Minute + ttlMin = time.Minute //nolint:time-naming // min here means 'minimum' not 'minutes' ttlMax = 7 * 24 * time.Hour errTTLMin = xerrors.New("time until shutdown must be at least one minute") From 2725cd6b302210b2da8baafa759d8150dd9d6e48 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 29 Jul 2022 12:19:44 +0100 Subject: [PATCH 5/5] fixup! nolint --- coderd/workspaces.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 027baa31b3063..8101772232e1e 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -35,7 +35,7 @@ import ( const workspaceDefaultTTL = 2 * time.Hour var ( - ttlMin = time.Minute //nolint:time-naming // min here means 'minimum' not 'minutes' + ttlMin = time.Minute //nolint:revive // min here means 'minimum' not 'minutes' ttlMax = 7 * 24 * time.Hour errTTLMin = xerrors.New("time until shutdown must be at least one minute")