Skip to content

feat: add locked TTL field to template meta #8020

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

Merged
merged 8 commits into from
Jun 20, 2023
Merged

feat: add locked TTL field to template meta #8020

merged 8 commits into from
Jun 20, 2023

Conversation

sreya
Copy link
Collaborator

@sreya sreya commented Jun 14, 2023

No description provided.

@sreya sreya requested a review from Kira-Pilot June 15, 2023 18:50
@sreya sreya marked this pull request as ready for review June 15, 2023 18:50
@@ -635,6 +635,7 @@ func TestPatchTemplateMeta(t *testing.T) {
const (
failureTTL = 7 * 24 * time.Hour
inactivityTTL = 180 * 24 * time.Hour
lockedTTL = 360 * 24 * time.Hour
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I think I stuck my tests in the coderd folder as well - I'm wondering if they should actually live in enterprise/coderd

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably good to have tests in both

@@ -77,6 +77,7 @@ var auditableResourcesTypes = map[any]map[string]Action{
"max_ttl": ActionTrack,
"failure_ttl": ActionTrack,
"inactivity_ttl": ActionTrack,
"locked_ttl": ActionTrack,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😸

@@ -158,6 +167,25 @@ export const TemplateScheduleForm: FC<TemplateScheduleForm> = ({
}
}

const handleToggleLockedCleanup = async (e: ChangeEvent) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Since we now have 3 toggling functions, we might be able to refactor into one function that takes two form values, e.g. locked_cleanup_enabled and locked_ttl_ms and appropriately toggles TTL.

ttl={form.values.locked_ttl_ms}
/>,
)}
disabled={isSubmitting || !form.values.locked_cleanup_enabled}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this field also be disabled if Inactivity Cleanup is disabled? Not sure that it makes sense to have one without the other, although perhaps we'll have manual locking down the road:
Screenshot 2023-06-15 at 3 17 22 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that probably makes sense, we can fix it in a follow up PR.

@sreya sreya merged commit c3aef93 into main Jun 20, 2023
@sreya sreya deleted the jon/lockedttl branch June 20, 2023 02:37
@github-actions github-actions bot locked and limited conversation to collaborators Jun 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants