Merge pull request #175 from RandomNoun7/MODULES-10893-fix-last-day-of-month

(MODULES-10893) Fix Last Day Of Month Trigger

Author: sanfrancrisko

README.md

@@ -301,6 +301,9 @@ For all triggers:
     Each month must be an integer between 1 and 12.
   * `on` (Required) — Which days of the month the task should run, as an array.
     Each day must be an integer between 1 and 31.
+    * The string `last` may be used in the array for this property to trigger a
+    task to run on the last day of each selected month. This feature is only
+    available for tasks with compatibility level `2` or higher.
 * For monthly (by weekday) triggers:
   * `months` — Which months the task should run, as an array. Defaults to all months. Each month must be an integer between 1 and 12.
   * `day_of_week` (Required) — Which day of the week the task should run, as an array with only one element.

lib/puppet/type/scheduled_task.rb

@@ -163,6 +163,7 @@ def insync?(current)
             all months. Each month must be an integer between 1 and 12.
           * `on` **(Required)** --- Which days of the month the task should run,
             as an array. Each day must be an integer between 1 and 31 or the string `last`.
+            The string `last` is only supported for tasks with level 2 compatibility or higher.
       * For `monthly` (by weekday) triggers:
           * `months` --- Which months the task should run, as an array. Defaults to
             all months. Each month must be an integer between 1 and 12.

lib/puppet_x/puppetlabs/scheduled_task/trigger.rb

@@ -5,7 +5,8 @@
 # @api private
 # PuppetX::PuppetLabs::ScheduledTask module
 module PuppetX::PuppetLabs::ScheduledTask
-  # This module is used to manage the triggers for the Task Scheduler V2 API
+  # @api private
+  # PuppetX::PuppetLabs::ScheduledTask::Trigger module
   module Trigger
     # Gets or sets the amount of time that is allowed to complete the task.
     class Duration
@@ -389,42 +390,46 @@ def self.bitmask_to_names(bitmask)
 
       # Defines the day of the month the task will run.
       class Days
-        # 32 bits for 31 possible days to set + value 'last'
-        MAX_VALUE = 0b11111111111111111111111111111111
+        # 32 bit mask, but only 31 days can be set.
+        # This is contrary to the V2 IMonthlyTrigger::DaysOfMonth documentation
+        # referenced below, but in testing, setting the last bit of the bit
+        # mask does not set 'last' day of month as that documentation suggests.
+        # Instead it results in an error. That feature will be handled instead
+        # by the RunOnLastDayOfMonth property of the trigger object.
+        # V2 IMonthlyTrigger::RunOnLastDayOfMonth
+        # https://docs.microsoft.com/en-us/windows/win32/api/taskschd/nf-taskschd-imonthlytrigger-put_runonlastdayofmonth
+        MAX_VALUE = 0b01111111111111111111111111111111
 
         # V1 MONTHLYDATE structure
         # https://msdn.microsoft.com/en-us/library/windows/desktop/aa381918(v=vs.85).aspx
         # V2 IMonthlyTrigger::DaysOfMonth
         # https://msdn.microsoft.com/en-us/library/windows/desktop/aa380735(v=vs.85).aspx
         def self.indexes_to_bitmask(day_indexes)
-          day_indexes = [day_indexes].flatten.map do |m|
-            # The special "day" of 'last' is represented by day "number"
-            # 32. 'last' has the special meaning of "the last day of the
-            # month", no matter how many days there are in the month.
-            # raises if unable to convert
-            (m.is_a?(String) && m.casecmp('last').zero?) ? 32 : Integer(m)
+          if day_indexes.nil? || (day_indexes.is_a?(Hash) && day_indexes.empty?)
+            raise TypeError, 'Day indexes value must not be nil or an empty hash.'
           end
 
-          invalid_days = day_indexes.reject { |i| i.between?(1, 32) }
+          integer_days = Array(day_indexes).select { |i| i.is_a?(Integer) }
+          invalid_days = integer_days.reject { |i| i.between?(1, 31) }
+
           unless invalid_days.empty?
-            raise ArgumentError, "Day indexes value #{invalid_days.join(', ')} is invalid. Integers must be in the range 1-31, or 'last'"
+            raise ArgumentError, "Day indexes value #{invalid_days.join(', ')} is invalid. Integers must be in the range 1-31"
           end
-
-          day_indexes.reduce(0) { |bitmask, day_index| bitmask | 1 << day_index - 1 }
+          integer_days.reduce(0) { |bitmask, day_index| bitmask | 1 << day_index - 1 }
         end
 
         # Converts bitmask to index
-        def self.bitmask_to_indexes(bitmask)
+        def self.bitmask_to_indexes(bitmask, run_on_last_day_of_month = nil)
           bitmask = Integer(bitmask)
           if bitmask.negative? || bitmask > MAX_VALUE
             raise ArgumentError, "bitmask must be specified as an integer from 0 to #{MAX_VALUE.to_s(10)}"
           end
 
-          bit_index(bitmask).map do |bit_index|
-            # Day 32 has the special meaning of "the last day of the
-            # month", no matter how many days there are in the month.
-            (bit_index == 31) ? 'last' : bit_index + 1
-          end
+          indexes = bit_index(bitmask).map { |bit_index| bit_index + 1 }
+
+          indexes << 'last' if run_on_last_day_of_month
+
+          indexes
         end
 
         # Returns bit index
@@ -435,6 +440,14 @@ def self.bit_index(bitmask)
             (bitmask & bit_to_check) == bit_to_check
           end
         end
+
+        def self.last_day_of_month?(day_indexes)
+          invalid_day_names = Array(day_indexes).select { |i| i.is_a?(String) && (i != 'last') }
+          unless invalid_day_names.empty?
+            raise ArgumentError, "Only 'last' is allowed as a day name. All other values must be integers between 1 and 31."
+          end
+          Array(day_indexes).include? 'last'
+        end
       end
 
       # Gets or sets the months of the year during which the task runs.
@@ -705,16 +718,16 @@ def self.to_manifest_hash(i_trigger)
           manifest_hash['schedule'] = 'daily'
           manifest_hash['every'] = i_trigger.DaysInterval
         when Type::TASK_TRIGGER_WEEKLY
-          manifest_hash.merge!('schedule' => 'weekly',
+          manifest_hash.merge!('schedule'    => 'weekly',
                                'every'       => i_trigger.WeeksInterval,
                                'day_of_week' => Day.bitmask_to_names(i_trigger.DaysOfWeek))
         when Type::TASK_TRIGGER_MONTHLY
           manifest_hash.merge!('schedule' => 'monthly',
                                'months'   => Month.bitmask_to_indexes(i_trigger.MonthsOfYear),
-                               'on'       => Days.bitmask_to_indexes(i_trigger.DaysOfMonth))
+                               'on'       => Days.bitmask_to_indexes(i_trigger.DaysOfMonth, i_trigger.RunOnLastDayOfMonth))
         when Type::TASK_TRIGGER_MONTHLYDOW
           occurrences = V2::WeeksOfMonth.bitmask_to_names(i_trigger.WeeksOfMonth)
-          manifest_hash.merge!('schedule' => 'monthly',
+          manifest_hash.merge!('schedule'         => 'monthly',
                                'months'           => Month.bitmask_to_indexes(i_trigger.MonthsOfYear),
                                # HACK: choose only the first week selected when converting - this LOSES information
                                'which_occurrence' => occurrences.first || '',
@@ -783,6 +796,7 @@ def self.append_trigger(definition, manifest_hash)
 
         when Type::TASK_TRIGGER_MONTHLY
           # https://msdn.microsoft.com/en-us/library/windows/desktop/aa382062(v=vs.85).aspx
+          i_trigger.RunOnLastDayOfMonth = Days.last_day_of_month?(manifest_hash['on'])
           i_trigger.DaysOfMonth = Days.indexes_to_bitmask(manifest_hash['on'])
           i_trigger.MonthsOfYear = Month.indexes_to_bitmask(manifest_hash['months'] || Month.indexes)
 

spec/acceptance/should_create_task_spec.rb

@@ -26,6 +26,35 @@
     end
   end
 
+  it 'creates a task that runs on the last day of the month: taskscheduler_api2', tier_high: true do
+    pp = <<-MANIFEST
+    scheduled_task {'#{taskname}':
+      ensure      => present,
+      compatibility => 2,
+      command     => 'c:\\\\windows\\\\system32\\\\notepad.exe',
+      arguments   => "foo bar baz",
+      working_dir => 'c:\\\\windows',
+      trigger => {
+        schedule   => 'monthly',
+        start_time => '12:00',
+        on         => [1, 3, 'last'],
+      },
+      provider    => 'taskscheduler_api2'
+    }
+    MANIFEST
+    apply_manifest(pp, catch_failures: true)
+
+    # Verify the task exists
+    query_cmd = "schtasks.exe /query /v /fo list /tn #{taskname}"
+    run_shell(query_cmd) do |result|
+      # Even though the bit mask value for '32' doesn't actuall work to set
+      # `last` day of the month as a trigger, schtasks.exe still returns day 32
+      # if `last` is set as a trigger day. My guess is that this is for backward
+      # compatability with something in schtasks.exe
+      expect(result.stdout).to match(%r{Days:.+01, 03, 32})
+    end
+  end
+
   it 'creates a task if it does not exist: taskscheduler_api2', tier_high: true do
     pp = <<-MANIFEST
     scheduled_task {'#{taskname}':

spec/unit/puppet_x/puppetlabs/scheduled_task/trigger_spec.rb

@@ -760,23 +760,21 @@
 
     # 31 bits for 31 days
     ALL_NUMERIC_DAYS_SET = 0b01111111111111111111111111111111
-    # 32 bits for 31 days + 'last'
-    ALL_DAYS_SET = 0b11111111111111111111111111111111
 
     EXPECTED_DAYS_CONVERSIONS =
       [
-        { days: 1,                       bitmask: 0b00000000000000000000000000000001 },
-        { days: [],                      bitmask: 0 },
-        { days: [2],                     bitmask: 0b00000000000000000000000000000010 },
-        { days: [3, 5, 8, 12],           bitmask: 0b00000000000000000000100010010100 },
-        { days: [3, 5, 8, 12, 'last'],   bitmask: 0b10000000000000000000100010010100 },
-        { days: (1..31).to_a,            bitmask: ALL_NUMERIC_DAYS_SET },
-        { days: (1..31).to_a + ['last'], bitmask: ALL_DAYS_SET },
+        { days: 1,                       bitmask: 0b00000000000000000000000000000001, LastDayOfMonth: false },
+        { days: [],                      bitmask: 0, LastDayOfMonth: false },
+        { days: [2],                     bitmask: 0b00000000000000000000000000000010, LastDayOfMonth: false },
+        { days: [3, 5, 8, 12],           bitmask: 0b00000000000000000000100010010100, LastDayOfMonth: false },
+        { days: [3, 5, 8, 12, 'last'],   bitmask: 0b00000000000000000000100010010100, LastDayOfMonth: true },
+        { days: (1..31).to_a,            bitmask: ALL_NUMERIC_DAYS_SET, LastDayOfMonth: false },
+        { days: (1..31).to_a + ['last'], bitmask: ALL_NUMERIC_DAYS_SET, LastDayOfMonth: true },
         # equivalent representations
-        { days: 'last',                  bitmask: 0b10000000000000000000000000000000 },
-        { days: ['last'],                bitmask: 1 << (32 - 1) },
-        { days: [1, 'last'],             bitmask: 0b10000000000000000000000000000001 },
-        { days: [1, 30, 31, 'last'],     bitmask: 0b11100000000000000000000000000001 },
+        { days: 'last',                  bitmask: 0b00000000000000000000000000000000, LastDayOfMonth: true },
+        { days: ['last'],                bitmask: 0b00000000000000000000000000000000, LastDayOfMonth: true },
+        { days: [1, 'last'],             bitmask: 0b00000000000000000000000000000001, LastDayOfMonth: true },
+        { days: [1, 30, 31, 'last'],     bitmask: 0b01100000000000000000000000000001, LastDayOfMonth: true },
       ].freeze
 
     describe '#indexes_to_bitmask' do
@@ -792,7 +790,7 @@
         end
       end
 
-      ['foo', ['bar'], -1, 0x1000, [33]].each do |value|
+      [-1, 0x1000, [33]].each do |value|
         it "should raise an ArgumentError with value: #{value}" do
           expect { days.class.indexes_to_bitmask(value) }.to raise_error(ArgumentError)
         end
@@ -802,7 +800,7 @@
     describe '#bitmask_to_indexes' do
       EXPECTED_DAYS_CONVERSIONS.each do |conversion|
         it "should create expected days #{conversion[:days]} from bitmask #{'%32b' % conversion[:bitmask]}" do
-          expect(days.class.bitmask_to_indexes(conversion[:bitmask])).to eq([conversion[:days]].flatten)
+          expect(days.class.bitmask_to_indexes(conversion[:bitmask], conversion[:LastDayOfMonth])).to eq([conversion[:days]].flatten)
         end
       end
 
@@ -812,9 +810,21 @@
         end
       end
 
-      ['foo', -1, ALL_DAYS_SET + 1].each do |value|
-        it "should raise an ArgumentError with value: #{value}" do
-          expect { days.class.bitmask_to_indexes(value) }.to raise_error(ArgumentError)
+      it "should raise an ArgumentError with value: #{ALL_NUMERIC_DAYS_SET + 1}" do
+        expect { days.class.bitmask_to_indexes(ALL_NUMERIC_DAYS_SET + 1) }.to raise_error(ArgumentError)
+      end
+    end
+
+    describe '#last_day_of_month?' do
+      EXPECTED_DAYS_CONVERSIONS.each do |conversion|
+        it "returns #{conversion[:LastDayOfMonth]} from days #{conversion[:days]}" do
+          expect(days.class.last_day_of_month?(conversion[:days])).to eq(conversion[:LastDayOfMonth])
+        end
+      end
+
+      [[1, 3, 'first'], [1, 3, 'last', 'first']].each do |value|
+        it "returns an ArgumentError for days #{value}" do
+          expect { days.class.last_day_of_month?(value) }.to raise_error(ArgumentError)
         end
       end
     end
@@ -1048,9 +1058,9 @@
         {
           i_trigger:         {
             Type: v2::Type::TASK_TRIGGER_MONTHLY,
-            DaysOfMonth: 0b11111111111111111111111111111111,
+            DaysOfMonth: 0b01111111111111111111111111111111,
             MonthsOfYear: 1,
-            RunOnLastDayOfMonth: true, # ignored
+            RunOnLastDayOfMonth: true,
             RandomDelay: 'P2DT5S', # ignored
           },
           expected:         {
@@ -1066,7 +1076,7 @@
             # HACK: choose only the last week selected for test conversion, as this LOSES information
             WeeksOfMonth: 0b10000,
             MonthsOfYear: 0b111111111111,
-            RunOnLastWeekOfMonth: true, # ignored
+            RunOnLastWeekOfMonth: true,
             RandomDelay: 'P2DT5S', # ignored
           },
           expected:         {