Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jun 13, 2022
Merged

cli: streamline autostart ux #2251

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7c7a290
wip
johnstcn Jun 9, 2022
8983ef9
Merge remote-tracking branch 'origin/main' into cj/1647/autostart-set…
johnstcn Jun 10, 2022
84f1a12
fix some unit tests
johnstcn Jun 10, 2022
bad0ac2
fix more unit tests
johnstcn Jun 10, 2022
68187d8
more fixes
johnstcn Jun 10, 2022
bcb1e8c
fix handling of powershell output
johnstcn Jun 10, 2022
2556ba2
update CLI output
johnstcn Jun 10, 2022
0884e0c
make schedule format easier to parse
johnstcn Jun 10, 2022
377b708
improve time parsing
johnstcn Jun 10, 2022
25ef437
Address PR comments
johnstcn Jun 13, 2022
4bab79f
fixup! Address PR comments
johnstcn Jun 13, 2022
e6921a7
a pox upon you darwin
johnstcn Jun 13, 2022
9e76791
remove unused import on windows
johnstcn Jun 13, 2022
6a82f77
add missing time formats
johnstcn Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
improve time parsing
  • Loading branch information
@johnstcn
johnstcn committed Jun 10, 2022
commit 377b708e4ad5146ae4c11c1515268a746b67f9dc
36 changes: 23 additions & 13 deletions cli/autostart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: start-time [day-of-week] [timezone].
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)
* Timezone (optional) must be a valid location in the IANA timezone database.
* 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This help is ❤️.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is excellent.

`
Expand Down Expand Up @@ -85,7 +85,7 @@ func autostartShow() *cobra.Command {

func autostartSet() *cobra.Command {
cmd := &cobra.Command{
Use: "set <workspace_name> <start-time> [day-of-week] [timezone]",
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)
Expand Down Expand Up @@ -156,6 +156,7 @@ 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")
var errUnsupportedTimezone = xerrors.New("The location you provided looks like a timezone. Check https://ipinfo.io for your location.")
Copy link
Member

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

Copy link
Member Author

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.


// parseCLISchedule parses a schedule in the format HH:MM{AM|PM} [DOW] [LOCATION]
func parseCLISchedule(parts ...string) (*schedule.Schedule, error) {
Expand All @@ -178,6 +179,10 @@ func parseCLISchedule(parts ...string) (*schedule.Schedule, error) {
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:
Expand Down Expand Up @@ -217,15 +222,20 @@ func parseCLISchedule(parts ...string) (*schedule.Schedule, error) {
}

func parseTime(s string) (time.Time, error) {
// Assume only time provided, HH:MM[AM|PM]
t, err := time.Parse(time.Kitchen, s)
if err == nil {
return t, nil
}
// Try 24-hour format without AM/PM suffix.
t, err = time.Parse("15:04", s)
if err != nil {
return time.Time{}, errInvalidTimeFormat
// Try a number of possible layouts.
for _, layout := range []string{
time.Kitchen, // 03:04PM
"15:04",
"1504",
"3pm",
"3PM",
"3:04pm",
"3:04PM",
} {
t, err := time.Parse(layout, s)
if err == nil {
return t, nil
}
}
return t, nil
return time.Time{}, errInvalidTimeFormat
}
19 changes: 15 additions & 4 deletions cli/autostart_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ func TestParseCLISchedule(t *testing.T) {
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"},
Expand All @@ -53,24 +59,29 @@ func TestParseCLISchedule(t *testing.T) {
},
{
name: "InvalidTime",
input: []string{"9am"},
input: []string{"nine"},
expectedError: errInvalidTimeFormat.Error(),
},
{
name: "DayOfWeekAndInvalidTime",
input: []string{"9am", "Sun-Sat"},
input: []string{"nine", "Sun-Sat"},
expectedError: errInvalidTimeFormat.Error(),
},
{
name: "InvalidTimeAndLocation",
input: []string{"9:", "America/Chicago"},
input: []string{"nine", "America/Chicago"},
expectedError: errInvalidTimeFormat.Error(),
},
{
name: "DayOfWeekAndInvalidTimeAndLocation",
input: []string{"9am", "Sun-Sat", "America/Chicago"},
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
Copy link
Member Author

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 :-)

{
name: "WhoKnows",
input: []string{"Time", "is", "a", "human", "construct"},
Expand Down