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 all commits
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
157 changes: 123 additions & 34 deletions cli/autostart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,41 @@ package cli

import (
"fmt"
"os"
"strings"
"time"

"github.com/spf13/cobra"
"golang.org/x/xerrors"

"github.com/coder/coder/coderd/autobuild/schedule"
"github.com/coder/coder/coderd/util/ptr"
"github.com/coder/coder/coderd/util/tz"
"github.com/coder/coder/codersdk"
)

const autostartDescriptionLong = `To have your workspace build automatically at a regular time you can enable autostart.
When enabling autostart, provide the minute, hour, and day(s) of week.
The default schedule is at 09:00 in your local timezone (TZ env, UTC by default).
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)
* 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.

`

func autostart() *cobra.Command {
autostartCmd := &cobra.Command{
Annotations: workspaceCommand,
Use: "autostart enable <workspace>",
Use: "autostart set <workspace> <start-time> [day-of-week] [location]",
Short: "schedule a workspace to automatically start at a regular time",
Long: autostartDescriptionLong,
Example: "coder autostart enable my-workspace --minute 30 --hour 9 --days 1-5 --tz Europe/Dublin",
Example: "coder autostart set my-workspace 9:30AM Mon-Fri Europe/Dublin",
}

autostartCmd.AddCommand(autostartShow())
autostartCmd.AddCommand(autostartEnable())
autostartCmd.AddCommand(autostartDisable())
autostartCmd.AddCommand(autostartSet())
autostartCmd.AddCommand(autostartUnset())

return autostartCmd
}
Expand Down Expand Up @@ -60,13 +69,12 @@ func autostartShow() *cobra.Command {
}

next := validSchedule.Next(time.Now())
loc, _ := time.LoadLocation(validSchedule.Timezone())

_, _ = fmt.Fprintf(cmd.OutOrStdout(),
"schedule: %s\ntimezone: %s\nnext: %s\n",
validSchedule.Cron(),
validSchedule.Timezone(),
next.In(loc),
validSchedule.Location(),
next.In(validSchedule.Location()),
)

return nil
Expand All @@ -75,23 +83,17 @@ func autostartShow() *cobra.Command {
return cmd
}

func autostartEnable() *cobra.Command {
// yes some of these are technically numbers but the cron library will do that work
var autostartMinute string
var autostartHour string
var autostartDayOfWeek string
var autostartTimezone string
func autostartSet() *cobra.Command {
cmd := &cobra.Command{
Use: "enable <workspace_name> <schedule>",
Args: cobra.ExactArgs(1),
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)
if err != nil {
return err
}

spec := fmt.Sprintf("CRON_TZ=%s %s %s * * %s", autostartTimezone, autostartMinute, autostartHour, autostartDayOfWeek)
validSchedule, err := schedule.Weekly(spec)
sched, err := parseCLISchedule(args[1:]...)
if err != nil {
return err
}
Expand All @@ -102,32 +104,30 @@ func autostartEnable() *cobra.Command {
}

err = client.UpdateWorkspaceAutostart(cmd.Context(), workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{
Schedule: &spec,
Schedule: ptr.Ref(sched.String()),
})
if err != nil {
return err
}

_, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nThe %s workspace will automatically start at %s.\n\n", workspace.Name, validSchedule.Next(time.Now()))

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

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?

Copy link
Member Author

@johnstcn johnstcn Jun 13, 2022 •
edited
Loading

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.

Copy link
Member

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.

sched.DaysOfWeek(),
sched.Location().String(),
)
return nil
},
}

cmd.Flags().StringVar(&autostartMinute, "minute", "0", "autostart minute")
cmd.Flags().StringVar(&autostartHour, "hour", "9", "autostart hour")
cmd.Flags().StringVar(&autostartDayOfWeek, "days", "1-5", "autostart day(s) of week")
tzEnv := os.Getenv("TZ")
if tzEnv == "" {
tzEnv = "UTC"
}
cmd.Flags().StringVar(&autostartTimezone, "tz", tzEnv, "autostart timezone")
return cmd
}

func autostartDisable() *cobra.Command {
func autostartUnset() *cobra.Command {
return &cobra.Command{
Use: "disable <workspace_name>",
Use: "unset <workspace_name>",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
client, err := createClient(cmd)
Expand All @@ -147,9 +147,98 @@ func autostartDisable() *cobra.Command {
return err
}

_, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nThe %s workspace will no longer automatically start.\n\n", workspace.Name)
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s will no longer automatically start.\n", workspace.Name)

return nil
},
}
}

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) {
// 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])
}
var loc *time.Location
dayOfWeek := "*"
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 3:
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:
// 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 {
loc = maybeLoc
}
case 1: // already handled
default:
return nil, errInvalidScheduleFormat
}

// 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")
}
}

sched, err := schedule.Weekly(fmt.Sprintf(
"CRON_TZ=%s %d %d * * %s",
loc.String(),
minute,
hour,
dayOfWeek,
))
if err != nil {
// This will either be an invalid dayOfWeek or an invalid timezone.
return nil, xerrors.Errorf("Invalid schedule: %w", err)
}

return sched, nil
}

func parseTime(s string) (time.Time, error) {
// Try a number of possible layouts.
for _, layout := range []string{
time.Kitchen, // 03:04PM
"03:04pm",
"3:04PM",
"3:04pm",
"15:04",
"1504",
"03PM",
"03pm",
"3PM",
"3pm",
} {
t, err := time.Parse(layout, s)
if err == nil {
return t, nil
}
}
return time.Time{}, errInvalidTimeFormat
}
119 changes: 119 additions & 0 deletions cli/autostart_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package cli

import (
"testing"

"github.com/stretchr/testify/assert"
)

//nolint:paralleltest // t.Setenv
func TestParseCLISchedule(t *testing.T) {
for _, testCase := range []struct {
name string
input []string
expectedSchedule string
expectedError string
tzEnv string
}{
{
name: "TimeAndDayOfWeekAndLocation",
input: []string{"09:00AM", "Sun-Sat", "America/Chicago"},
expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat",
tzEnv: "UTC",
},
{
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",
},
{
name: "TimeOfDayOnly",
input: []string{"09:00AM"},
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"},
expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * Sun-Sat",
tzEnv: "America/Chicago",
},
{
name: "TimeAndLocation",
input: []string{"09:00AM", "America/Chicago"},
expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *",
tzEnv: "UTC",
},
{
name: "LazyTime",
input: []string{"9am", "America/Chicago"},
expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *",
tzEnv: "UTC",
},
{
name: "ZeroPrefixedLazyTime",
input: []string{"09am", "America/Chicago"},
expectedSchedule: "CRON_TZ=America/Chicago 0 9 * * *",
tzEnv: "UTC",
},
{
name: "InvalidTime",
input: []string{"nine"},
expectedError: errInvalidTimeFormat.Error(),
},
{
name: "DayOfWeekAndInvalidTime",
input: []string{"nine", "Sun-Sat"},
expectedError: errInvalidTimeFormat.Error(),
},
{
name: "InvalidTimeAndLocation",
input: []string{"nine", "America/Chicago"},
expectedError: errInvalidTimeFormat.Error(),
},
{
name: "DayOfWeekAndInvalidTimeAndLocation",
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"},
expectedError: errInvalidTimeFormat.Error(),
},
} {
testCase := testCase
//nolint:paralleltest // t.Setenv
t.Run(testCase.name, func(t *testing.T) {
t.Setenv("TZ", testCase.tzEnv)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

actualSchedule, actualError := parseCLISchedule(testCase.input...)
if testCase.expectedError != "" {
assert.Nil(t, actualSchedule)
assert.ErrorContains(t, actualError, testCase.expectedError)
return
}
assert.NoError(t, actualError)
if assert.NotEmpty(t, actualSchedule) {
assert.Equal(t, testCase.expectedSchedule, actualSchedule.String())
}
})
Copy link
Member

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! ❤️

}
}
Loading