-
Notifications
You must be signed in to change notification settings - Fork 936
fix(coderd): make activitybump aware of default template ttl #10253
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The refactored ActivityBump query did not take into account the template-level TTL, resulting in potentially incorrect bump amounts for workspaces that have both a user-defined and template- defined TTL that differ. This change is ported over from PR#10035 to reduce the overall size of that PR. Co-Authored-By: Dean Sheather <dean@deansheather.com>
These changes have been broken out to a separate PR: #10253 Removing from this PR
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.
Except for lacking context, this looks good (assuming my concern is a non-issue)!
CASE | ||
WHEN templates.allow_user_autostop | ||
THEN (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval | ||
ELSE (templates.default_ttl / 1000 / 1000 / 1000 || ' seconds')::interval |
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.
Perhaps I'm missing some context, but is the workspace.ttl
the same as this setting?

If yes, that's IMO quite different from the template one:

In the screenshots, the workspace autostop does not IMO translate to a TTL and would produce unexpected results given this query logic. But as I said, I may be missing context and perhaps this is hat Deans PR is about (I haven't looked at 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.
Yeah this makes more sense in the context of the original PR. I do agree that the current autostop/TTL behaviour has grown unwieldy though.
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.
Port LGTM!
These changes have been broken out to a separate PR: #10253 Removing from this PR
The refactored ActivityBump query did not take into account the
template-level TTL, resulting in potentially incorrect bump
amounts for workspaces that have both a user-defined and template-
defined TTL that differ.
This change is ported over from PR#10035 to reduce the overall
size of that PR.
Also includes a drive-by unit test in autobuild for checking template autostop/TTL.