- Notifications
You must be signed in to change notification settings - Fork925
feat: add inactivity cleanup and failure cleanup configuration fields to Template Schedule Form#7402
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
…plate-route/kira-pilot
bpmct commentedMay 4, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Let's set "7 days" as a default for failure cleanup when enabled. And "180 days" for inactivity cleanup. It's pretty arbitrary, but here's my thinking:
And an admin can always tweak these, but I'd rather default to something more conservative and allow the admin to add more aggressive settings. After all, its disabled by default. |
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateScheduleForm.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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.
The FE code looks good to me
| }, nil | ||
| } | ||
| func (*enterpriseTemplateScheduleStore) SetTemplateScheduleOptions(ctx context.Context, db database.Store, tpl database.Template, opts schedule.TemplateScheduleOptions) (database.Template, error) { | ||
| if int64(opts.DefaultTTL) == tpl.DefaultTTL && | ||
| int64(opts.MaxTTL) == tpl.MaxTTL && | ||
| int64(opts.FailureTTL) == tpl.FailureTTL && | ||
| int64(opts.InactivityTTL) == tpl.InactivityTTL && |
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.
@deansheather I see us making what looks like a duplicate check in thepatchTemplateMetahandler - curious why we need to check at all (I don't see us doing this in other routes) and also why we need to check twice, once in the handler and once in the store.
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're right this does seem unnecessary and can probably be removed from the handlers and just kept in the store. Make sure the AGPL version of the store has the equivalent check though (but only for the AGPL-permitted fields)!
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.
Great, will handle that in a follow-up!
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.
BE looks good, I'm glad you could use of the TemplateScheduleStore interface for this
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| }, nil | ||
| } | ||
| func (*enterpriseTemplateScheduleStore) SetTemplateScheduleOptions(ctx context.Context, db database.Store, tpl database.Template, opts schedule.TemplateScheduleOptions) (database.Template, error) { | ||
| if int64(opts.DefaultTTL) == tpl.DefaultTTL && | ||
| int64(opts.MaxTTL) == tpl.MaxTTL && | ||
| int64(opts.FailureTTL) == tpl.FailureTTL && | ||
| int64(opts.InactivityTTL) == tpl.InactivityTTL && |
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're right this does seem unnecessary and can probably be removed from the handlers and just kept in the store. Make sure the AGPL version of the store has the equivalent check though (but only for the AGPL-permitted fields)!
…wn.sqlCo-authored-by: Dean Sheather <dean@deansheather.com>
….sqlCo-authored-by: Dean Sheather <dean@deansheather.com>
Uh oh!
There was an error while loading.Please reload this page.
Resolves#7346 andresolves#7347.
#7401 will be a fast follow.Spoke with Ben - we will not be adding these fields to the new template form at the moment.Run locally via
CODER_EXPERIMENTS='workspace_actions' ./scripts/develop.shSome notes:
workspace_actionsfeature. I feel like for now, this feature can live under thecodersdk.FeatureAdvancedTemplateSchedulingentitlement. It is still feature flagged separately with a new experiment (workspace_actions)I am using 14 days as a default value for both "Failure Cleanup" and "Inactivity Cleanup".@bpmct does this seem reasonable?Changed to 7 and 180 respectively as per Ben's comment.Demo
Loom Link
Auditing works, too: