feat: update workspace deadline when workspace ttl updated #2165

johnstcn · 2022-06-08T14:12:35Z

This PR adds the following changes:

CLI: updating a workspace TTL updates the deadline of the workspace.
- If the TTL is being un-set, the workspace deadline is set to zero.
- If the TTL is being set, the workspace deadline is updated to be the last updated time of the workspace build plus the requested TTL. Additionally, the user is prompted to confirm interactively (can be bypassed with -y).
UI: updating the workspace schedule behaves similarly to the CLI, showing a message to the user if the updated TTL/time to shutdown would effect changes to the lifetime of the running workspace.

…e TTL

…space-ttl-updates-deadline

johnstcn · 2022-06-09T10:51:13Z

cli/ttl.go

+				_, err = cliui.Prompt(cmd, cliui.PromptOptions{
+					Text: fmt.Sprintf(
+						"Workspace %q will be stopped %s. Are you sure?",
+						workspace.Name,
+						humanRemaining,
+					),
+					Default:   "yes",
+					IsConfirm: true,


review: we can skip the prompt if timeRemaining >= SOME_CUTOFF.

johnstcn · 2022-06-09T10:52:38Z

coderd/workspaces_test.go

+			if testCase.expectedDeadline != nil {
+				require.WithinDuration(t, *testCase.expectedDeadline, updated.LatestBuild.Deadline, time.Minute, "expected autostop deadline to be equal expected")
+			}


review: I'm fudging a bit here out of necessity as the provision step can take a while with a test database.

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

johnstcn · 2022-06-09T10:54:06Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

 }

 export interface WorkspaceScheduleFormProps {
  fieldErrors?: FieldErrors
  initialValues?: WorkspaceScheduleFormValues
  isLoading: boolean
+  now: dayjs.Dayjs


review: I'm not sure if there's a better way to do it, but I figured it's easiest to just pass in the current time as a prop.

Comment(approving): I'm OK with this, it seems like we either don't do this and wrestle with mocks or spies in tests and storybook...or we just do this and live with it being a little weird.

Suggestion(nit): Just let's make it optional:

Suggested change

now: dayjs.Dayjs

now?: dayjs.Dayjs

mafredri

Clean change! I had one question but otherwise LGTM (backend)

cli/ttl.go

coderd/workspaces.go

mafredri · 2022-06-09T12:45:57Z

coderd/workspaces.go

+
+		if latestBuild.UpdatedAt.IsZero() {
+			// Build in progress; provisionerd should update with the new TTL.
+			return nil


Question: What happens if a user starts a workspace with one TTL, but tries to update the TTL before the build is complete? As a user, I'd expect the new TTL to be in-effect once the build has completed or receive an error that TTL cannot be updated (yet).

See

coder/coderd/provisionerdaemons.go

Line 598 in 567e4af

workspaceDeadline = now.Add(time.Duration(workspace.Ttl.Int64))

-- we fetch the workspace TTL in a transaction and update the deadline upon finishing the provisioner job.

If the user updates the TTL first, then the updated TTL will be used by provisionerd.

If provisionerd updates the TTL first, then the deadline should be updated.

Ooh yeah that makes sense, thanks for clarifying!

Kira-Pilot · 2022-06-09T12:59:50Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

+    return `⚠️ ${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownSoon} ⚠️`
+  }
+  const newDeadlineString = newDeadline.tz(tz).format("hh:mm A z")
+  return `${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownAt} ${newDeadlineString}.`


NIT: would making the Language value a function alleviate the need for some of these keys? Example:

const Language = { ttlCausesShutdownAtText: (newDeadlineString: string): string => { return `Your workspace will shut down at ${newDeadlineString}` }, }

I guess it would, but it would make the Language portion more complex. I feel like we'd want to keep Language as a simple lookup table and keep the complexity close to where it's used. I'm fine with going either way though if folks have strong opinions about it!

No strong opinions here - that sounds good to me!

Kira-Pilot

nice work! ✔️ for FE

cli/ttl.go

greyscaled · 2022-06-09T14:06:02Z

cli/ttl.go

+				humanRemaining := "in " + timeRemaining.String()
+				if timeRemaining <= 0 {
+					humanRemaining = "immediately"
+				}
+				_, err = cliui.Prompt(cmd, cliui.PromptOptions{
+					Text: fmt.Sprintf(
+						"Workspace %q will be stopped %s. Are you sure?",
+						workspace.Name,
+						humanRemaining,
+					),


praise: wonderful strings!

greyscaled · 2022-06-09T14:10:23Z

coderd/workspaces_test.go

@@ -550,19 +550,16 @@ func TestWorkspaceUpdateAutostart(t *testing.T) {
 			name:          "invalid location",
 			schedule:      ptr.Ref("CRON_TZ=Imaginary/Place 30 9 * * 1-5"),
 			expectedError: "parse schedule: provided bad location Imaginary/Place: unknown time zone Imaginary/Place",
-			// expectedError: "status code 500: Invalid autostart schedule\n\tError: parse schedule: provided bad location Imaginary/Place: unknown time zone Imaginary/Place",


Nit: Are these leftover debug?

Yep, left over from some previous PR where they snuck in!

greyscaled · 2022-06-09T14:11:20Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.stories.tsx

-Example.args = {
+export const WorkspaceNotRunning = Template.bind({})
+WorkspaceNotRunning.args = {
+  now: dayjs("2022-05-17T17:40:00Z"),


Praise: Thank you for making this constant.

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

greyscaled · 2022-06-09T14:36:42Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

 }

 export interface WorkspaceScheduleFormProps {
  fieldErrors?: FieldErrors
  initialValues?: WorkspaceScheduleFormValues
  isLoading: boolean
+  now: dayjs.Dayjs


Comment(approving): I'm OK with this, it seems like we either don't do this and wrestle with mocks or spies in tests and storybook...or we just do this and live with it being a little weird.

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx

greyscaled · 2022-06-09T14:46:10Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

 }

 export interface WorkspaceScheduleFormProps {
  fieldErrors?: FieldErrors
  initialValues?: WorkspaceScheduleFormValues
  isLoading: boolean
+  now: dayjs.Dayjs


Suggestion(nit): Just let's make it optional:

Suggested change

now: dayjs.Dayjs

now?: dayjs.Dayjs

@johnstcn

This does not finish all tasks in #2175 but is one of the asks. @johnstcn - you should do the same lowercasing of HH->hh in #2165

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

greyscaled · 2022-06-09T20:21:21Z

ah crud I created a conflict for you @johnstcn :(

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.test.ts

johnstcn · 2022-06-09T20:22:06Z

ah crud I created a conflict for you @johnstcn :(

s'alright!

…space-ttl-updates-deadline

This commit adds the following changes to workspace scheduling behaviour: * CLI: updating a workspace TTL updates the deadline of the workspace. * If the TTL is being un-set, the workspace deadline is set to zero. * If the TTL is being set, the workspace deadline is updated to be the last updated time of the workspace build plus the requested TTL. Additionally, the user is prompted to confirm interactively (can be bypassed with -y). * UI: updating the workspace schedule behaves similarly to the CLI, showing a message to the user if the updated TTL/time to shutdown would effect changes to the lifetime of the running workspace.

This commit makes the following changes: - Partially reverts the changes of feat: update workspace deadline when workspace ttl updated #2165, making the deadline of a running workspace build independant of TTL, once started. - CLI: updating a workspace TTL no longer updates the deadline of the workspace. - UI: updating a workspace TTL no longer updates the deadline of the workspace. - Drive-by: API: When creating a workspace, default TTL to min(12 hours, template max_ttl) if not instructed otherwise. - Drive-by: CLI: list: measure workspace extension correctly (+X in last column) from the time the provisioner job was completed - Drive-by: WorkspaceSchedule: show timezone of schedule if it is set, defaulting to dayjs guess otherwise. - Drive-by: WorkspaceScheduleForm: fixed an issue where deleting the "TTL" value in the form would show the text "Your workspace will shut down a few seconds after start".

johnstcn added 5 commits June 8, 2022 11:50

RED: update unit test with new desired behaviour

5c443de

cli: warn user upon ttl-triggered workspace shutdown

c2127c1

GREEN: do the thing

07320a4

REFACTOR: coderd: putWorkspaceTTL: wrap in tx

7846dd8

update unit tests

6d7187d

johnstcn self-assigned this Jun 8, 2022

johnstcn added 4 commits June 8, 2022 21:20

feat: WorkspaceScheduleForm: show shutdown time when editing workspac…

daa9a5f

…e TTL

make it prettier

fe79c0c

hopefully fix unit tests

85da57b

Merge remote-tracking branch 'origin/main' into cj/1783/updating-work…

8c38a74

…space-ttl-updates-deadline

johnstcn marked this pull request as ready for review June 9, 2022 10:49

johnstcn requested a review from a team as a code owner June 9, 2022 10:49

johnstcn requested review from a team and greyscaled June 9, 2022 10:49

johnstcn commented Jun 9, 2022

View reviewed changes

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx Outdated Show resolved Hide resolved

johnstcn commented Jun 9, 2022

View reviewed changes

mafredri approved these changes Jun 9, 2022

View reviewed changes

Kira-Pilot reviewed Jun 9, 2022

View reviewed changes

Kira-Pilot approved these changes Jun 9, 2022

View reviewed changes

johnstcn and others added 2 commits June 9, 2022 14:30

Address PR comments

a4c07df

Merge origin/main

582873a

mafredri reviewed Jun 9, 2022

View reviewed changes

cli/ttl.go Outdated Show resolved Hide resolved

greyscaled reviewed Jun 9, 2022

View reviewed changes

greyscaled added a commit that referenced this pull request Jun 9, 2022

fix: ws schedule as 12-hour format

38ee675

This does not finish all tasks in #2175 but is one of the asks. @johnstcn - you should do the same lowercasing of HH->hh in #2165

greyscaled mentioned this pull request Jun 9, 2022

fix: ws schedule as 12-hour format #2209

Merged

greyscaled reviewed Jun 9, 2022

View reviewed changes

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx Outdated Show resolved Hide resolved

johnstcn added 2 commits June 9, 2022 18:58

chore: WorkspaceScheduleForm: add unit tests

9a7c623

WorkspaceScheduleForm: refactor ttlShutdownAt

0278a93

johnstcn added 3 commits June 9, 2022 19:10

apply suggestions from code review

a372dce

fixup! apply suggestions from code review

ac1d29a

yarn lint; more pr comments

23b474c

johnstcn requested a review from greyscaled June 9, 2022 19:51

bump ci

de28477

greyscaled reviewed Jun 9, 2022

View reviewed changes

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.test.ts Outdated Show resolved Hide resolved

greyscaled approved these changes Jun 9, 2022

View reviewed changes

johnstcn added 3 commits June 9, 2022 21:27

Merge remote-tracking branch 'origin/main' into cj/1783/updating-work…

a52d815

…space-ttl-updates-deadline

mind your language

fd86474

fumpt

2b9f93c

johnstcn merged commit 119db78 into main Jun 9, 2022

johnstcn deleted the cj/1783/updating-workspace-ttl-updates-deadline branch June 9, 2022 21:10

johnstcn mentioned this pull request Jun 10, 2022

TTL/Workspace Schedule is too coupled #2229

Closed

johnstcn mentioned this pull request Jun 14, 2022

fix: coderd: decouple ttl and deadline #2282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update workspace deadline when workspace ttl updated #2165

feat: update workspace deadline when workspace ttl updated #2165

johnstcn commented Jun 8, 2022 •

edited

Loading

johnstcn Jun 9, 2022

johnstcn Jun 9, 2022

johnstcn Jun 9, 2022

greyscaled Jun 9, 2022

greyscaled Jun 9, 2022

mafredri left a comment

mafredri Jun 9, 2022

johnstcn Jun 9, 2022

mafredri Jun 9, 2022

Kira-Pilot Jun 9, 2022

johnstcn Jun 9, 2022

Kira-Pilot Jun 9, 2022

Kira-Pilot left a comment

greyscaled Jun 9, 2022

greyscaled Jun 9, 2022

johnstcn Jun 9, 2022

greyscaled Jun 9, 2022

greyscaled Jun 9, 2022

greyscaled Jun 9, 2022

greyscaled commented Jun 9, 2022

johnstcn commented Jun 9, 2022

feat: update workspace deadline when workspace ttl updated #2165

feat: update workspace deadline when workspace ttl updated #2165

Conversation

johnstcn commented Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kira-Pilot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greyscaled commented Jun 9, 2022

johnstcn commented Jun 9, 2022

johnstcn commented Jun 8, 2022 •

edited

Loading