- Notifications
You must be signed in to change notification settings - Fork928
feat: add default autostart and ttl for new workspaces#1632
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
| @@ -94,6 +94,9 @@ export interface CreateWorkspaceBuildRequest { | |||
| export interface CreateWorkspaceRequest { | |||
| readonly template_id: string | |||
| readonly name: string | |||
| readonly autostart_schedule?: string | |||
| // This is likely an enum in an external package ("time.Duration") | |||
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.
@Emyrk Can we not have this happen for time.Duration?
Uh oh!
There was an error while loading.Please reload this page.
| if schedule.Location == time.Local { | ||
| return nil, xerrors.Errorf("schedules scoped to time.Local are not supported") | ||
| } |
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.
NOTE: added an extra check here, we don't want these sneaking in.
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.
TIL:time.Local !=time.UTC even whenTZ=UTC, cool!
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.
FE = ✔️
Is this implemented globally or by template? Also, how does one create a workspace without a TTL? |
ammario commentedMay 23, 2022 • 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.
If it happens we're implementing it site-level, we should consider implementing it on the organization level. See#617. |
Only by workspace right now. There's a separate issue for by-template.
I figured we would want to enforce a default TTL that users can then un-set if required. If this isn't the case, we can add an opt-out option. |
Oh, I meant the default. It looks like it's implemented installation-wide with a CLI flag.
I think this is fine. And, I can see how implementing it site-wide via CLI first is easier than doing an org option. |
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.
Code changes look good! Some small suggestions, feel free to amend or dismiss.
| @@ -54,6 +60,20 @@ func create() *cobra.Command { | |||
| } | |||
| } | |||
| tz, err := time.LoadLocation(tzName) | |||
| if err != nil { | |||
| return xerrors.Errorf("Invalid workspace autostart timezone: %w", err) | |||
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.
nit: Error strings should not be capitalized:https://github.com/golang/go/wiki/CodeReviewComments#error-strings
cli/create.go Outdated
| @@ -207,5 +229,10 @@ func create() *cobra.Command { | |||
| cliui.AllowSkipPrompt(cmd) | |||
| cliflag.StringVarP(cmd.Flags(), &templateName, "template", "t", "CODER_TEMPLATE_NAME", "", "Specify a template name.") | |||
| cliflag.StringVarP(cmd.Flags(), ¶meterFile, "parameter-file", "", "CODER_PARAMETER_FILE", "", "Specify a file path with parameter values.") | |||
| cliflag.StringVarP(cmd.Flags(), &autostartMinute, "autostart-minute", "", "CODER_WORKSPACE_AUTOSTART", "0", "Specify the minute(s) at which the workspace should autostart.") | |||
| cliflag.StringVarP(cmd.Flags(), &autostartHour, "autostart-hour", "", "CODER_WORKSPACE_AUTOSTART", "9", "Specify the hour(s) at which the workspace should autostart.") | |||
| cliflag.StringVarP(cmd.Flags(), &autostartDow, "autostart-day-of-week", "", "CODER_WORKSPACE_AUTOSTART", "MON-FRI", "Specify the days(s) on which the workspace should autostart.") | |||
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.
A user might wonder how this should look if they want Monday, Wednesday and Friday (e.g.MON,WED,FRI, multiple flags)? Should we add some examples to illustrate acceptable formats to the user?
cli/create.go Outdated
| cliflag.StringVarP(cmd.Flags(), &autostartHour, "autostart-hour", "", "CODER_WORKSPACE_AUTOSTART", "9", "Specify the hour(s) at which the workspace should autostart.") | ||
| cliflag.StringVarP(cmd.Flags(), &autostartDow, "autostart-day-of-week", "", "CODER_WORKSPACE_AUTOSTART", "MON-FRI", "Specify the days(s) on which the workspace should autostart.") | ||
| cliflag.StringVarP(cmd.Flags(), &tzName, "tz", "", "TZ", "", "Specify your timezone location for workspace autostart.") | ||
| cliflag.DurationVarP(cmd.Flags(), &ttl, "ttl", "", "CODER_WORKSPACE_TTL", 8*time.Hour, "Specify a TTL for the workspace.") |
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.
| cliflag.DurationVarP(cmd.Flags(),&ttl,"ttl","","CODER_WORKSPACE_TTL",8*time.Hour,"Specify a TTL for the workspace.") | |
| cliflag.DurationVarP(cmd.Flags(),&ttl,"ttl","","CODER_WORKSPACE_TTL",8*time.Hour,"Specify atime-to-live (TTL) for the workspace.") |
| err := cmd.Execute() | ||
| require.EqualError(t, err, "Invalid workspace autostart timezone: unknown time zone invalid") | ||
| }() | ||
| <-doneChan |
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.
As we were hunting this style today, should we eliminate them here as well? 😄
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.
For these ones, I'm going to just useassert instead ofrequire.
| if schedule.Location == time.Local { | ||
| return nil, xerrors.Errorf("schedules scoped to time.Local are not supported") | ||
| } |
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.
TIL:time.Local !=time.UTC even whenTZ=UTC, cool!
coderd/workspaces.go Outdated
| Name: createWorkspace.Name, | ||
| ID: uuid.New(), | ||
| CreatedAt: database.Now(), | ||
| UpdatedAt: database.Now(), |
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.
Minor: Do we want to use the same value for these?
codersdk/organizations.go Outdated
| Name string `json:"name" validate:"username,required"` | ||
| TemplateID uuid.UUID `json:"template_id" validate:"required"` | ||
| Name string `json:"name" validate:"username,required"` | ||
| AutostartSchedule *string `json:"autostart_schedule" validate:""` |
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.
| AutostartSchedule*string`json:"autostart_schedule" validate:""` | |
| AutostartSchedule*string`json:"autostart_schedule"` |
| @@ -41,6 +41,13 @@ func Test_Weekly(t *testing.T) { | |||
| expectedTz: "UTC", | |||
| expectedString: "CRON_TZ=UTC 30 9 * * 1-5", | |||
| }, | |||
| { | |||
| name: "time.Local will bite you", | |||
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.
❤️
| @@ -357,6 +357,25 @@ func (api *api) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req | |||
| return | |||
| } | |||
| var dbAutostartSchedule sql.NullString | |||
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 don't have a strong reasoning for this, but I don't love using nullstring. I know there's a lot ofNOT NULL DEFAULT '' in v1 and I generally prefer than to handling a null type. Do you have any thoughts on 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.
Is it mainly the extra overhead of managing the pointer type as opposed to just checking for the empty string?
I can do a separate PR to migrate these types on the database layer, but I'd like to keep the existingnull | String API for the FE.
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.
TIL what the squirrel detective signifies. 🤣 |
* database: add autostart_schedule and ttl to InsertWorkspace; make gen* coderd: workspaces: consume additional fields of CreateWorkspaceRequest* cli: update: add support for TTL and autostart_schedule* cli: create: add unit tests* coder: import `time/tzdata` for embedded timezone database* autobuild: fix unit test that only runs with a real db
What
This PR adds default values for autostart and TTL values for all new workspaces.
NOTE: I am open to just keeping the TTL and leaving the autostart if folks think that's preferable.
Fixes#634