- Notifications
You must be signed in to change notification settings - Fork928
feat: update workspace deadline when workspace ttl updated#2165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| _, 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: we can skip the prompt iftimeRemaining >= SOME_CUTOFF.
| if testCase.expectedDeadline != nil { | ||
| require.WithinDuration(t, *testCase.expectedDeadline, updated.LatestBuild.Deadline, time.Minute, "expected autostop deadline to be equal expected") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| export interface WorkspaceScheduleFormProps { | ||
| fieldErrors?: FieldErrors | ||
| initialValues?: WorkspaceScheduleFormValues | ||
| isLoading: boolean | ||
| now: dayjs.Dayjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion(nit): Just let's make it optional:
| now:dayjs.Dayjs | |
| now?:dayjs.Dayjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Clean change! I had one question but otherwise LGTM (backend)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| if latestBuild.UpdatedAt.IsZero() { | ||
| // Build in progress; provisionerd should update with the new TTL. | ||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See
coder/coderd/provisionerdaemons.go
Line 598 in567e4af
| workspaceDeadline=now.Add(time.Duration(workspace.Ttl.Int64)) |
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ooh yeah that makes sense, thanks for clarifying!
| return `⚠️ ${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownSoon} ⚠️` | ||
| } | ||
| const newDeadlineString = newDeadline.tz(tz).format("hh:mm A z") | ||
| return `${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownAt} ${newDeadlineString}.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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}` },}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess it would, but it would make theLanguage portion more complex. I feel like we'd want to keepLanguage 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No strong opinions here - that sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice work! ✔️ for FE
Uh oh!
There was an error while loading.Please reload this page.
| 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, | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
praise: wonderful strings!
| @@ -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", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Are these leftover debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, left over from some previous PR where they snuck in!
| Example.args = { | ||
| export const WorkspaceNotRunning = Template.bind({}) | ||
| WorkspaceNotRunning.args = { | ||
| now: dayjs("2022-05-17T17:40:00Z"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Praise: Thank you for making this constant.
site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| export interface WorkspaceScheduleFormProps { | ||
| fieldErrors?: FieldErrors | ||
| initialValues?: WorkspaceScheduleFormValues | ||
| isLoading: boolean | ||
| now: dayjs.Dayjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| export interface WorkspaceScheduleFormProps { | ||
| fieldErrors?: FieldErrors | ||
| initialValues?: WorkspaceScheduleFormValues | ||
| isLoading: boolean | ||
| now: dayjs.Dayjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion(nit): Just let's make it optional:
| now:dayjs.Dayjs | |
| now?:dayjs.Dayjs |
site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ah crud I created a conflict for you@johnstcn :( |
site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.test.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
s'alright! |
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".
Uh oh!
There was an error while loading.Please reload this page.
This PR adds the following changes:
-y).Closes#1783