- Notifications
You must be signed in to change notification settings - Fork928
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.
Conversation
coderd/util/tz/tz_darwin.go Outdated
| 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 | ||
| } |
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: at a glance, the Darwin and Linux versions of these functions seems identical. Be assured they are not.
coderd/util/tz/tz_windows.go Outdated
| 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 | ||
| } |
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: just don't stare too closely at this
| @@ -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. | |||
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: This wrapper is no longer thin.
| 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 | ||
| } | ||
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: For display purposes, this just replaces the day-of-week numbers with their abbreviated equivalents.
| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Review: Doingsetenv here to validate that the timezone specified in the schedule is used
| { | ||
| name: "TimezoneProvidedInsteadOfLocation", | ||
| input: []string{"09:00AM", "Sun-Sat", "CST"}, | ||
| expectedError: errUnsupportedTimezone.Error(), | ||
| }, |
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 this one just for you :-)
Uh oh!
There was an error while loading.Please reload this page.
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 few suggestions and questions, but in general looks good! 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
| 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 comment
The 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this is excellent.
| _, _ = 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 comment
The 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?
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.
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 comment
The 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.
| 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 comment
The 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 separatetype cliError struct{} type which we print in this special way.
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 comment
The 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 ;-)
But yes, this needs more noodling.
Uh oh!
There was an error while loading.Please reload this page.
| 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 comment
The 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.
Uh oh!
There was an error while loading.Please reload this page.
| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this is excellent.
| _, _ = 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 comment
The 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.
Uh oh!
There was an error while loading.Please reload this page.
This PR makes the following changes:
autostart enable|disable=>autostart set|unsetautostart enablenow accepts a more natual schedule format:<time> <days-of-week> <location>autostart shownow shows configured timezoneFixes#1647