allow random_delay and execution_time_limit to be integers and properly validate

Also set the default the right way with execution_time_limit

Signed-off-by: Tim Smith <tsmith@chef.io>

2 files changed, 61 insertions, 33 deletions

diff --git a/lib/chef/resource/windows_task.rb b/lib/chef/resource/windows_task.rb
index b879cbe6e0..2e73cef04d 100644
--- a/lib/chef/resource/windows_task.rb
+++ b/lib/chef/resource/windows_task.rb
@@ -44,22 +44,22 @@ class Chef
       property :interactive_enabled, [TrueClass, FalseClass], default: false
       property :frequency_modifier, [Integer, String], default: 1
       property :frequency, equal_to: [:minute,
-                                       :hourly,
-                                       :daily,
-                                       :weekly,
-                                       :monthly,
-                                       :once,
-                                       :on_logon,
-                                       :onstart,
-                                       :on_idle,
-                                       :none], default: :hourly
+                                      :hourly,
+                                      :daily,
+                                      :weekly,
+                                      :monthly,
+                                      :once,
+                                      :on_logon,
+                                      :onstart,
+                                      :on_idle,
+                                      :none], default: :hourly
       property :start_day, String
       property :start_time, String
       property :day, [String, Integer]
       property :months, String
       property :idle_time, Integer
-      property :random_delay, String
-      property :execution_time_limit, String
+      property :random_delay, [String, Integer]
+      property :execution_time_limit, [String, Integer], default: "PT72H"
 
       attr_accessor :exists, :status, :enabled
 
@@ -71,15 +71,11 @@ class Chef
         end
 
         if execution_time_limit
-          raise ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as a String e.g. '60'." if execution_time_limit.to_i == 0
-          duration = sec_to_dur(execution_time_limit)
-          execution_time_limit(duration)
-        else
-          # schtask sets execution_time_limit as PT72H by default
-          # We are setting the default value here so that we can do idempotency check later
-          # Note: We can't use `default` in the property
-          # because it will raise error for Invalid values passed as "PT72H" is not in seconds
-          execution_time_limit("PT72H")
+          unless execution_time_limit == "PT72H" # don't double convert an iso08601 format duration
+            raise ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds an Integer or a String with numeric values only e.g. '60'." unless numeric_value_in_string?(execution_time_limit)
+            duration = sec_to_dur(execution_time_limit)
+            execution_time_limit(duration)
+          end
         end
 
         validate_start_time(start_time, frequency)
@@ -94,12 +90,18 @@ class Chef
 
       private
 
+      # Validate the passed value is numeric values only if it is a string
+      def numeric_value_in_string?(val)
+        return false if val.is_a?(String) && /\D/ =~ val # \D is any non-numeric value
+        true
+      end
+
       def validate_random_delay(random_delay, frequency)
         if [:once, :on_logon, :onstart, :on_idle, :none].include? frequency
           raise ArgumentError, "`random_delay` property is supported only for frequency :minute, :hourly, :daily, :weekly and :monthly"
         end
 
-        raise ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as a String e.g. '60'." if random_delay.to_i == 0
+        raise ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds an Integer or a String with numeric values only e.g. '60'." unless numeric_value_in_string?(random_delay)
       end
 
       def validate_start_day(start_day, frequency)
@@ -197,7 +199,7 @@ class Chef
           raise "idle_time property is only valid for tasks that run on_idle"
         end
 
-        unless idle_time.to_i > 0 && idle_time.to_i <= 999
+        unless idle_time > 0 && idle_time <= 999
           raise "idle_time value #{idle_time} is invalid. Valid values for :on_idle frequency are 1 - 999."
         end
       end
diff --git a/spec/unit/resource/windows_task_spec.rb b/spec/unit/resource/windows_task_spec.rb
index d5c95235cd..65b4111e8e 100644
--- a/spec/unit/resource/windows_task_spec.rb
+++ b/spec/unit/resource/windows_task_spec.rb
@@ -89,15 +89,28 @@ describe Chef::Resource::WindowsTask do
     it "raises error for invalid random_delay" do
       resource.frequency :monthly
       resource.random_delay "xyz"
-      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as a String e.g. '60'.")
+      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds an Integer or a String with numeric values only e.g. '60'.")
     end
 
-    it "converts seconds into iso8601 duration format" do
+    it "raises error for invalid random_delay which looks like an Integer" do
+      resource.frequency :monthly
+      resource.random_delay "5,000"
+      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds an Integer or a String with numeric values only e.g. '60'.")
+    end
+
+    it "converts seconds String into iso8601 duration format" do
       resource.frequency :monthly
       resource.random_delay "60"
       resource.after_created
       expect(resource.random_delay).to eq("PT60S")
     end
+
+    it "converts seconds Integer into iso8601 duration format" do
+      resource.frequency :monthly
+      resource.random_delay 60
+      resource.after_created
+      expect(resource.random_delay).to eq("PT60S")
+    end
   end
 
   context "when execution_time_limit isn't specified" do
@@ -110,10 +123,21 @@ describe Chef::Resource::WindowsTask do
   context "when execution_time_limit is passed" do
     it "raises error for invalid execution_time_limit" do
       resource.execution_time_limit "abc"
-      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as a String e.g. '60'.")
+      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds an Integer or a String with numeric values only e.g. '60'.")
     end
 
-    it "converts seconds into iso8601 format" do
+    it "raises error for invalid execution_time_limit that looks like an Integer" do
+      resource.execution_time_limit "5,000"
+      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds an Integer or a String with numeric values only e.g. '60'.")
+    end
+
+    it "converts seconds Integer into iso8601 format" do
+      resource.execution_time_limit 60
+      resource.after_created
+      expect(resource.execution_time_limit).to eq("PT60S")
+    end
+
+    it "converts seconds String into iso8601 format" do
       resource.execution_time_limit "60"
       resource.after_created
       expect(resource.execution_time_limit).to eq("PT60S")
@@ -205,35 +229,37 @@ describe Chef::Resource::WindowsTask do
     end
 
     it "raises error for invalid day value" do
-      expect  { resource.send(:validate_create_day, "xyz", :weekly) }.to raise_error("day property invalid. Only valid values are: MON, TUE, WED, THU, FRI, SAT, SUN and *. Multiple values must be separated by a comma.")
+      expect  { resource.send(:validate_create_day, "xyz", :weekly) }.to raise_error(Chef::Exceptions::RuntimeError, "day property invalid. Only valid values are: MON, TUE, WED, THU, FRI, SAT, SUN and *. Multiple values must be separated by a comma.")
     end
   end
 
   context "#validate_create_months" do
     it "raises error if frequency is not :monthly" do
-      expect  { resource.send(:validate_create_months, "Jan", :once) }.to raise_error("months property is only valid for tasks that run monthly")
+      expect  { resource.send(:validate_create_months, "Jan", :once) }.to raise_error(Chef::Exceptions::RuntimeError, "months property is only valid for tasks that run monthly")
     end
 
     it "accepts a valid single month" do
-      expect  { resource.send(:validate_create_months, "Feb", :monthly) }.not_to raise_error
+      expect  { resource.send(:validate_create_months, "Feb", :monthly) }.not_to raise_error(Chef::Exceptions::ArgumentError)
     end
 
     it "accepts a comma separated list of valid months" do
-      expect  { resource.send(:validate_create_months, "Jan, mar, AUG", :monthly) }.not_to raise_error
+      expect  { resource.send(:validate_create_months, "Jan, mar, AUG", :monthly) }.not_to raise_error(Chef::Exceptions::ArgumentError)
     end
 
     it "raises error for invalid month value" do
-      expect  { resource.send(:validate_create_months, "xyz", :monthly) }.to raise_error("months property invalid. Only valid values are: JAN, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV, DEC and *. Multiple values must be separated by a comma.")
+      expect  { resource.send(:validate_create_months, "xyz", :monthly) }.to raise_error(Chef::Exceptions::RuntimeError, "months property invalid. Only valid values are: JAN, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV, DEC and *. Multiple values must be separated by a comma.")
     end
   end
 
   context "#validate_idle_time" do
     it "raises error if frequency is not :on_idle" do
-      expect  { resource.send(:validate_idle_time, 5, :hourly) }.to raise_error("idle_time property is only valid for tasks that run on_idle")
+      expect  { resource.send(:validate_idle_time, 5, :hourly) }.to raise_error(Chef::Exceptions::RuntimeError, "idle_time property is only valid for tasks that run on_idle")
     end
 
     it "raises error if idle_time > 999" do
-      expect  { resource.send(:validate_idle_time, 1000, :on_idle) }.to raise_error("idle_time value 1000 is invalid. Valid values for :on_idle frequency are 1 - 999.")
+      expect  { resource.send(:validate_idle_time, 1000, :on_idle) }.to raise_error(Chef::Exceptions::RuntimeError, "idle_time value 1000 is invalid. Valid values for :on_idle frequency are 1 - 999.")
+    end
+  end
 
   context "#sec_to_dur" do
     it "return PT1S when passed 1" do