(PUP-1165) Perform validation on desired triggers not current

The `enabled` and `index` trigger parameters are read-only, so they will be set
in the `current` trigger, but are not allowed in the `desired` trigger.
Previously, the `translate_hash_to_trigger` method performed validation based
on whether it was passed a `current` or `desired` trigger.

This commit moves the validation logic to the `validate_trigger` method, which
is only ever called with `desired` trigger values. It also allows the `index`
and `enabled` keys to be present in a trigger. This isn't an issue because they
are ignored for the purposes of determining if two triggers are the same.

The reason for this change, is because ultimately we don't want the
`translate_hash_to_trigger` method to mutate the `current` trigger, and this
commit makes it so that the `index` and `enabled` keys can be in the
`puppet_trigger` but will be ignored.

2 files changed, 27 insertions, 9 deletions

diff --git a/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb b/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb
index 91526d7fa..ea7b54c96 100644
--- a/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb
+++ b/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb
@@ -274,13 +274,9 @@ Puppet::Type.type(:scheduled_task).provide(:win32_taskscheduler) do
     }
   end
 
-  def translate_hash_to_trigger(puppet_trigger, user_provided_input=false)
+  def translate_hash_to_trigger(puppet_trigger)
     trigger = dummy_time_trigger
 
-    if user_provided_input
-      self.fail "'enabled' is read-only on scheduled_task triggers and should be removed ('enabled' is usually provided in puppet resource scheduled_task)." if puppet_trigger.has_key?('enabled')
-      self.fail "'index' is read-only on scheduled_task triggers and should be removed ('index' is usually provided in puppet resource scheduled_task)."   if puppet_trigger.has_key?('index')
-    end
     puppet_trigger.delete('index')
 
     if puppet_trigger.delete('enabled') == false
@@ -289,7 +285,7 @@ Puppet::Type.type(:scheduled_task).provide(:win32_taskscheduler) do
       trigger['flags'] &= ~Win32::TaskScheduler::TASK_TRIGGER_FLAG_DISABLED
     end
 
-    extra_keys = puppet_trigger.keys.sort - ['schedule', 'start_date', 'start_time', 'every', 'months', 'on', 'which_occurrence', 'day_of_week']
+    extra_keys = puppet_trigger.keys.sort - ['index', 'enabled', 'schedule', 'start_date', 'start_time', 'every', 'months', 'on', 'which_occurrence', 'day_of_week']
     self.fail "Unknown trigger option(s): #{Puppet::Parameter.format_value_for_display(extra_keys)}" unless extra_keys.empty?
     self.fail "Must specify 'start_time' when defining a trigger" unless puppet_trigger['start_time']
 
@@ -361,9 +357,17 @@ Puppet::Type.type(:scheduled_task).provide(:win32_taskscheduler) do
   def validate_trigger(value)
     value = [value] unless value.is_a?(Array)
 
-    # translate_hash_to_trigger handles the same validation that we
-    # would be doing here at the individual trigger level.
-    value.each {|t| translate_hash_to_trigger(t, true)}
+    value.each do |t|
+      if t.has_key?('index')
+        self.fail "'index' is read-only on scheduled_task triggers and should be removed ('index' is usually provided in puppet resource scheduled_task)."
+      end
+
+      if t.has_key?('enabled')
+        self.fail "'enabled' is read-only on scheduled_task triggers and should be removed ('enabled' is usually provided in puppet resource scheduled_task)."
+      end
+
+      translate_hash_to_trigger(t)
+    end
 
     true
   end
diff --git a/spec/unit/provider/scheduled_task/win32_taskscheduler_spec.rb b/spec/unit/provider/scheduled_task/win32_taskscheduler_spec.rb
index 3d37956c5..7d080174f 100644
--- a/spec/unit/provider/scheduled_task/win32_taskscheduler_spec.rb
+++ b/spec/unit/provider/scheduled_task/win32_taskscheduler_spec.rb
@@ -635,6 +635,20 @@ describe Puppet::Type.type(:scheduled_task).provider(:win32_taskscheduler), :if
   describe '#triggers_same?' do
     let(:provider) { described_class.new(:name => 'foobar', :command => 'C:\Windows\System32\notepad.exe') }
 
+    it "ignores 'index' in current trigger" do
+      current = {'index' => 0, 'schedule' => 'daily', 'start_date' => '2011-09-12', 'start_time' => '15:30', 'every' => 3}
+      desired = {'schedule' => 'daily', 'start_date' => '2011-09-12', 'start_time' => '15:30', 'every' => 3}
+
+      expect(provider).to be_triggers_same(current, desired)
+    end
+
+    it "ignores 'enabled' in current triggger" do
+      current = {'enabled' => true, 'schedule' => 'daily', 'start_date' => '2011-09-12', 'start_time' => '15:30', 'every' => 3}
+      desired = {'schedule' => 'daily', 'start_date' => '2011-09-12', 'start_time' => '15:30', 'every' => 3}
+
+      expect(provider).to be_triggers_same(current, desired)
+    end
+
     it "should not consider a disabled 'current' trigger to be the same" do
       current = {'schedule' => 'once', 'enabled' => false}
       desired = {'schedule' => 'once'}