- 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.
Conversation
| funcTimezoneIANA() (*time.Location,error) { | ||
| iftzEnv,found:=os.LookupEnv("TZ");found { | ||
| iftzEnv=="" { | ||
| returntime.UTC,nil | ||
| } | ||
| loc,err:=time.LoadLocation(tzEnv) | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("load location from TZ env: %w",err) | ||
| } | ||
| returnloc,nil | ||
| } | ||
| lp,err:=filepath.EvalSymlinks(etcLocaltime) | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("read location of %s: %w",etcLocaltime,err) | ||
| } | ||
| // On Darwin, /var/db/timezone/zoneinfo is also a symlink | ||
| realZoneInfoPath,err:=filepath.EvalSymlinks(zoneInfoPath) | ||
| iferr!=nil { | ||
| returnnil,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) | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("invalid location %q guessed from %s: %w",stripped,lp,err) | ||
| } | ||
| returnloc,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.
| funcTimezoneIANA() (*time.Location,error) { | ||
| iftzEnv,found:=os.LookupEnv("TZ");found { | ||
| iftzEnv=="" { | ||
| returntime.UTC,nil | ||
| } | ||
| loc,err:=time.LoadLocation(tzEnv) | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("load location from TZ env: %w",err) | ||
| } | ||
| returnloc,nil | ||
| } | ||
| // https://superuser.com/a/1584968 | ||
| cmd:=exec.Command("powershell.exe","-NoLogo","-NoProfile","-NonInteractive") | ||
| stdin,err:=cmd.StdinPipe() | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("run powershell: %w",err) | ||
| } | ||
| done:=make(chanstruct{}) | ||
| gofunc() { | ||
| deferstdin.Close() | ||
| deferclose(done) | ||
| _,_=fmt.Fprintln(stdin,cmdTimezone) | ||
| }() | ||
| <-done | ||
| outBytes,err:=cmd.CombinedOutput() | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("execute powershell command %q: %w",cmdTimezone,err) | ||
| } | ||
| outLines:=strings.Split(string(outBytes),"\n") | ||
| iflen(outLines)<2 { | ||
| returnnil,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) | ||
| iferr!=nil { | ||
| returnnil,xerrors.Errorf("invalid location %q from powershell: %w",locStr,err) | ||
| } | ||
| returnloc,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
| } | ||
| // 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 (sSchedule)DaysOfWeek()string { | ||
| dow:=strings.Fields(s.cronStr)[4] | ||
| ifdow=="*" { | ||
| 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) | ||
| } | ||
| returndow | ||
| } | ||
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.
| varerrInvalidScheduleFormat=xerrors.New("Schedule must be in the format Mon-Fri 09:00AM America/Chicago") | ||
| varerrInvalidTimeFormat=xerrors.New("Start time must be in the format hh:mm[am|pm] or HH:MM") | ||
| varerrUnsupportedTimezone=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.
| ifassert.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