Merge pull request #6688 from MsysTechnologiesllc/vasundhara/chef#6638_windows_task_not_fully_idempotent

Fix windows_task resource not being idempotent for random_delay and execution_time_limit

4 files changed, 141 insertions, 42 deletions

diff --git a/lib/chef/provider/windows_task.rb b/lib/chef/provider/windows_task.rb
index 02e45cb9e0..d8f9094fd1 100644
--- a/lib/chef/provider/windows_task.rb
+++ b/lib/chef/provider/windows_task.rb
@@ -229,8 +229,7 @@ class Chef
             current_resource.frequency_modifier != new_resource.frequency_modifier ||
             current_resource.frequency != new_resource.frequency ||
             current_resource.idle_time != new_resource.idle_time ||
-            current_resource.random_delay != new_resource.random_delay ||
-            !new_resource.execution_time_limit.include?(current_resource.execution_time_limit) ||
+            random_delay_updated? || execution_time_limit_updated? ||
             (new_resource.start_day && new_resource.start_day != "N/A" && start_day_updated?) ||
             (new_resource.start_time && new_resource.start_time != "N/A" && start_time_updated?)
         begin
@@ -243,6 +242,37 @@ class Chef
         false
       end
 
+      # Comparing random_delay values using ISO8601::Duration object Ref: https://github.com/arnau/ISO8601/blob/master/lib/iso8601/duration.rb#L18-L23
+      # di = ISO8601::Duration.new(65707200)
+      # ds = ISO8601::Duration.new('P65707200S')
+      # dp = ISO8601::Duration.new('P2Y1MT2H')
+      # di == dp # => true
+      # di == ds # => true
+      def random_delay_updated?
+        if new_resource.random_delay.nil?
+          false
+        elsif current_resource.random_delay.nil? && new_resource.random_delay == "PT0S" # when user sets random_dealy to 0 sec
+          false
+        elsif current_resource.random_delay.nil?
+          true
+        else
+          ISO8601::Duration.new(current_resource.random_delay) != ISO8601::Duration.new(new_resource.random_delay)
+        end
+      end
+
+      # Comparing execution_time_limit values using Ref: https://github.com/arnau/ISO8601/blob/master/lib/iso8601/duration.rb#L18-L23
+      def execution_time_limit_updated?
+        if new_resource.execution_time_limit.nil?
+          false
+        elsif current_resource.execution_time_limit.nil? && new_resource.execution_time_limit == "PT0S" # when user sets random_dealy to 0 sec
+          false
+        elsif current_resource.execution_time_limit.nil?
+          true
+        else
+          ISO8601::Duration.new(current_resource.execution_time_limit) != ISO8601::Duration.new(new_resource.execution_time_limit)
+        end
+      end
+
       def start_day_updated?
         current_day = DateTime.strptime(current_resource.start_day, convert_system_date_format_to_ruby_date_format)
         new_day = parse_day(new_resource.start_day)
diff --git a/lib/chef/resource/windows_task.rb b/lib/chef/resource/windows_task.rb
index f61a119b5a..14f5da46fa 100644
--- a/lib/chef/resource/windows_task.rb
+++ b/lib/chef/resource/windows_task.rb
@@ -205,38 +205,13 @@ class Chef
         end
       end
 
-      # Convert the number of seconds to an ISO8601 duration format
-      # @see http://tools.ietf.org/html/rfc2445#section-4.3.6
-      # @param [Integer] seconds The amount of seconds for this duration
+      # Converts the number of seconds to an ISO8601 duration format and returns it.
+      # Ref : https://github.com/arnau/ISO8601/blob/master/lib/iso8601/duration.rb#L18-L23
+      # e.g.
+      # ISO8601::Duration.new(65707200)
+      # returns 'P65707200S'
       def sec_to_dur(seconds)
-        seconds = seconds.to_i
-        iso_str = "P"
-        if seconds > 604_800 # more than a week
-          weeks = seconds / 604_800
-          seconds -= (604_800 * weeks)
-          iso_str << "#{weeks}W"
-        end
-        if seconds > 86_400 # more than a day
-          days = seconds / 86_400
-          seconds -= (86_400 * days)
-          iso_str << "#{days}D"
-        end
-        if seconds >= 0
-          iso_str << "T"
-          if seconds > 3600 # more than an hour
-            hours = seconds / 3600
-            seconds -= (3600 * hours)
-            iso_str << "#{hours}H"
-          end
-          if seconds > 60 # more than a minute
-            minutes = seconds / 60
-            seconds -= (60 * minutes)
-            iso_str << "#{minutes}M"
-          end
-          iso_str << "#{seconds}S"
-        end
-
-        iso_str
+        ISO8601::Duration.new(seconds.to_i).to_s
       end
 
     end
diff --git a/spec/unit/provider/windows_task_spec.rb b/spec/unit/provider/windows_task_spec.rb
index ce0fe9e50e..a71ce92f56 100644
--- a/spec/unit/provider/windows_task_spec.rb
+++ b/spec/unit/provider/windows_task_spec.rb
@@ -620,4 +620,98 @@ describe Chef::Provider::WindowsTask do
       expect(provider.send(:frequency_modifier_allowed)).to be(false)
     end
   end
+
+  # In windows_task resource sec_to_dur method converts seconds to duration in format 60 == 'PT60S'
+  # random_delay_updated? method use the value return by sec_to_dur as input for comparison for new_resource.random_delay mocking the same here
+  describe "#random_delay_updated?" do
+    before do
+      new_resource.command "chef-client"
+      new_resource.run_level :highest
+      new_resource.frequency :minute
+      new_resource.frequency_modifier 15
+      new_resource.user "SYSTEM"
+    end
+
+    it "returns false if current_resource.random_delay = nil & random_delay is set to '0' seconds" do
+      task_hash[:random_delay] = nil
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.random_delay = "PT0S"
+      expect(provider.send(:random_delay_updated?)).to be(false)
+    end
+
+    it "returns false if current_resource.random_delay = 'P7D' & random_delay is set to '604800' seconds " do
+      task_hash[:random_delay] = "P7D"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.random_delay = "PT604800S"
+      expect(provider.send(:random_delay_updated?)).to be(false)
+    end
+
+    it "returns false if current_resource.random_delay = 'P7DT1S' & random_delay is set to '604801' seconds" do
+      task_hash[:random_delay] = "P7DT1S"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.random_delay = "PT604801S"
+      expect(provider.send(:random_delay_updated?)).to be(false)
+    end
+
+    it "returns true if current_resource.random_delay = 'PT1S' & random_delay is set to '3600' seconds" do
+      task_hash[:random_delay] = "PT1S"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.random_delay = "PT3600S"
+      expect(provider.send(:random_delay_updated?)).to be(true)
+    end
+
+    it "returns false if current_resource.random_delay = 'P2Y1MT2H' & random_delay is set to '65707200' seconds" do
+      task_hash[:random_delay] = "P2Y1MT2H"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.random_delay = "PT65707200S"
+      expect(provider.send(:random_delay_updated?)).to be(false)
+    end
+  end
+
+  describe "#execution_time_limit_updated?" do
+    before do
+      new_resource.command "chef-client"
+      new_resource.run_level :highest
+      new_resource.frequency :minute
+      new_resource.frequency_modifier 15
+      new_resource.user "SYSTEM"
+    end
+
+    it "returns false if current_resource.execution_time_limit = 'P7D' & execution_time_limit is set to 604800 seconds " do
+      task_hash[:execution_time_limit] = "P7D"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.execution_time_limit = "PT604800S"
+      expect(provider.send(:execution_time_limit_updated?)).to be(false)
+    end
+
+    it "returns false if current_resource.execution_time_limit = 'P7DT1S' & execution_time_limit is set to 604801 seconds" do
+      task_hash[:execution_time_limit] = "P7DT1S"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.execution_time_limit = "PT604801S"
+      expect(provider.send(:execution_time_limit_updated?)).to be(false)
+    end
+
+    it "returns true if current_resource.execution_time_limit = 'PT1S' & execution_time_limit is set to '3600' seconds" do
+      task_hash[:execution_time_limit] = "PT1S"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.execution_time_limit = "PT3600S"
+      expect(provider.send(:execution_time_limit_updated?)).to be(true)
+    end
+
+    it "returns false if current_resource.execution_time_limit = 'P2Y1MT2H' & execution_time_limit is set to '65707200' seconds" do
+      task_hash[:execution_time_limit] = "P2Y1MT2H"
+      allow(provider).to receive(:load_task_hash).and_return(task_hash)
+      provider.load_current_resource
+      new_resource.execution_time_limit = "PT65707200S"
+      expect(provider.send(:execution_time_limit_updated?)).to be(false)
+    end
+  end
 end
diff --git a/spec/unit/resource/windows_task_spec.rb b/spec/unit/resource/windows_task_spec.rb
index fe0c55ecaf..145063b53e 100644
--- a/spec/unit/resource/windows_task_spec.rb
+++ b/spec/unit/resource/windows_task_spec.rb
@@ -281,23 +281,23 @@ describe Chef::Resource::WindowsTask do
   end
 
   context "#sec_to_dur" do
-    it "return PT0S when passed 0" do
+    it "return nil when passed 0" do
       expect(resource.send(:sec_to_dur, 0)).to eql("PT0S")
     end
     it "return PT1S when passed 1" do
       expect(resource.send(:sec_to_dur, 1)).to eql("PT1S")
     end
-    it "return PT24H0S when passed 86400" do
-      expect(resource.send(:sec_to_dur, 86400)).to eql("PT24H0S")
+    it "return PT86400S when passed 86400" do
+      expect(resource.send(:sec_to_dur, 86400)).to eql("PT86400S")
     end
-    it "return P1DT1S when passed 86401" do
-      expect(resource.send(:sec_to_dur, 86401)).to eql("P1DT1S")
+    it "return PT86401S when passed 86401" do
+      expect(resource.send(:sec_to_dur, 86401)).to eql("PT86401S")
     end
-    it "return P1DT1M40S when passed 86500" do
-      expect(resource.send(:sec_to_dur, 86500)).to eql("P1DT1M40S")
+    it "return PT86500S when passed 86500" do
+      expect(resource.send(:sec_to_dur, 86500)).to eql("PT86500S")
     end
-    it "return P1WT1S when passed 604801" do
-      expect(resource.send(:sec_to_dur, 604801)).to eql("P1WT1S")
+    it "return PT604801S when passed 604801" do
+      expect(resource.send(:sec_to_dur, 604801)).to eql("PT604801S")
     end
   end
 end