cli: streamline autostart ux #2251

johnstcn · 2022-06-10T16:56:45Z

This PR makes the following changes:

autostart enable|disable => autostart set|unset
autostart enable now accepts a more natual schedule format: <time> <days-of-week> <location>
autostart show now shows configured timezone
🎉 automatic timezone detection across mac, windows, linux 🎉

…-unset

johnstcn · 2022-06-10T18:10:15Z

coderd/util/tz/tz_darwin.go

+func TimezoneIANA() (*time.Location, error) {
+	if tzEnv, found := os.LookupEnv("TZ"); found {
+		if tzEnv == "" {
+			return time.UTC, nil
+		}
+		loc, err := time.LoadLocation(tzEnv)
+		if err != nil {
+			return nil, xerrors.Errorf("load location from TZ env: %w", err)
+		}
+		return loc, nil
+	}
+
+	lp, err := filepath.EvalSymlinks(etcLocaltime)
+	if err != nil {
+		return nil, xerrors.Errorf("read location of %s: %w", etcLocaltime, err)
+	}
+
+	// On Darwin, /var/db/timezone/zoneinfo is also a symlink
+	realZoneInfoPath, err := filepath.EvalSymlinks(zoneInfoPath)
+	if err != nil {
+		return nil, xerrors.Errorf("read location of %s: %w", zoneInfoPath, err)
+	}
+
+	stripped := strings.Replace(lp, realZoneInfoPath, "", -1)
+	stripped = strings.TrimPrefix(stripped, string(filepath.Separator))
+	loc, err := time.LoadLocation(stripped)
+	if err != nil {
+		return nil, xerrors.Errorf("invalid location %q guessed from %s: %w", stripped, lp, err)
+	}
+	return loc, nil
+}


Review: at a glance, the Darwin and Linux versions of these functions seems identical. Be assured they are not.

johnstcn · 2022-06-10T18:11:24Z

coderd/util/tz/tz_windows.go

+func TimezoneIANA() (*time.Location, error) {
+	if tzEnv, found := os.LookupEnv("TZ"); found {
+		if tzEnv == "" {
+			return time.UTC, nil
+		}
+		loc, err := time.LoadLocation(tzEnv)
+		if err != nil {
+			return nil, xerrors.Errorf("load location from TZ env: %w", err)
+		}
+		return loc, nil
+	}
+
+	// https://superuser.com/a/1584968
+	cmd := exec.Command("powershell.exe", "-NoLogo", "-NoProfile", "-NonInteractive")
+	stdin, err := cmd.StdinPipe()
+	if err != nil {
+		return nil, xerrors.Errorf("run powershell: %w", err)
+	}
+
+	done := make(chan struct{})
+	go func() {
+		defer stdin.Close()
+		defer close(done)
+		_, _ = fmt.Fprintln(stdin, cmdTimezone)
+	}()
+
+	<-done
+
+	outBytes, err := cmd.CombinedOutput()
+	if err != nil {
+		return nil, xerrors.Errorf("execute powershell command %q: %w", cmdTimezone, err)
+	}
+
+	outLines := strings.Split(string(outBytes), "\n")
+	if len(outLines) < 2 {
+		return nil, xerrors.Errorf("unexpected output from powershell command %q: %q", cmdTimezone, outLines)
+	}
+	// What we want is the second line of output
+	locStr := strings.TrimSpace(outLines[1])
+	loc, err := time.LoadLocation(locStr)
+	if err != nil {
+		return nil, xerrors.Errorf("invalid location %q from powershell: %w", locStr, err)
+	}
+
+	return loc, nil
+}


Review: just don't stare too closely at this

johnstcn · 2022-06-10T18:14:11Z

coderd/autobuild/schedule/schedule.go

@@ -74,7 +75,8 @@ func Weekly(raw string) (*Schedule, error) {
 }

 // Schedule represents a cron schedule.
-// It's essentially a thin wrapper for robfig/cron/v3 that implements Stringer.


Review: This wrapper is no longer thin.

johnstcn · 2022-06-10T18:14:51Z

coderd/autobuild/schedule/schedule.go

+func (s Schedule) DaysOfWeek() string {
+	dow := strings.Fields(s.cronStr)[4]
+	if dow == "*" {
+		return "daily"
+	}
+	for _, weekday := range []time.Weekday{
+		time.Sunday,
+		time.Monday,
+		time.Tuesday,
+		time.Wednesday,
+		time.Thursday,
+		time.Friday,
+		time.Saturday,
+	} {
+		dow = strings.Replace(dow, fmt.Sprintf("%d", weekday), weekday.String()[:3], 1)
+	}
+	return dow
+}
+


Review: For display purposes, this just replaces the day-of-week numbers with their abbreviated equivalents.

johnstcn · 2022-06-10T18:15:46Z

cli/autostart_internal_test.go

+		testCase := testCase
+		//nolint:paralleltest // t.Setenv
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Setenv("TZ", testCase.tzEnv)


Review: Doing setenv here to validate that the timezone specified in the schedule is used

johnstcn · 2022-06-10T19:51:12Z

cli/autostart_internal_test.go

+		{
+			name:          "TimezoneProvidedInsteadOfLocation",
+			input:         []string{"09:00AM", "Sun-Sat", "CST"},
+			expectedError: errUnsupportedTimezone.Error(),
+		},


@Emyrk this one just for you :-)

cli/autostart.go

mafredri

A few suggestions and questions, but in general looks good! 👍🏻

cli/autostart.go

mafredri · 2022-06-13T10:44:45Z

cli/autostart.go

+    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.


This help is ❤️.

Yes, this is excellent.

mafredri · 2022-06-13T10:48:05Z

cli/autostart.go

+			_, _ = fmt.Fprintf(cmd.OutOrStdout(),
+				"%s will automatically start at %s %s (%s)\n",
+				workspace.Name,
+				schedNext.In(sched.Location()).Format(time.Kitchen),


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?

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.

I think preference is very location-dependent. It would be unfortunate to add more state for am/pm.

mafredri · 2022-06-13T11:11:18Z

cli/autostart.go

+
+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.")


Nit: Go Errors should be lower case.

But technically these are help/documentation, so I understand the use of proper sentence structure.

(This is non-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:

func FormatCliError(err error) string { switch err { case errInvalidScheduleFormat: return "Schedule ..." // ... } }

We could optionally have a separate type cliError struct{} type which we print in this special way.

Or perhaps rather we could have:

type cliError struct{ err string code clierror.Code } func newCliError(code clierror.Code, e string) error var errInvalidScheduleFormat = 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.

We have something very similar inside another codebase which shall not be named ;-)
But yes, this needs more noodling.

cli/autostart.go

mafredri · 2022-06-13T11:23:13Z

cli/autostart_internal_test.go

+			if assert.NotEmpty(t, actualSchedule) {
+				assert.Equal(t, testCase.expectedSchedule, actualSchedule.String())
+			}
+		})


These are some beautiful tests! ❤️

coderd/util/tz/tz_windows.go

coderd/util/tz/tz_linux.go

ammario · 2022-06-13T20:27:37Z

cli/autostart.go

+    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.


Yes, this is excellent.

ammario · 2022-06-13T20:29:44Z

cli/autostart.go

+			_, _ = fmt.Fprintf(cmd.OutOrStdout(),
+				"%s will automatically start at %s %s (%s)\n",
+				workspace.Name,
+				schedNext.In(sched.Location()).Format(time.Kitchen),


I think preference is very location-dependent. It would be unfortunate to add more state for am/pm.

johnstcn and others added 9 commits June 9, 2022 21:38

wip

7c7a290

Merge remote-tracking branch 'origin/main' into cj/1647/autostart-set…

8983ef9

…-unset

fix some unit tests

84f1a12

fix more unit tests

bad0ac2

more fixes

68187d8

fix handling of powershell output

bcb1e8c

update CLI output

2556ba2

make schedule format easier to parse

0884e0c

improve time parsing

377b708

johnstcn requested a review from a team June 10, 2022 17:21

johnstcn marked this pull request as ready for review June 10, 2022 17:21

johnstcn self-assigned this Jun 10, 2022

johnstcn requested a review from ammario June 10, 2022 18:08

johnstcn commented Jun 10, 2022

View reviewed changes

cli/autostart.go Outdated Show resolved Hide resolved

mafredri approved these changes Jun 13, 2022

View reviewed changes

ammario approved these changes Jun 13, 2022

View reviewed changes

johnstcn added 2 commits June 13, 2022 20:39

Address PR comments

25ef437

fixup! Address PR comments

4bab79f

johnstcn enabled auto-merge (squash) June 13, 2022 20:44

johnstcn and others added 3 commits June 13, 2022 21:48

a pox upon you darwin

e6921a7

remove unused import on windows

9e76791

add missing time formats

6a82f77

johnstcn disabled auto-merge June 13, 2022 21:00

johnstcn merged commit 0a949aa into main Jun 13, 2022

johnstcn deleted the cj/1647/autostart-set-unset branch June 13, 2022 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: streamline autostart ux #2251

cli: streamline autostart ux #2251

johnstcn commented Jun 10, 2022 •

edited

Loading

johnstcn Jun 10, 2022

johnstcn Jun 10, 2022

johnstcn Jun 10, 2022

johnstcn Jun 10, 2022

johnstcn Jun 10, 2022

johnstcn Jun 10, 2022

mafredri left a comment

mafredri Jun 13, 2022

ammario Jun 13, 2022

mafredri Jun 13, 2022

johnstcn Jun 13, 2022 •

edited

Loading

ammario Jun 13, 2022

mafredri Jun 13, 2022

johnstcn Jun 13, 2022

mafredri Jun 13, 2022

ammario Jun 13, 2022

ammario Jun 13, 2022

cli: streamline autostart ux #2251

cli: streamline autostart ux #2251

Conversation

johnstcn commented Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn commented Jun 10, 2022 •

edited

Loading

johnstcn Jun 13, 2022 •

edited

Loading