refactor: workspace autostop_schedule -> ttl by johnstcn · Pull Request #1578 · coder/coder

johnstcn · 2022-05-18T20:50:18Z

This PR makes the following changes:

Adds a migration to replace the autostop_schedule column of workspaces with a ttl.
- Note: I elected to store this as a 64-bit integer, as apparently pq (still) does not support time.Duration.
Replaces all existing usages of autostop_schedule to ttl in database code etc.
Renames CLI command autostop to ttl, updates API routes accordingly
- Note: To maintain consistency with the autostart behaviour, ttl is truncated to time.Minute. A zero value or only seconds value for TTL is not valid per the CLI.
Lifecycle executor now checks if TTL is set and greater than 0 instead of checking the next scheduled time of autostop schedule.
- Note: I am doing a small bit of fudging of the autostop deadline by adding one minute to ensure that we don't terminate too early.

deansheather · 2022-05-19T14:08:13Z

+		if ws.TTL == nil || *ws.TTL == 0 {
 			return time.Time{}, nil
 		}


Instead of using a pointer why not just have 0 mean disabled?

I considered that initially, but I think using the zero value could lead to bugs and/or unintended behaviour. Grey also preferred the semantics of nil | time.Duration from a FE consumption perspective.

You do have a point, though -- 0 is already an invalid TTL.

Comment: I think the FE or other clients having to know 0 means "manual" is unfortunate, but I'm willing to accept it if it's cleaner for our backend/APIs.

As stated earlier, from v2 forwards I believe it's best that we look at the Coder system as a frontend for a backend, not the other way around.

Should we then consider that in the API response? We could return something like {"ttl": 0, "ttl_mode": "manual"}?

It's certainly golang convention to make use of "zero-values" in this way, but I don't think we should extend that to the HTTP API, CLI, or frontend, which should behave in ways that are independent of golang's idioms.

@spikecurtis are you referring to @deansheather 's comment here: #1578 (comment)

why not just have 0 mean disabled?

Or the fact that we're using a pointer such that the value can be nil or a number.

Yeah, I feel like among go practitioners, 0 means disabled would be preferred to making it a pointer and nil means disabled.

greyscaled

Might need to generate the types, since I see codersdk changed types.

This will have FE breakages.

If you don't mind, I can push those fixes to this PR so that it's all taken care of in one. Are you cool with that?

greyscaled · 2022-05-19T15:51:08Z

+		if ws.TTL == nil || *ws.TTL == 0 {
 			return time.Time{}, nil
 		}


Comment: I think the FE or other clients having to know 0 means "manual" is unfortunate, but I'm willing to accept it if it's cleaner for our backend/APIs.

As stated earlier, from v2 forwards I believe it's best that we look at the Coder system as a frontend for a backend, not the other way around.

mafredri

LGTM 👍🏻. Attached some (optional) suggestions.

mafredri · 2022-05-19T16:35:23Z

-type UpdateWorkspaceAutostopRequest struct {
-	Schedule string `json:"schedule"`
+type UpdateWorkspaceTTLRequest struct {
+	TTL *time.Duration `json:"ttl"`


Suggestion: Should we implement a custom Duration type like type NullDuration { Duration time.Duration; Valid bool } (optionally in a types or null package) with a custom json.Marshaler? I'm mostly thinking about (Go) consumers here. It's not possible to e.g. do:

req := UpdateWorkspaceTTLRequest { TTL: &time.Duration(time.Second), }

without first assigning to a variable. Similarly the pointer usage could lead to null pointer dereferences (which arguably would be the "correct" behavior, unless it's known that the zero value is OK).

Co-authored-by: G r e y <grey@coder.com>

johnstcn self-assigned this May 18, 2022

johnstcn commented May 18, 2022

View reviewed changes

Comment thread coderd/database/migrations/999999_rename_this_autostop_ttl.down.sql

johnstcn commented May 18, 2022

View reviewed changes

Comment thread coderd/database/migrations/999999_rename_this_autostop_ttl.up.sql

johnstcn commented May 18, 2022

View reviewed changes

Comment thread coderd/database/queries/workspaces.sql

johnstcn force-pushed the cj/workspace-autostop-ttl branch 2 times, most recently from 0593de8 to 22cd000 Compare May 19, 2022 11:21

johnstcn marked this pull request as ready for review May 19, 2022 13:41

greyscaled reviewed May 19, 2022

View reviewed changes

Comment thread cli/ttl.go Outdated

johnstcn requested a review from a team May 19, 2022 13:55

deansheather reviewed May 19, 2022

View reviewed changes

dwahler reviewed May 19, 2022

View reviewed changes

Comment thread coderd/coderd.go Outdated

greyscaled reviewed May 19, 2022

View reviewed changes

Comment thread agent/usershell/usershell_darwin.go

greyscaled reviewed May 19, 2022

View reviewed changes

johnstcn requested a review from a team as a code owner May 19, 2022 16:35

mafredri approved these changes May 19, 2022

View reviewed changes

johnstcn and others added 11 commits May 19, 2022 19:21

refactor: workspace: autostop_schedule -> ttl

3980007

fix: truncate ttl to time.Minute

3ea3e90

noop: rename migration

ea49127

cli: rename ttl enable/disable -> ttl set/unset

a06218f

address PR comments

58d72c3

make gen

6f67b79

fixup! make gen

f65e3de

cli: ttl: xerrors.Errorf everywhere

77b8ad3

frontend

80d7b1c

codersdk change

32c92f6

fixup! frontend

b3168c1

johnstcn force-pushed the cj/workspace-autostop-ttl branch from badcfec to b3168c1 Compare May 19, 2022 18:34

greyscaled reviewed May 19, 2022

View reviewed changes

Comment thread site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx Outdated

Update site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx

d00fefb

greyscaled reviewed May 19, 2022

View reviewed changes

Comment thread site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx Outdated

greyscaled added 3 commits May 19, 2022 14:45

unnecessary else

d8ccd6c

fixup api param

22f0222

fix FE paths for ttl

50a5823

greyscaled approved these changes May 19, 2022

View reviewed changes

greyscaled changed the title ~~refactor: workspace: autostop_schedule -> ttl~~ refactor: workspace autostop_schedule -> ttl May 19, 2022

greyscaled merged commit d72c45e into main May 19, 2022

greyscaled deleted the cj/workspace-autostop-ttl branch May 19, 2022 19:09

greyscaled mentioned this pull request May 19, 2022

See the next scheduled auto-off time in the UI #1458

Closed

kylecarbs pushed a commit that referenced this pull request Jun 10, 2022

refactor: workspace autostop_schedule -> ttl (#1578)

f7c19fd

Co-authored-by: G r e y <grey@coder.com>

Conversation

johnstcn commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greyscaled left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

johnstcn commented May 18, 2022 •

edited

Loading