make schedule format easier to parse

coder · johnstcn · Jun 13, 2022 · Jun 9, 2022 · Jun 10, 2022 · Jun 10, 2022
commit 0884e0ccc43819449d36991714c26be988a992ee
diff --git a/cli/autostart.go b/cli/autostart.go
@@ -15,12 +15,12 @@ import (
 )
 
 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: [day-of-week] start-time [location].
-  * Start-time (required) is accepted either in 12-hour (hh:mm{am|pm}) format, or 24-hour format {hh:mm}.
+When enabling autostart, enter a schedule in the format: start-time [day-of-week] [timezone].
+  * 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.
+  * Timezone (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.
 `
@@ -85,8 +85,8 @@ func autostartShow() *cobra.Command {
 
 func autostartSet() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:  "set <workspace_name> ",
-		Args: cobra.MinimumNArgs(2),
+		Use:  "set <workspace_name> <start-time> [day-of-week] [timezone]",
+		Args: cobra.RangeArgs(2, 4),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			client, err := createClient(cmd)
 			if err != nil {
@@ -112,10 +112,10 @@ func autostartSet() *cobra.Command {
 
 			schedNext := sched.Next(time.Now())
 			_, _ = fmt.Fprintf(cmd.OutOrStdout(),
-				"%s will automatically start %s at %s (%s)\n",
+				"%s will automatically start at %s %s (%s)\n",
 				workspace.Name,
-				sched.DaysOfWeek(),
 				schedNext.In(sched.Location()).Format(time.Kitchen),
+				sched.DaysOfWeek(),
 				sched.Location().String(),
 			)
 			return nil
@@ -157,66 +157,53 @@ func autostartUnset() *cobra.Command {
 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")
 
-// parseCLISchedule parses a schedule in the format Mon-Fri 09:00AM America/Chicago.
+// 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])
 	}
-	timezone := ""
+	var loc *time.Location
 	dayOfWeek := "*"
-	var hour, minute int
+	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 1:
-		t, err := parseTime(parts[0])
+	case 3:
+		dayOfWeek = parts[1]
+		loc, err = time.LoadLocation(parts[2])
 		if err != nil {
-			return nil, err
+			return nil, xerrors.Errorf("Invalid timezone %q specified: a valid IANA timezone is required", parts[2])
 		}
-		hour, minute = t.Hour(), t.Minute()
 	case 2:
-		if !strings.Contains(parts[0], ":") {
-			// DOW + Time
-			t, err := parseTime(parts[1])
-			if err != nil {
-				return nil, err
-			}
-			hour, minute = t.Hour(), t.Minute()
-			dayOfWeek = parts[0]
+		// 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 {
-			// Time + TZ
-			t, err := parseTime(parts[0])
-			if err != nil {
-				return nil, err
-			}
-			hour, minute = t.Hour(), t.Minute()
-			timezone = parts[1]
-		}
-	case 3:
-		// DOW + Time + TZ
-		t, err := parseTime(parts[1])
-		if err != nil {
-			return nil, err
+			loc = maybeLoc
 		}
-		hour, minute = t.Hour(), t.Minute()
-		dayOfWeek = parts[0]
-		timezone = parts[2]
+	case 1: // already handled
 	default:
 		return nil, errInvalidScheduleFormat
 	}
 
-	// If timezone was not specified, attempt to automatically determine it as a last resort.
-	if timezone == "" {
-		loc, err := tz.TimezoneIANA()
+	// 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.")
+			return nil, xerrors.Errorf("Could not automatically determine your timezone")
 		}
-		timezone = loc.String()
 	}
 
 	sched, err := schedule.Weekly(fmt.Sprintf(
 		"CRON_TZ=%s %d %d * * %s",
-		timezone,
+		loc.String(),
 		minute,
 		hour,
 		dayOfWeek,

diff --git a/cli/autostart_internal_test.go b/cli/autostart_internal_test.go
@@ -16,14 +16,20 @@ func TestParseCLISchedule(t *testing.T) {
 		tzEnv            string
 	}{
 		{
-			name:             "DefaultSchedule",
-			input:            []string{"Sun-Sat", "09:00AM", "America/Chicago"},
+			name:             "TimeAndDayOfWeekAndLocation",
+			input:            []string{"09:00AM", "Sun-Sat", "America/Chicago"},
 			expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat",
 			tzEnv:            "UTC",
 		},
 		{
-			name:             "DefaultSchedule24Hour",
-			input:            []string{"Sun-Sat", "09:00", "America/Chicago"},
+			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",
 		},
@@ -35,7 +41,7 @@ func TestParseCLISchedule(t *testing.T) {
 		},
 		{
 			name:             "DayOfWeekAndTime",
-			input:            []string{"Sun-Sat", "09:00AM"},
+			input:            []string{"09:00AM", "Sun-Sat"},
 			expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat",
 			tzEnv:            "America/Chicago",
 		},
@@ -52,7 +58,7 @@ func TestParseCLISchedule(t *testing.T) {
 		},
 		{
 			name:          "DayOfWeekAndInvalidTime",
-			input:         []string{"Sun-Sat", "9am"},
+			input:         []string{"9am", "Sun-Sat"},
 			expectedError: errInvalidTimeFormat.Error(),
 		},
 		{
@@ -62,13 +68,13 @@ func TestParseCLISchedule(t *testing.T) {
 		},
 		{
 			name:          "DayOfWeekAndInvalidTimeAndLocation",
-			input:         []string{"Sun-Sat", "9am", "America/Chicago"},
+			input:         []string{"9am", "Sun-Sat", "America/Chicago"},
 			expectedError: errInvalidTimeFormat.Error(),
 		},
 		{
 			name:          "WhoKnows",
 			input:         []string{"Time", "is", "a", "human", "construct"},
-			expectedError: errInvalidScheduleFormat.Error(),
+			expectedError: errInvalidTimeFormat.Error(),
 		},
 	} {
 		testCase := testCase

diff --git a/cli/autostart_test.go b/cli/autostart_test.go
@@ -60,7 +60,7 @@ func TestAutostart(t *testing.T) {
 			project   = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
 			workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID)
 			tz        = "Europe/Dublin"
-			cmdArgs   = []string{"autostart", "set", workspace.Name, "Mon-Fri", "9:30AM", tz}
+			cmdArgs   = []string{"autostart", "set", workspace.Name, "9:30AM", "Mon-Fri", tz}
 			sched     = "CRON_TZ=Europe/Dublin 30 9 * * Mon-Fri"
 			stdoutBuf = &bytes.Buffer{}
 		)
@@ -71,7 +71,7 @@ func TestAutostart(t *testing.T) {
 
 		err := cmd.Execute()
 		require.NoError(t, err, "unexpected error")
-		require.Contains(t, stdoutBuf.String(), "will automatically start Mon-Fri at 9:30AM (Europe/Dublin)", "unexpected output")
+		require.Contains(t, stdoutBuf.String(), "will automatically start at 9:30AM Mon-Fri (Europe/Dublin)", "unexpected output")
 
 		// Ensure autostart schedule updated
 		updated, err := client.Workspace(ctx, workspace.ID)
@@ -154,5 +154,5 @@ func TestAutostartSetDefaultSchedule(t *testing.T) {
 	updated, err := client.Workspace(ctx, workspace.ID)
 	require.NoError(t, err, "fetch updated workspace")
 	require.Equal(t, expectedSchedule, *updated.AutostartSchedule, "expected default autostart schedule")
-	require.Contains(t, stdoutBuf.String(), "will automatically start daily at 9:30AM (UTC)")
+	require.Contains(t, stdoutBuf.String(), "will automatically start at 9:30AM daily (UTC)")
 }