- Notifications
You must be signed in to change notification settings - Fork928
fix: allow setting workspace deadline as early as now plus 30 minutes#2328
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
coderd/workspaces.go Outdated
| now := time.Now() | ||
| if new.Before(now) { | ||
| return xerrors.New("new deadline must be in the future") | ||
| } | ||
| delta := new.Sub(old) | ||
| if delta < time.Minute { | ||
| return xerrors.New("minimum extension is one minute") | ||
| } | ||
| if delta > 24*time.Hour { | ||
| return xerrors.New("maximum extension is 24 hours") |
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.
review: Dropping these validations for the moment till we have more information from#2318
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.
Actually, there's one very important validation involving templates that I may have overlooked! Adding this in now.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // And with a deadline greater than the template max_ttl should also fail | ||
| deadlineExceedsMaxTTL := time.Now().Add(time.Duration(template.MaxTTLMillis) * time.Millisecond).Add(time.Minute) | ||
| err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ | ||
| Deadline: deadlineExceedsMaxTTL, | ||
| }) | ||
| require.ErrorContains(t, err, "new deadline is greater than template allows", "setting a deadline greater than that allowed by the template should fail") |
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.
review: I forgot to add this in earlier. We don't folks folks sneaking around template-level restrictions this way.
| return xerrors.New("minimum extension is one minute") | ||
| // No idea how this could happen. | ||
| ifnewDeadline.Before(startedAt) { | ||
| return xerrors.Errorf("new deadline must be before workspace start time") |
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.
I figure if we get here, time is broken.
| if build.Deadline.IsZero() { | ||
| code = http.StatusConflict | ||
| resp.Message = "Workspace shutdown is manual." | ||
| return xerrors.Errorf("workspace shutdown is manual") | ||
| } |
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.
review: I'm not allowing this yet as if someone does, then there's currently no way to un-set this.
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.
LGTM except for "bump" doesn't make sense anymore
| @@ -62,7 +72,6 @@ func bump() *cobra.Command { | |||
| newDeadline.Format(timeFormat), | |||
| newDeadline.Format(dateFormat), | |||
| ) | |||
| return nil | |||
| }, | |||
| } | |||
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.
Also: this won't be calledcoder bump any more.
This PR makes the following changes:
/api/v2/workspaces/:workspace/extendnow accepts any time at least 30 minutes in the futurecoder bumpcommand also allows the above. Some small copy changes to command.UI changes to come later.