Skip to content

chore: cover deadline crossing autostart border #13115

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 5 commits into from
May 1, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 30, 2024

Closes #12864

This PR adds the second case in this image. If a user starts a workspace, and that TTL crosses the next autostart, the deadline is set to autostart + TTL

schedule drawio (1)

@Emyrk Emyrk marked this pull request as ready for review April 30, 2024 21:20
@Emyrk Emyrk requested a review from johnstcn May 1, 2024 13:20
// - This crosses the autostart deadline, so the deadline is extended to 9pm
nextAutostart, ok := NextAutostartSchedule(params.Now, params.WorkspaceAutostart, templateSchedule)
if ok && autostop.Deadline.After(nextAutostart) {
autostop.Deadline = nextAutostart.Add(ttl)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle the min(deadline, max_deadline) here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that is handled down below.

autostop.Deadline = autostop.MaxDeadline

I'd rather not duplicate that logic since it also handles the case where MaxDeadline is not set.

templateAutostopRequirement: schedule.TemplateAutostopRequirement{},
userQuietHoursSchedule: "",

expectedDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 21, 0, 0, 0, chicago),
Copy link
Member

Choose a reason for hiding this comment

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

could you use pastDateNight.AddDate() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

AddDate cannot add hours. So it would be AddDate(0,0,1).Add(time.Hour*X) But the number of hours is 23.25 hours, which is such a strange amount.

pastDateNight = 9:45pm on Wednesday
expectedDeadline = 9pm Thursday, which is 12hrs + autostart (which is a cron string, not a Go time.Time).

It was easier for me to reason because the offset being 23.25 hours is not really the assertion I am making. I want to assert it's 12hrs after autostart.

Copy link
Member

Choose a reason for hiding this comment

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

Fair!

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

LGTM, especially with the new test cases!

@Emyrk Emyrk merged commit 845407f into main May 1, 2024
25 checks passed
@Emyrk Emyrk deleted the stevenmasley/schedule_border branch May 1, 2024 15:43
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
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.

Bump stop time even when start time occurs even if there's no activity
2 participants