feat: add template max_ttl #6114
Conversation
7702d2d to
3a8cb3a
Compare
a5b7723 to
ad64806
Compare
37d4193 to
e28e32e
Compare
| cmd.Flags().StringVarP(&description, "description", "", "", "Edit the template description.") | ||
| cmd.Flags().StringVarP(&icon, "icon", "", "", "Edit the template icon path.") | ||
| cmd.Flags().DurationVarP(&defaultTTL, "default-ttl", "", 0, "Edit the template default time before shutdown - workspaces created from this template default to this value.") | ||
| cmd.Flags().DurationVarP(&maxTTL, "max-ttl", "", 0, "Edit the template maximum time before shutdown - workspaces created from this template must shutdown within the given duration after starting. This is an enterprise-only feature.") |
There was a problem hiding this comment.
This is an enterprise-only feature
nit: do we need to mention it? If we decide to open this feature to the OSS audience, it would be misleading. I'm not sure what's the rule of thumb for enterprise features in general.
There was a problem hiding this comment.
Seems weird to not mention it if it's a licensed feature, otherwise people will try --max-ttl and see errors. The frontend does something similar, by showing the box but disabling it and adding a "enterprise" label
There was a problem hiding this comment.
Is it common for other flags? I might have missed it.
There was a problem hiding this comment.
I have done it for deployment config flags before but I'm unsure if we've done it for CLI flags.
|
|
||
| // TemplateScheduleStore provides an interface for retrieving template | ||
| // scheduling options set by the template/site admin. | ||
| type TemplateScheduleStore interface { |
There was a problem hiding this comment.
I'm wondering why is TemplateScheduleStore an interface. I see only one implementation here: agplTemplateScheduleStore. Is it for testing purposes?
Ok, I see now, there is one for enterprise too.
WDYT about renaming stores to be more descriptive instead of agpl... and enterprise.... If we decide to introduce another pricing plan, then we will need to rename them accordingly :) I would move these implementations to a seperate package, maybe schedule or scheduling?
There was a problem hiding this comment.
We currently have a schedule package but it's under autobuild/schedule, whereas this package doesn't pertain to autostart at all, only autostop...
Maybe I should move the existing schedule package to coderd/schedule and then copy this code to it instead?
There was a problem hiding this comment.
This is done, I've moved coderd/autobuild/schedule to coderd/schedule and added the code from this file.
There was a problem hiding this comment.
This is not entirely what I meant. Currently, the agpl... and enterprise... logic are scattered. The idea is place them in the same package, but rename accordingly. These are just different strategies, so it's perfectly fine to keep them together.
For instance, agplTemplateScheduleStore -> userControlledTemplateScheduleStore, enterpriseTemplateScheduleStore -> maxTTLTemplateScheduleStore.
There was a problem hiding this comment.
We cannot place them in the same package as AGPL code can't be mixed with enterprise code, and there needs to be an implementation outside of AGPL otherwise AGPL-compiled binaries wouldn't be able to compile properly.
There was a problem hiding this comment.
I can change the names of them but I feel the names you propose are too specific especially if we add other features to them in the future.
There was a problem hiding this comment.
I see, that's unfortunate. Let's leave as is.
There was a problem hiding this comment.
This is the same pattern we use for other enterprise features that hide behind interfaces btw
| } | ||
|
|
||
| return provisionerdserver.TemplateScheduleOptions{ | ||
| // TODO: make configurable at template level |
| "defaultTTLHelperText_other": "Workspaces will default to stopping after {{count}} hours.", | ||
| "maxTTLHelperText_zero": "Workspaces may run indefinitely.", | ||
| "maxTTLHelperText_one": "Workspaces must stop within 1 hour of starting.", | ||
| "maxTTLHelperText_other": "Workspaces must stop within {{count}} hours of starting.", |
|
@Kira-Pilot I won't take credit for the FE, Bruno tidied it up for me :D |
Adds an optional max TTL setting to templates. This value will apply to workspaces after they are restarted (similarly to how scheduling changes only apply after restart).
max_ttl(alongside the existingdefault_ttl) in the templates table.max_deadlineto the workspace_builds table, which is set totime.Now().Add(template.maxTTL)if the template has a max TTL set.TODO:
Closes #6047