fix: allow setting workspace deadline as early as now plus 30 minutes by johnstcn · Pull Request #2328 · coder/coder

fix: allow setting workspace deadline as early as now plus 30 minutes by johnstcn · Pull Request #2328 · coder/coder

This PR makes the following changes:

coderd: /api/v2/workspaces/:workspace/extend now accepts any time at least 30 minutes in the future
coder bump command also allows the above. Some small copy changes to command.

UI changes to come later.

Comment on lines -886 to -897

		now := time.Now()
		if new.Before(now) {
		return xerrors.New("new deadline must be in the future")
		}

		delta := new.Sub(old)
		if delta < time.Minute {
		return xerrors.New("minimum extension is one minute")
		}

		if delta > 24*time.Hour {
		return xerrors.New("maximum extension is 24 hours")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: Dropping these validations for the moment till we have more information from #2318

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there's one very important validation involving templates that I may have overlooked! Adding this in now.

Comment on lines +1020 to +1025

		// 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")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: I forgot to add this in earlier. We don't folks folks sneaking around template-level restrictions this way.

		return xerrors.New("minimum extension is one minute")
		// No idea how this could happen.
		if newDeadline.Before(startedAt) {
		return xerrors.Errorf("new deadline must be before workspace start time")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figure if we get here, time is broken.

Comment on lines +611 to +615

		if build.Deadline.IsZero() {
		code = http.StatusConflict
		resp.Message = "Workspace shutdown is manual."
		return xerrors.Errorf("workspace shutdown is manual")
		}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: I'm not allowing this yet as if someone does, then there's currently no way to un-set this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for "bump" doesn't make sense anymore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: this won't be called coder bump any more.

johnstcn deleted the cj/2224/deadline-now-plus-30m branch

June 14, 2022 21:39