- Notifications
You must be signed in to change notification settings - Fork1k
feat: allow disabling autostart and custom autostop for template#6933
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
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 can only imagine how much time you spent on wiring it all together :)
Anyway, I went through the source, but due to the massive size of the PR, please keep in mind that for sure I missed something.
General comments:
- Autostart vs "auto start", it seems that we're using the first one.
- I checked the main flow: set options via template settings, and make sure that the user can autostart or autostop the workspace. It seems 👍 to me.
- Maybe try to split it into smaller chunks next time :)
| ADD COLUMN"allow_user_auto_start"boolean DEFAULT trueNOT NULL, | ||
| ADD COLUMN"allow_user_auto_stop"boolean DEFAULT trueNOT NULL; |
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.
You won't like this comment... In the codebase, we're using "autostart" and "autostop".
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.
nothing a sed can't fix :^)
| updated_at= $2, | ||
| default_ttl= $3, | ||
| max_ttl= $4 | ||
| allow_user_auto_start= $3, |
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.
Oh, is it safe and backward-compatible to change the order of items here?
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.
It shouldn't matter since the generated Golang query uses named parameters in a struct, not ordered parameters in a function.
| Telemetry:api.Telemetry, | ||
| Auditor:&api.AGPL.Auditor, | ||
| TemplateScheduleStore:&api.AGPL.TemplateScheduleStore, | ||
| TemplateScheduleStore:api.AGPL.TemplateScheduleStore, |
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: is it intentional change?
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.
Yes,provisionerdserver was using a*atomic.Pointer[T] so we had to use& before sincecoderd was only usingatomic.Pointer[T].coderd now uses*atomic.Pointer[T] so we just copy the pointer here.
| returnxerrors.Errorf("notify systemd: %w",err) | ||
| } | ||
| autobuildPoller:=time.NewTicker(cfg.AutobuildPollInterval.Value()) |
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: why did you remove this part?
Maybe I have to go through the entire review to find the answer...
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.
executor.New in the review.
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.
Yeah this was a mistake, good catch to both of you. I removed this code and moved it somewhere else, and decided I wanted to move it back but forgot to put it back I guess.
| e.log.Error(e.ctx,"get workspaces for autostart or autostop",slog.Error(err)) | ||
| returnstats | ||
| } | ||
| workspaces:=database.ConvertWorkspaceRows(workspaceRows) |
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.
👍
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.
Is the deletion incli/server.go on line 1020 expected?
| funcisEligibleForAutoStartStop(ws database.Workspace,priorHistory database.WorkspaceBuild,templateSchedule schedule.TemplateScheduleOptions)bool { | ||
| ifws.Deleted { | ||
| returnfalse | ||
| } | ||
| iftemplateSchedule.UserAutoStartEnabled&&ws.AutostartSchedule.Valid&&ws.AutostartSchedule.String!="" { | ||
| returntrue | ||
| } | ||
| // Don't check the template schedule to see whether it allows autostop, this | ||
| // is done during the build when determining the deadline. | ||
| ifpriorHistory.Transition==database.WorkspaceTransitionStart&&!priorHistory.Deadline.IsZero() { | ||
| returntrue | ||
| } | ||
| returnfalse |
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.
👍
coderd/templates_test.go Outdated
| allowAutoStart atomic.Bool | ||
| allowAutoStop atomic.Bool |
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.
When I initially looked at this, I was wondering whether it would make more sense to instead use achan bool instead to assert the values passed tosetFn. But this does seem to be simpler in some ways and, crucially, will failquickly if it fails. Not so for a channel... so 👍
| returnxerrors.Errorf("notify systemd: %w",err) | ||
| } | ||
| autobuildPoller:=time.NewTicker(cfg.AutobuildPollInterval.Value()) |
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.
executor.New in the review.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I'll make a ticket for doing frontend in a follow-up PR. |
Actually we can just reuse the other tickets. |
Uh oh!
There was an error while loading.Please reload this page.
Adds template controls for disabling autostart and for disabling user-customizable autostop for all workspaces on the template. Autostart preferences apply instantly, but autostop preferences will apply after workspaces are rebuilt.
TODO:
Relates to#6047
Relates to#6497