- Notifications
You must be signed in to change notification settings - Fork1k
cli: streamline autostart ux#2251
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.
Changes fromall commits
7c7a2908983ef984f1a12bad0ac268187d8bcb1e8c2556ba20884e0c377b70825ef4374bab79fe6921a79e767916a82f77File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2,32 +2,41 @@ package cli | ||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| "time" | ||
| "github.com/spf13/cobra" | ||
| "golang.org/x/xerrors" | ||
| "github.com/coder/coder/coderd/autobuild/schedule" | ||
| "github.com/coder/coder/coderd/util/ptr" | ||
| "github.com/coder/coder/coderd/util/tz" | ||
| "github.com/coder/coder/codersdk" | ||
| ) | ||
| const autostartDescriptionLong = `To have your workspace build automatically at a regular time you can enable autostart. | ||
| When enabling autostart, enter a schedule in the format: <start-time> [day-of-week] [location]. | ||
| * Start-time (required) is accepted either in 12-hour (hh:mm{am|pm}) format, or 24-hour format hh:mm. | ||
| * Day-of-week (optional) allows specifying in the cron format, e.g. 1,3,5 or Mon-Fri. | ||
| Aliases such as @daily are not supported. | ||
| Default: * (every day) | ||
| * Location (optional) must be a valid location in the IANA timezone database. | ||
| If omitted, we will fall back to either the TZ environment variable or /etc/localtime. | ||
| You can check your corresponding location by visiting https://ipinfo.io - it shows in the demo widget on the right. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This help is ❤️. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yes, this is excellent. | ||
| ` | ||
| func autostart() *cobra.Command { | ||
| autostartCmd := &cobra.Command{ | ||
| Annotations: workspaceCommand, | ||
| Use: "autostartset <workspace> <start-time> [day-of-week] [location]", | ||
| Short: "schedule a workspace to automatically start at a regular time", | ||
| Long: autostartDescriptionLong, | ||
| Example: "coder autostartset my-workspace9:30AM Mon-Fri Europe/Dublin", | ||
| } | ||
| autostartCmd.AddCommand(autostartShow()) | ||
| autostartCmd.AddCommand(autostartSet()) | ||
| autostartCmd.AddCommand(autostartUnset()) | ||
| return autostartCmd | ||
| } | ||
| @@ -60,13 +69,12 @@ func autostartShow() *cobra.Command { | ||
| } | ||
| next := validSchedule.Next(time.Now()) | ||
| _, _ = fmt.Fprintf(cmd.OutOrStdout(), | ||
| "schedule: %s\ntimezone: %s\nnext: %s\n", | ||
| validSchedule.Cron(), | ||
| validSchedule.Location(), | ||
| next.In(validSchedule.Location()), | ||
| ) | ||
| return nil | ||
| @@ -75,23 +83,17 @@ func autostartShow() *cobra.Command { | ||
| return cmd | ||
| } | ||
| func autostartSet() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "set <workspace_name> <start-time> [day-of-week] [location]", | ||
| Args: cobra.RangeArgs(2, 4), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| client, err := createClient(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| sched, err := parseCLISchedule(args[1:]...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| @@ -102,32 +104,30 @@ func autostartEnable() *cobra.Command { | ||
| } | ||
| err = client.UpdateWorkspaceAutostart(cmd.Context(), workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ | ||
| Schedule:ptr.Ref(sched.String()), | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| schedNext := sched.Next(time.Now()) | ||
| _, _ = fmt.Fprintf(cmd.OutOrStdout(), | ||
| "%s will automatically start at %s %s (%s)\n", | ||
| workspace.Name, | ||
| schedNext.In(sched.Location()).Format(time.Kitchen), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Do we always want to show am/pm here, even if 24h was used to define it? Or perhaps vary depending on defaults for the users system or selected location? Or perhaps we should have a user preference down the line? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This sounds like an internationalization problem to me, which I'm going to place inside a tin can and kick down the road for later. 😅 But yes, yes we should. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I think preference is very location-dependent. It would be unfortunate to add more state for am/pm. | ||
| sched.DaysOfWeek(), | ||
| sched.Location().String(), | ||
| ) | ||
| return nil | ||
| }, | ||
| } | ||
| return cmd | ||
| } | ||
| funcautostartUnset() *cobra.Command { | ||
| return &cobra.Command{ | ||
| Use: "unset <workspace_name>", | ||
| Args: cobra.ExactArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| client, err := createClient(cmd) | ||
| @@ -147,9 +147,98 @@ func autostartDisable() *cobra.Command { | ||
| return err | ||
| } | ||
| _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s will no longer automatically start.\n", workspace.Name) | ||
| return nil | ||
| }, | ||
| } | ||
| } | ||
| var errInvalidScheduleFormat = xerrors.New("Schedule must be in the format Mon-Fri 09:00AM America/Chicago") | ||
| var errInvalidTimeFormat = xerrors.New("Start time must be in the format hh:mm[am|pm] or HH:MM") | ||
| var errUnsupportedTimezone = xerrors.New("The location you provided looks like a timezone. Check https://ipinfo.io for your location.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Nit: Go Errors should be lower case. But technically these are help/documentation, so I understand the use of proper sentence structure. (This isnon-blocking as I think the right approach might require some more think-through, but writing down my thoughts.) I have one suggestion, that could have some benefits down the line if we e.g. want to translate the CLI, etc: funcFormatCliError(errerror)string {switcherr {caseerrInvalidScheduleFormat:return"Schedule ..."// ...}} We could optionally have a separate Or perhaps rather we could have: typecliErrorstruct{errstringcode clierror.Code}funcnewCliError(code clierror.Code,estring)errorvarerrInvalidScheduleFormat=newCliError(clierror.InvalidSchedule,"invalid schedule format") Where we could keep an enum of cli error messages in e.g. a separate package. Just ideas of the top of my head. We could also do something simpler like uppercasing the first letter of the error when we print it. I think it'd be helpful if error messages looked consistent across the CLI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We have something very similar inside another codebase which shall not be named ;-) | ||
| // parseCLISchedule parses a schedule in the format HH:MM{AM|PM} [DOW] [LOCATION] | ||
| func parseCLISchedule(parts ...string) (*schedule.Schedule, error) { | ||
| // If the user was careful and quoted the schedule, un-quote it. | ||
| // In the case that only time was specified, this will be a no-op. | ||
| if len(parts) == 1 { | ||
| parts = strings.Fields(parts[0]) | ||
| } | ||
| var loc *time.Location | ||
| dayOfWeek := "*" | ||
| t, err := parseTime(parts[0]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| hour, minute := t.Hour(), t.Minute() | ||
| // Any additional parts get ignored. | ||
| switch len(parts) { | ||
| case 3: | ||
| dayOfWeek = parts[1] | ||
| loc, err = time.LoadLocation(parts[2]) | ||
| if err != nil { | ||
| _, err = time.Parse("MST", parts[2]) | ||
| if err == nil { | ||
| return nil, errUnsupportedTimezone | ||
| } | ||
| return nil, xerrors.Errorf("Invalid timezone %q specified: a valid IANA timezone is required", parts[2]) | ||
| } | ||
| case 2: | ||
| // Did they provide day-of-week or location? | ||
| if maybeLoc, err := time.LoadLocation(parts[1]); err != nil { | ||
| // Assume day-of-week. | ||
| dayOfWeek = parts[1] | ||
| } else { | ||
| loc = maybeLoc | ||
| } | ||
| case 1: // already handled | ||
| default: | ||
| return nil, errInvalidScheduleFormat | ||
| } | ||
| // If location was not specified, attempt to automatically determine it as a last resort. | ||
| if loc == nil { | ||
| loc, err = tz.TimezoneIANA() | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("Could not automatically determine your timezone") | ||
| } | ||
| } | ||
| sched, err := schedule.Weekly(fmt.Sprintf( | ||
| "CRON_TZ=%s %d %d * * %s", | ||
| loc.String(), | ||
| minute, | ||
| hour, | ||
| dayOfWeek, | ||
| )) | ||
| if err != nil { | ||
| // This will either be an invalid dayOfWeek or an invalid timezone. | ||
| return nil, xerrors.Errorf("Invalid schedule: %w", err) | ||
| } | ||
| return sched, nil | ||
| } | ||
| func parseTime(s string) (time.Time, error) { | ||
| // Try a number of possible layouts. | ||
| for _, layout := range []string{ | ||
| time.Kitchen, // 03:04PM | ||
| "03:04pm", | ||
| "3:04PM", | ||
| "3:04pm", | ||
| "15:04", | ||
| "1504", | ||
| "03PM", | ||
| "03pm", | ||
| "3PM", | ||
| "3pm", | ||
| } { | ||
| t, err := time.Parse(layout, s) | ||
| if err == nil { | ||
| return t, nil | ||
| } | ||
| } | ||
| return time.Time{}, errInvalidTimeFormat | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| package cli | ||
| import ( | ||
| "testing" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
| //nolint:paralleltest // t.Setenv | ||
| func TestParseCLISchedule(t *testing.T) { | ||
| for _, testCase := range []struct { | ||
| name string | ||
| input []string | ||
| expectedSchedule string | ||
| expectedError string | ||
| tzEnv string | ||
| }{ | ||
| { | ||
| name: "TimeAndDayOfWeekAndLocation", | ||
| input: []string{"09:00AM", "Sun-Sat", "America/Chicago"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat", | ||
| tzEnv: "UTC", | ||
| }, | ||
| { | ||
| name: "TimeOfDay24HourAndDayOfWeekAndLocation", | ||
| input: []string{"09:00", "Sun-Sat", "America/Chicago"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat", | ||
| tzEnv: "UTC", | ||
| }, | ||
| { | ||
| name: "TimeOfDay24HourAndDayOfWeekAndLocationButItsAllQuoted", | ||
| input: []string{"09:00 Sun-Sat America/Chicago"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat", | ||
| tzEnv: "UTC", | ||
| }, | ||
| { | ||
| name: "TimeOfDayOnly", | ||
| input: []string{"09:00AM"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *", | ||
| tzEnv: "America/Chicago", | ||
| }, | ||
| { | ||
| name: "Time24Military", | ||
| input: []string{"0900"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *", | ||
| tzEnv: "America/Chicago", | ||
| }, | ||
| { | ||
| name: "DayOfWeekAndTime", | ||
| input: []string{"09:00AM", "Sun-Sat"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat", | ||
| tzEnv: "America/Chicago", | ||
| }, | ||
| { | ||
| name: "TimeAndLocation", | ||
| input: []string{"09:00AM", "America/Chicago"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *", | ||
| tzEnv: "UTC", | ||
| }, | ||
| { | ||
| name: "LazyTime", | ||
| input: []string{"9am", "America/Chicago"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *", | ||
| tzEnv: "UTC", | ||
| }, | ||
| { | ||
| name: "ZeroPrefixedLazyTime", | ||
| input: []string{"09am", "America/Chicago"}, | ||
| expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *", | ||
| tzEnv: "UTC", | ||
| }, | ||
| { | ||
| name: "InvalidTime", | ||
| input: []string{"nine"}, | ||
| expectedError: errInvalidTimeFormat.Error(), | ||
| }, | ||
| { | ||
| name: "DayOfWeekAndInvalidTime", | ||
| input: []string{"nine", "Sun-Sat"}, | ||
| expectedError: errInvalidTimeFormat.Error(), | ||
| }, | ||
| { | ||
| name: "InvalidTimeAndLocation", | ||
| input: []string{"nine", "America/Chicago"}, | ||
| expectedError: errInvalidTimeFormat.Error(), | ||
| }, | ||
| { | ||
| name: "DayOfWeekAndInvalidTimeAndLocation", | ||
| input: []string{"nine", "Sun-Sat", "America/Chicago"}, | ||
| expectedError: errInvalidTimeFormat.Error(), | ||
| }, | ||
| { | ||
| name: "TimezoneProvidedInsteadOfLocation", | ||
| input: []string{"09:00AM", "Sun-Sat", "CST"}, | ||
| expectedError: errUnsupportedTimezone.Error(), | ||
| }, | ||
Comment on lines +92 to +96 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @Emyrk this one just for you :-) | ||
| { | ||
| name: "WhoKnows", | ||
| input: []string{"Time", "is", "a", "human", "construct"}, | ||
| expectedError: errInvalidTimeFormat.Error(), | ||
| }, | ||
| } { | ||
| testCase := testCase | ||
| //nolint:paralleltest // t.Setenv | ||
| t.Run(testCase.name, func(t *testing.T) { | ||
| t.Setenv("TZ", testCase.tzEnv) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Review: Doing | ||
| actualSchedule, actualError := parseCLISchedule(testCase.input...) | ||
| if testCase.expectedError != "" { | ||
| assert.Nil(t, actualSchedule) | ||
| assert.ErrorContains(t, actualError, testCase.expectedError) | ||
| return | ||
| } | ||
| assert.NoError(t, actualError) | ||
| if assert.NotEmpty(t, actualSchedule) { | ||
| assert.Equal(t, testCase.expectedSchedule, actualSchedule.String()) | ||
| } | ||
| }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. These are some beautiful tests! ❤️ | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading.Please reload this page.