From ce8cb505c3ca78e93b297d0b98e8d88a36fcb163 Mon Sep 17 00:00:00 2001 From: Tom McSweeney Date: Fri, 5 Aug 2016 12:32:21 -0700 Subject: [PATCH] Fixes #1126: Adds code to consistently apply CLI parameters when creating a task using snapctl --- cmd/snapctl/flags.go | 2 +- cmd/snapctl/task.go | 321 +++++++++++++++++++++--------- pkg/schedule/schedule.go | 2 - pkg/schedule/windowed_schedule.go | 4 + 4 files changed, 229 insertions(+), 100 deletions(-) diff --git a/cmd/snapctl/flags.go b/cmd/snapctl/flags.go index 1067d5974..03970673a 100644 --- a/cmd/snapctl/flags.go +++ b/cmd/snapctl/flags.go @@ -124,7 +124,7 @@ var ( } flTaskMaxFailures = cli.StringFlag{ Name: "max-failures", - Usage: "The number of consecutive failures before snap disable the task", + Usage: "The number of consecutive failures before snap disables the task", } // metric diff --git a/cmd/snapctl/task.go b/cmd/snapctl/task.go index 3013dca99..5b2df2496 100644 --- a/cmd/snapctl/task.go +++ b/cmd/snapctl/task.go @@ -101,8 +101,8 @@ func createTask(ctx *cli.Context) error { } func stringValToInt(val string) (int, error) { - // parse the input (string) value as an integer (log a Fatal - // error if we cannot parse the value as an integer) + // parse the input (string) as an integer value (and return that integer value + // to the caller or an error if the input value cannot be parsed as an integer) parsedField, err := strconv.Atoi(val) if err != nil { splitErr := strings.Split(err.Error(), ": ") @@ -114,14 +114,210 @@ func stringValToInt(val string) (int, error) { return parsedField, nil } +// Parses the command-line parameters (if any) and uses them to override the underlying +// sehedule for this task or to set a schedule for that task (if one is not already defined, +// as is the case when we're building a new task from a workflow manifest). +// +// Note; in this method, any of the following types of time windows can be specified: +// +// +---------------------------... (start with no stop and no duration; no end time for window) +// | +// start +// +// ...---------------------------+ (stop with no start and no duration; no start time for window) +// | +// stop +// +// +-----------------------------+ (start with a duration but no stop) +// | | +// |---------------------------->| +// start duration +// +// +-----------------------------+ (stop with a duration but no start) +// | | +// |<----------------------------| +// duration stop +// +// +-----------------------------+ (start and stop both specified) +// | | +// |<--------------------------->| +// start stop +// +// +-----------------------------+ (only duration specified, implies start is the current time) +// | | +// |---------------------------->| +// Now() duration +// +func (t *task) setWindowedSchedule(start *time.Time, stop *time.Time, duration *time.Duration) error { + // if there is an empty schedule already defined for this task, then set the + // type for that schedule to 'windowed' + if t.Schedule.Type == "" { + t.Schedule.Type = "windowed" + } else if t.Schedule.Type != "windowed" { + // else if the task's existing schedule is not a 'windowed' schedule, + // then return an error + return fmt.Errorf("Usage error (schedule type mismatch); cannot replace existing schedule of type '%v' with a new, 'windowed' schedule", t.Schedule.Type) + } + // if a duration was passed in, determine the start and stop times for our new + // 'windowed' schedule from the input parameters + if duration != nil { + // if start and stop were both defined, then return an error (since all three parameters cannot be used + // to define the 'windowed' schedule) + if start != nil && stop != nil { + return fmt.Errorf("Usage error (too many parameters); only two of the parameters that define the window (start time, stop time and duration) can be specified for a 'windowed' schedule") + } + // if start is set and stop is not then use duration to create stop + if start != nil && stop == nil { + newStop := start.Add(*duration) + stop = &newStop + } else if stop != nil && start == nil { + // else if stop is set and start is not then use duration to create start + newStart := stop.Add(*duration * -1) + start = &newStart + } else { + // else, the start and stop are both undefined but a duration was passed in, + // so use the current date/time (plus the 'createTaskNowPad' value) as the + // start date/time and construct a stop date/time from that start date/time + // and the duration + newStart := time.Now().Add(createTaskNowPad) + newStop := newStart.Add(*duration) + start = &newStart + stop = &newStop + } + // use the new start/stop values determined, above, to set the start/stop date/times for the + // schedule associated with this task + t.Schedule.StartTime = start + t.Schedule.StopTime = stop + } else if start != nil && stop == nil { + // else if only the start date/time was specified, we will use it to replace + // the current schedule's start date/time + t.Schedule.StartTime = start + } else if start == nil && stop != nil { + // otherwise, if only the stop date/time was specified, we will use it to replace the + // current schedule's stop date/time + t.Schedule.StopTime = stop + } + // and return a nil error (indicating success) + return nil +} + +// parse the command-line options and use them to setup a new schedule for this task +func (t *task) setScheduleFromCliOptions(ctx *cli.Context) error { + // check the start, stop, and duration values to see if we're looking at a windowed schedule (or not) + // first, get the parameters that define the windowed schedule + start := mergeDateTime( + strings.ToUpper(ctx.String("start-time")), + strings.ToUpper(ctx.String("start-date")), + ) + stop := mergeDateTime( + strings.ToUpper(ctx.String("stop-time")), + strings.ToUpper(ctx.String("stop-date")), + ) + // Grab the duration string (if one was passed in) and parse it + durationStr := ctx.String("duration") + var duration *time.Duration + if ctx.IsSet("duration") || durationStr != "" { + d, err := time.ParseDuration(durationStr) + if err != nil { + return fmt.Errorf("Usage error (bad duration format); %v", err) + } + duration = &d + } + // Grab the interval for the schedule (if one was provided); we will use this + // string to determine the type of schedule ('simple' or 'cron') and to update + // the interval of the existing schedule (note that an interval value was + // not passed in and there is no interval defined for the schedule associated + // with this task, it's an error) + interval := ctx.String("interval") + if !ctx.IsSet("interval") && interval == "" && t.Schedule.Interval == "" { + return fmt.Errorf("Usage error (missing interval value); when constructing a new task schedule an interval must be provided") + } + // if a start, stop, or duration value was provided, then the CLI options are being used to + // specify a new 'windowed' schedule for this task, so use them to replace the 'windowed' schedule + // in the current task (if one exists); note that it is an error to try to replace an existing + // sechedule with a new schedule of a different type, so throw an error if that is the case + if start != nil || stop != nil || duration != nil { + return t.setWindowedSchedule(start, stop, duration) + } + + // otherwise, we're looking at either a 'simple' schedule or a 'cron' schedule; + // the type we're looking at is determined by the interval value passed in via the CLI + if interval != "" { + isCron := false + _, err := time.ParseDuration(interval) + if err != nil { + // try interpreting interval as cron entry + _, e := cron.Parse(interval) + if e != nil { + return fmt.Errorf("Usage error (bad interval value): cannot parse interval value '%v' either as a duration or a cron entry", interval) + } + isCron = true + } + // set the schedule type based on the 'isCron' flag, which is set above (note it is + // an error to attempt to replace an existing schedule with a schedule of a different + // type, so throw an error if that is the case) + schedType := "simple" + if isCron { + // make sure the current schedule type (if there is one) matches + if t.Schedule.Type != "" && t.Schedule.Type != "cron" { + return fmt.Errorf("Usage error; cannot replace existing schedule of type '%v' with a new, 'cron' schedule", t.Schedule.Type) + } + schedType = "cron" + } else if t.Schedule.Type != "" && t.Schedule.Type != "simple" { + // make sure the current schedule type (if there is one) matches + return fmt.Errorf("Usage error; cannot replace existing schedule of type '%v' with a new, 'simple' schedule", t.Schedule.Type) + } + // set the type (if it's not already set) and interval for the schedule associated with this task + if t.Schedule.Type == "" { + t.Schedule.Type = schedType + } + t.Schedule.Interval = interval + } + // return a nil error (indicating success) + return nil +} + +// merge the command-line options into the current task +func (t *task) mergeCliOptions(ctx *cli.Context) error { + // set the name of the task (if a 'name' was provided in the CLI options) + name := ctx.String("name") + if ctx.IsSet("name") || name != "" { + t.Name = name + } + // set the deadline of the task (if a 'deadline' was provided in the CLI options) + deadline := ctx.String("deadline") + if ctx.IsSet("deadline") || deadline != "" { + t.Deadline = deadline + } + // set the MaxFailures for the task (if a 'max-failures' value was provided in the CLI options) + maxFailuresStrVal := ctx.String("max-failures") + if ctx.IsSet("max-failures") || maxFailuresStrVal != "" { + maxFailures, err := stringValToInt(maxFailuresStrVal) + if err != nil { + return err + } + t.MaxFailures = maxFailures + } + // shouldn't ever happen, but... + if t.Version != 1 { + return fmt.Errorf("Invalid version provided while creating task") + } + // set the schedule for the task from the CLI options (and return the results + // of that method call, indicating whether or not an error was encountered while + // setting up that schedule) + return t.setScheduleFromCliOptions(ctx) +} + func createTaskUsingTaskManifest(ctx *cli.Context) error { + // get the task manifest file to use path := ctx.String("task-manifest") ext := filepath.Ext(path) file, e := ioutil.ReadFile(path) if e != nil { - return fmt.Errorf("File error [%s]- %v\n", ext, e) + return fmt.Errorf("File error [%s] - %v\n", ext, e) } + // create an empty task struct and unmarshal the contents of the file into that object t := task{} switch ext { case ".yaml", ".yml": @@ -138,11 +334,13 @@ func createTaskUsingTaskManifest(ctx *cli.Context) error { return fmt.Errorf("Unsupported file type %s\n", ext) } - t.Name = ctx.String("name") - if t.Version != 1 { - return fmt.Errorf("Invalid version provided") + // merge any CLI options specified by the user (if any) into the current task; + // if an error is encountered, return it + if err := t.mergeCliOptions(ctx); err != nil { + return err } + // and use the resulting struct to create a new task r := pClient.CreateTask(t.Schedule, t.Workflow, t.Name, t.Deadline, !ctx.IsSet("no-start"), t.MaxFailures) if r.Err != nil { @@ -162,18 +360,21 @@ func createTaskUsingTaskManifest(ctx *cli.Context) error { } func createTaskUsingWFManifest(ctx *cli.Context) error { - // Get the workflow + // Get the workflow manifest filename from the command-line path := ctx.String("workflow-manifest") ext := filepath.Ext(path) file, e := ioutil.ReadFile(path) - - if !ctx.IsSet("interval") && !ctx.IsSet("i") { - return fmt.Errorf("Workflow manifest requires interval to be set via flag.") - } if e != nil { - return fmt.Errorf("File error [%s]- %v\n", ext, e) + return fmt.Errorf("File error [%s] - %v\n", ext, e) } + // check to make sure that an interval was specified using the appropriate command-line flag + interval := ctx.String("interval") + if !ctx.IsSet("interval") && interval != "" { + return fmt.Errorf("Workflow manifest requires that an interval be set via a command-line flag.") + } + + // and unmarshal the contents of the workflow manifest file into a local workflow map var wf *wmap.WorkflowMap switch ext { case ".yaml", ".yml": @@ -189,95 +390,21 @@ func createTaskUsingWFManifest(ctx *cli.Context) error { return fmt.Errorf("Error parsing JSON file input - %v\n", e) } } - // Get the task name - name := ctx.String("name") - // Get the interval - isCron := false - i := ctx.String("interval") - _, err := time.ParseDuration(i) - if err != nil { - // try interpreting interval as cron entry - _, e := cron.Parse(i) - if e != nil { - return fmt.Errorf("Bad interval format:\nfor simple schedule: %v\nfor cron schedule: %v\n", err, e) - } - isCron = true - } - // Deadline for a task - dl := ctx.String("deadline") - maxFailuresStr := ctx.String("max-failures") - maxFailures, err := stringValToInt(maxFailuresStr) - if err != nil { - return fmt.Errorf("Value for command-line option 'max-failures' (%v) cannot be parsed as an integer\n", maxFailuresStr) + // create a dummy task with an empty schedule + t := task{ + Version: 1, + Schedule: &client.Schedule{}, } - var sch *client.Schedule - // None of these mean it is a simple schedule - if !ctx.IsSet("start-date") && !ctx.IsSet("start-time") && !ctx.IsSet("stop-date") && !ctx.IsSet("stop-time") { - // Check if duration was set - if ctx.IsSet("duration") && !isCron { - d, err := time.ParseDuration(ctx.String("duration")) - if err != nil { - return fmt.Errorf("Bad duration format:\n%v\n", err) - } - start := time.Now().Add(createTaskNowPad) - stop := start.Add(d) - sch = &client.Schedule{ - Type: "windowed", - Interval: i, - StartTime: &start, - StopTime: &stop, - } - } else { - // No start or stop and no duration == simple schedule - t := "simple" - if isCron { - // It's a cron schedule, ignore "duration" if set - t = "cron" - } - sch = &client.Schedule{ - Type: t, - Interval: i, - } - } - } else { - // We have some form of windowed schedule - start := mergeDateTime( - strings.ToUpper(ctx.String("start-time")), - strings.ToUpper(ctx.String("start-date")), - ) - stop := mergeDateTime( - strings.ToUpper(ctx.String("stop-time")), - strings.ToUpper(ctx.String("stop-date")), - ) - - // Use duration to create missing start or stop - if ctx.IsSet("duration") { - d, err := time.ParseDuration(ctx.String("duration")) - if err != nil { - return fmt.Errorf("Bad duration format:\n%v\n", err) - } - // if start is set and stop is not then use duration to create stop - if start != nil && stop == nil { - t := start.Add(d) - stop = &t - } - // if stop is set and start is not then use duration to create start - if stop != nil && start == nil { - t := stop.Add(d * -1) - start = &t - } - } - sch = &client.Schedule{ - Type: "windowed", - Interval: i, - StartTime: start, - StopTime: stop, - } + // fill in the details for that task from the command-line arguments passed in by the user; + // if an error is encountered, return it + if err := t.mergeCliOptions(ctx); err != nil { + return err } - // Create task - r := pClient.CreateTask(sch, wf, name, dl, !ctx.IsSet("no-start"), maxFailures) + + // and use the resulting struct (along with the workflow map we constructed, above) to create a new task + r := pClient.CreateTask(t.Schedule, wf, t.Name, t.Deadline, !ctx.IsSet("no-start"), t.MaxFailures) if r.Err != nil { errors := strings.Split(r.Err.Error(), " -- ") errString := "Error creating task:" diff --git a/pkg/schedule/schedule.go b/pkg/schedule/schedule.go index 45e737420..23388b5ba 100644 --- a/pkg/schedule/schedule.go +++ b/pkg/schedule/schedule.go @@ -8,8 +8,6 @@ import ( var ( // ErrInvalidInterval - Error message for the valid schedule interval must ne greater than 0 ErrInvalidInterval = errors.New("Interval must be greater than 0") - // ErrInvalidStartTime - Error message for the start time is in the past - ErrInvalidStartTime = errors.New("Start time is in the past") // ErrInvalidStopTime - Error message for the stop tome is in the past ErrInvalidStopTime = errors.New("Stop time is in the past") // ErrStopBeforeStart - Error message for the stop time cannot occur before start time diff --git a/pkg/schedule/windowed_schedule.go b/pkg/schedule/windowed_schedule.go index 9cdc3ab71..abb4f16a0 100644 --- a/pkg/schedule/windowed_schedule.go +++ b/pkg/schedule/windowed_schedule.go @@ -36,12 +36,16 @@ func (w *WindowedSchedule) GetState() ScheduleState { // Validate validates the start, stop and duration interval of // WindowedSchedule func (w *WindowedSchedule) Validate() error { + // if the stop time was set but it is in the past, return an error if w.StopTime != nil && time.Now().After(*w.StopTime) { return ErrInvalidStopTime } + // if the start and stop time were both set and the the stop time is before + // the start time, return an error if w.StopTime != nil && w.StartTime != nil && w.StopTime.Before(*w.StartTime) { return ErrStopBeforeStart } + // if the interval is less than zero, return an error if w.Interval <= 0 { return ErrInvalidInterval }