Skip to content

Commit

Permalink
(WIP)(MODULES-6895) Extract Trigger manifest validation
Browse files Browse the repository at this point in the history 
 - Refactor from_manifest_hash, moving the canonicalization and
   validation into a completely separate method called on the V1 class
   called canonicalize_and_validate_manifest

 - Simplify code as appropriate, removing unnecessary or verbose
   code - for instance .keys.include? becomes .key?

 - Change the raised error type from Puppet::Error to ArgumentError to
   be more consistent with Ruby. This exception type can be caught and
   raised as a Puppet::Error later if necessary

 - Update tests to understand this error type
  • Loading branch information
@Iristyle
Iristyle committed Apr 17, 2018
1 parent ef56798 commit 0ef2369
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 47 deletions.
117 changes: 85 additions & 32 deletions lib/puppet_x/puppetlabs/scheduled_task/trigger.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ class V1
'minutes_duration'
]

ValidManifestScheduleKeys = [
'daily',
'weekly',
'monthly',
'once'
]

# https://msdn.microsoft.com/en-us/library/windows/desktop/aa383618(v=vs.85).aspx
class Flag
TASK_TRIGGER_FLAG_HAS_END_DATE = 0x1
Expand Down Expand Up @@ -377,8 +384,84 @@ def self.time_trigger_once_now
}
end

# canonicalize given trigger hash
# throws errors if hash structure is invalid
# @returns original object with downcased keys
def self.canonicalize_and_validate_manifest(manifest_hash)
raise TypeError unless manifest_hash.is_a?(Hash)
manifest_hash = downcase_keys(manifest_hash)

# check for valid key usage
invalid_keys = manifest_hash.keys - ValidManifestKeys
raise ArgumentError.new("Unknown trigger option(s): #{Puppet::Parameter.format_value_for_display(invalid_keys)}") unless invalid_keys.empty?

if !ValidManifestScheduleKeys.include?(manifest_hash['schedule'])
raise ArgumentError.new("Unknown schedule type: #{manifest_hash["schedule"].inspect}")
end

# required fields
%w{start_time}.each do |field|
next if manifest_hash.key?(field)
raise ArgumentError.new("Must specify '#{field}' when defining a trigger")
end

# specific setting rules for schedule types
case manifest_hash['schedule']
when 'monthly'
if manifest_hash.key?('on')
if manifest_hash.key?('day_of_week') || manifest_hash.key?('which_occurrence')
raise ArgumentError.new("Neither 'day_of_week' nor 'which_occurrence' can be specified when creating a monthly date-based trigger")
elsif manifest_hash.key?('which_occurrence') || manifest_hash.key?('day_of_week')
raise ArgumentError.new('which_occurrence cannot be specified as an array') if manifest_hash['which_occurrence'].is_a?(Array)

%w{day_of_week which_occurrence}.each do |field|
next if manifest_hash.key?(field)
raise ArgumentError.new("#{field} must be specified when creating a monthly day-of-week based trigger")
end
else
raise ArgumentError.new("Don't know how to create a 'monthly' schedule with the options: #{manifest_hash.keys.sort.join(', ')}")
end
end
when 'once'
raise ArgumentError.new("Must specify 'start_date' when defining a one-time trigger") unless manifest_hash['start_date']
end

if manifest_hash.key?('start_date')
min_date = Date.new(1753, 1, 1)
start_date = Date.parse(manifest_hash['start_date'])
raise ArgumentError.new("start_date must be on or after 1753-01-01") unless start_date >= min_date
end

# interval < 0
if manifest_hash.key?('minutes_interval') && Integer(manifest_hash['minutes_interval']) < 0
raise ArgumentError.new('minutes_interval must be an integer greater or equal to 0')
end

# interval set without duration
if manifest_hash.key?('minutes_interval') && !manifest_hash.key?('minutes_duration')
interval = Integer(manifest_hash['minutes_interval'])
if interval >= -1 && interval > 0
raise ArgumentError.new('minutes_interval cannot be set without minutes_duration also being set to a number greater than 0')
end
end

# duration with / without interval
if manifest_hash.key?('minutes_duration')
duration = Integer(manifest_hash['minutes_duration'])
# defaults to 1 day when unspecified
interval = Integer(manifest_hash['minutes_interval'] || 1440)
if duration <= interval && duration != 0
raise ArgumentError.new('minutes_duration must be an integer greater than minutes_interval and equal to or greater than 0')
end
end

manifest_hash
end

# manifest_hash is a hash created from a manifest
def self.from_manifest_hash(manifest_hash)
manifest_hash = canonicalize_and_validate_manifest(manifest_hash)

trigger = time_trigger_once_now

if manifest_hash['enabled'] == false
Expand All @@ -387,10 +470,6 @@ def self.from_manifest_hash(manifest_hash)
trigger['flags'] &= ~Flag::TASK_TRIGGER_FLAG_DISABLED
end

extra_keys = manifest_hash.keys.sort - ValidManifestKeys
raise Puppet::Error.new("Unknown trigger option(s): #{Puppet::Parameter.format_value_for_display(extra_keys)}") unless extra_keys.empty?
raise Puppet::Error.new("Must specify 'start_time' when defining a trigger") unless manifest_hash['start_time']

case manifest_hash['schedule']
when 'daily'
trigger['trigger_type'] = :TASK_TIME_TRIGGER_DAILY
Expand All @@ -411,60 +490,34 @@ def self.from_manifest_hash(manifest_hash)
}

if manifest_hash.keys.include?('on')
if manifest_hash.has_key?('day_of_week') or manifest_hash.has_key?('which_occurrence')
raise Puppet::Error.new("Neither 'day_of_week' nor 'which_occurrence' can be specified when creating a monthly date-based trigger")
end

trigger['trigger_type'] = :TASK_TIME_TRIGGER_MONTHLYDATE
trigger['type']['days'] = Days.indexes_to_bitfield(manifest_hash['on'])
elsif manifest_hash.keys.include?('which_occurrence') or manifest_hash.keys.include?('day_of_week')
raise Puppet::Error.new('which_occurrence cannot be specified as an array') if manifest_hash['which_occurrence'].is_a?(Array)
%w{day_of_week which_occurrence}.each do |field|
raise Puppet::Error.new("#{field} must be specified when creating a monthly day-of-week based trigger") unless manifest_hash.has_key?(field)
end

trigger['trigger_type'] = :TASK_TIME_TRIGGER_MONTHLYDOW
trigger['type']['weeks'] = Occurrence.name_to_constant(manifest_hash['which_occurrence'])
trigger['type']['days_of_week'] = Day.names_to_bitfield(manifest_hash['day_of_week'])
else
raise Puppet::Error.new("Don't know how to create a 'monthly' schedule with the options: #{manifest_hash.keys.sort.join(', ')}")
end
when 'once'
raise Puppet::Error.new("Must specify 'start_date' when defining a one-time trigger") unless manifest_hash['start_date']

trigger['trigger_type'] = :TASK_TIME_TRIGGER_ONCE
else
raise Puppet::Error.new("Unknown schedule type: #{manifest_hash["schedule"].inspect}")
end

integer_interval = -1
if manifest_hash['minutes_interval']
integer_interval = Integer(manifest_hash['minutes_interval'])
raise Puppet::Error.new('minutes_interval must be an integer greater or equal to 0') if integer_interval < 0
trigger['minutes_interval'] = integer_interval
end

integer_duration = -1
if manifest_hash['minutes_duration']
integer_duration = Integer(manifest_hash['minutes_duration'])
raise Puppet::Error.new('minutes_duration must be an integer greater than minutes_interval and equal to or greater than 0') if integer_duration <= integer_interval && integer_duration != 0
trigger['minutes_duration'] = integer_duration
trigger['minutes_duration'] = Integer(manifest_hash['minutes_duration'])
end

if integer_interval > 0 && integer_duration == -1
if integer_interval > 0 && !manifest_hash.key?('minutes_duration')
minutes_in_day = 1440
integer_duration = minutes_in_day
trigger['minutes_duration'] = minutes_in_day
end

if integer_interval >= integer_duration && integer_interval > 0
raise Puppet::Error.new('minutes_interval cannot be set without minutes_duration also being set to a number greater than 0')
end

if start_date = manifest_hash['start_date']
start_date = Date.parse(start_date)
raise Puppet::Error.new("start_date must be on or after 1753-01-01") unless start_date >= Date.new(1753, 1, 1)

trigger['start_year'] = start_date.year
trigger['start_month'] = start_date.month
trigger['start_day'] = start_date.day
Expand Down
40 changes: 25 additions & 15 deletions spec/unit/puppet/provider/scheduled_task/win32_taskscheduler_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
end
end

let(:error_class) do
described_class == Puppet::Type::Scheduled_task::ProviderWin32_taskscheduler ?
Puppet::Error : ArgumentError
end

before :each do
Win32::TaskScheduler.any_instance.stubs(:save)
PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2V1Task.any_instance.stubs(:save)
Expand Down Expand Up @@ -48,7 +53,7 @@ def date_component
trigger_hash['start_date'] = '1752-12-31'

expect { date_component }.to raise_error(
Puppet::Error,
error_class,
'start_date must be on or after 1753-01-01'
)
end
Expand Down Expand Up @@ -139,6 +144,11 @@ def time_component
Puppet::Type.type(:scheduled_task).stubs(:defaultprovider).returns(described_class)
end

let(:trigger_validation_error_class) do
described_class == Puppet::Type::Scheduled_task::ProviderWin32_taskscheduler ?
Puppet::Error : ArgumentError
end

describe 'when retrieving' do
before :each do
@mock_task = stub
Expand Down Expand Up @@ -1234,7 +1244,7 @@ def time_component
@puppet_trigger['minutes_interval'] = '-1'

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
'minutes_interval must be an integer greater or equal to 0'
)
end
Expand All @@ -1258,7 +1268,7 @@ def time_component
@puppet_trigger['minutes_duration'] = '-1'

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
'minutes_duration must be an integer greater than minutes_interval and equal to or greater than 0'
)
end
Expand Down Expand Up @@ -1293,7 +1303,7 @@ def time_component
@puppet_trigger['minutes_duration'] = '10'

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
'minutes_duration must be an integer greater than minutes_interval and equal to or greater than 0'
)
end
Expand All @@ -1311,7 +1321,7 @@ def time_component
@puppet_trigger['minutes_duration'] = '9'

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
'minutes_duration must be an integer greater than minutes_interval and equal to or greater than 0'
)
end
Expand All @@ -1321,7 +1331,7 @@ def time_component
@puppet_trigger['minutes_duration'] = '0'

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
'minutes_interval cannot be set without minutes_duration also being set to a number greater than 0'
)
end
Expand All @@ -1344,7 +1354,7 @@ def time_component
@puppet_trigger.delete('start_date')

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/Must specify 'start_date' when defining a one-time trigger/
)
end
Expand All @@ -1353,7 +1363,7 @@ def time_component
@puppet_trigger.delete('start_time')

expect { trigger }.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/Must specify 'start_time' when defining a trigger/
)
end
Expand Down Expand Up @@ -1475,7 +1485,7 @@ def time_component
@puppet_trigger['which_occurrence'] = 'first'

expect {trigger}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/Neither 'day_of_week' nor 'which_occurrence' can be specified when creating a monthly date-based trigger/
)
end
Expand All @@ -1484,7 +1494,7 @@ def time_component
@puppet_trigger['day_of_week'] = 'mon'

expect {trigger}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/Neither 'day_of_week' nor 'which_occurrence' can be specified when creating a monthly date-based trigger/
)
end
Expand All @@ -1493,7 +1503,7 @@ def time_component
@puppet_trigger.delete('on')

expect {trigger}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/Don't know how to create a 'monthly' schedule with the options: schedule, start_date, start_time/
)
end
Expand Down Expand Up @@ -1525,7 +1535,7 @@ def time_component
@puppet_trigger['on'] = 15

expect {trigger}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/Neither 'day_of_week' nor 'which_occurrence' can be specified when creating a monthly date-based trigger/
)
end
Expand All @@ -1534,7 +1544,7 @@ def time_component
@puppet_trigger.delete('which_occurrence')

expect {trigger}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/which_occurrence must be specified when creating a monthly day-of-week based trigger/
)
end
Expand All @@ -1543,7 +1553,7 @@ def time_component
@puppet_trigger.delete('day_of_week')

expect {trigger}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/day_of_week must be specified when creating a monthly day-of-week based trigger/
)
end
Expand Down Expand Up @@ -1582,7 +1592,7 @@ def time_component
]

expect {provider.validate_trigger(triggers_to_validate)}.to raise_error(
Puppet::Error,
trigger_validation_error_class,
/#{Regexp.escape("Unknown trigger option(s): ['this is invalid']")}/
)
end
Expand Down

0 comments on commit 0ef2369

Please sign in to comment.