(MODULES-6895) Refactor provider code to Trigger #21

Iristyle · 2018-03-22T00:08:43Z

Completely overhaul Trigger manipulation code, removing it from the provider instance and into the Trigger class.

See individual commits for more details, but note that there are a number of new classes:

Trigger::V1::Day
Trigger::V1::Days
Trigger::V1::Month
Trigger::V1::Occurrence
Trigger::V1::Flag

michaeltlombardi · 2018-04-17T19:32:28Z

lib/puppet/provider/scheduled_task/taskscheduler_api2.rb

@@ -313,7 +313,7 @@ def translate_hash_to_trigger(puppet_trigger)
      }

      trigger['type']['days_of_week'] = if puppet_trigger['day_of_week']
-                                          PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Day.bitfield_from_days_of_week(puppet_trigger['day_of_week'])
+                                          PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Day.names_to_bitfield(puppet_trigger['day_of_week'])


Much easier to follow with the renaming

nit: Typically that's a bitmask not a bitfield. I don't think it's worth changing though, particularly if it's used everywhere

Yeah, the data structure is as close to a bitfield as Ruby can manage. But bitmask is certainly the more common term - I'll change it as long as there isn't too much carnage.

glennsarti

Looks good.. A few nit comments

glennsarti · 2018-04-18T01:28:39Z

lib/puppet/provider/scheduled_task/taskscheduler_api2.rb

@@ -313,7 +313,7 @@ def translate_hash_to_trigger(puppet_trigger)
      }

      trigger['type']['days_of_week'] = if puppet_trigger['day_of_week']
-                                          PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Day.bitfield_from_days_of_week(puppet_trigger['day_of_week'])
+                                          PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Day.names_to_bitfield(puppet_trigger['day_of_week'])


nit: Typically that's a bitmask not a bitfield. I don't think it's worth changing though, particularly if it's used everywhere

glennsarti · 2018-04-18T01:32:18Z

lib/puppet_x/puppetlabs/scheduled_task/trigger.rb

+        'thurs' => TASK_THURSDAY,
+        'fri'   => TASK_FRIDAY,
+        'sat'   => TASK_SATURDAY,
+      }


nit: Should this be frozen?

glennsarti · 2018-04-18T01:34:49Z

lib/puppet_x/puppetlabs/scheduled_task/trigger.rb

+        10 => TASK_OCTOBER,
+        11 => TASK_NOVEMBER,
+        12 => TASK_DECEMBER,
+      }


nit: Should this be frozen?

glennsarti · 2018-04-18T01:36:52Z

lib/puppet/provider/scheduled_task/taskscheduler_api2.rb

@@ -95,11 +95,11 @@ def trigger
        puppet_trigger['day_of_week'] = PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Day.bitfield_to_names(trigger['type']['days_of_week'])
      when Win32::TaskScheduler::TASK_TIME_TRIGGER_MONTHLYDATE
        puppet_trigger['schedule'] = 'monthly'
-        puppet_trigger['months']   = PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Month.months_from_bitfield(trigger['type']['months'])
+        puppet_trigger['months']   = PuppetX::PuppetLabs::ScheduledTask::Trigger::V1::Month.bitfield_to_indexes(trigger['type']['months'])


Not sure how I feel about indexes. Index into what?

They are month indexes .. i.e. 1 through 12. I couldn't readily think of a better name, but open to suggestions if you have any.

https://en.wikipedia.org/wiki/Month#Julian_and_Gregorian_calendars doesn't have too much else...

Is bitmask_to_numbers or bitmask_to_integers any better than bitmask_to_indexes?

Longer, but possibly bitmask_to_month_index?

glennsarti · 2018-04-18T01:41:11Z

lib/puppet/provider/scheduled_task/taskscheduler_api2.rb

@@ -105,8 +105,8 @@ def trigger
      when Win32::TaskScheduler::TASK_TIME_TRIGGER_ONCE
        puppet_trigger['schedule'] = 'once'
      end
-      puppet_trigger['start_date'] = self.class.normalized_date(trigger['start_year'], trigger['start_month'], trigger['start_day'])
-      puppet_trigger['start_time'] = self.class.normalized_time(trigger['start_hour'], trigger['start_minute'])
+      puppet_trigger['start_date'] = PuppetX::PuppetLabs::ScheduledTask::Trigger::V1.normalized_date(trigger['start_year'], trigger['start_month'], trigger['start_day'])


Typo in commit message ((MODULES-6895) Move normalized helpers to V1 class): "codd" -> "code"

Iristyle · 2018-04-20T00:13:52Z

lib/puppet_x/puppetlabs/scheduled_task/trigger.rb

+        'third'  => TASK_THIRD_WEEK,
+        'fourth' => TASK_FOURTH_WEEK,
+        'last'   => TASK_LAST_WEEK,
+      }


freeze me ... also, better name than CONST_MAP?

I was ok with const map as it's internal to the class so easy to reason about.

michaeltlombardi

Paired and walked through commit by commit, makes sense. LGTM!

Iristyle · 2018-04-23T03:51:29Z

CI is failing per https://jenkins-master-prod-1.delivery.puppetlabs.net/view/All/job/forge-windows_puppetlabs-scheduled_task_intn-sys_smoke-adhoc/7/PLATFORM=windows2008r2-64default.a,WORKER_LABEL=beaker/testReport/junit/spec.acceptance/should_create_task_spec/Should_create_a_scheduled_task_Should_create_a_task_if_it_does_not_exist/

Specifically the test at https://github.com/puppetlabs/puppetlabs-scheduled_task/blob/master/spec/acceptance/should_create_task_spec.rb is failing, so we'll need to resolve that one issue prior to merge.

glennsarti · 2018-04-23T06:24:42Z

Ahh ok. So adhoc jobs run on your branch, unlike PRs which run on the fork's head of master. Seeing that the error you're getting is related to;

23:07:55   Error: /Stage[main]/Main/Scheduled_task[pl254138]/compatibility: change from 1 to 1 failed: The taskscheduler_api2 provider can not handle attribute compatibility

and the last good run on CI was for commit a890f34 which includes a setter for compatibility I suspect, if you rebased onto updated master and ran through adhoc it should be green.

Given that, I'm happy to get this merged.

glennsarti

LGTM

- Move the bitfield_from_days_of_week method from the provider code to the Trigger helpers, specifically adding the helper to a new Day class. Include V1 TASK_XXX day constants in the Day class so that its not necessary to refer to WIN32::TaskScheduler constants, which requires loading Windows guarded code Simplify name -> constant lookup by using a Hash - Adjust single spec looking for Puppet::Error, given the Day class cannot use those errors and must use ArgumentError instead. - Add some basic sanity check tests around extracted code

- Rename V1.bitfield_from_days_of_week to V1.names_to_bitmask so that the method name matches behavior more closely and is named _to_ rather than _from_ like other Trigger methods. - Simplify the validity checking and the bitmask calculation to use the Enumerable inject method

- Move constant arrays from provider code to the V1::Day class as they're not used outside of the class, and the logic for setting up the internal V1 trigger struct can be simplified.

- Move days_of_week_from_bitfield from taskscheduler_api2 to V1::Day. Remove unnecessary day_of_week_constant_to_name method

- Rename V1::Day.days_of_week_from_bitfield to V1::Day.bitmask_to_names so that the method name matches behavior more closely and is named _to_ rather than _from_ like other Trigger methods. - Simplify the validity checking and the bitmask to name mechanism - Add new specs demonstrating invalid inputs after adding basic type check to method input

- Move the bitfield_from_months method from the provider code to the Trigger helpers, specifically adding the helper to new Month class. Include V1 TASK_XMONTHX month constants in the Month class so that its not necessary to refer to WIN32::TaskScheduler constants, which requires loading Windows guarded code Simplify name -> constant lookup by using a Hash - Move the months_from_bitfield method from the provider code to the Trigger helpers, specifically adding the helper to new Month class. - Remove constant arrays from provider code to the V1::Month class as they're not used outside of the class, and the logic for setting up the internal V1 trigger struct can be simplified. - Add some basic unit tests around the functions

- Rename V1::Month.bitfield_from_months to V1::Month.indexes_to_bitmask so that the method name matches behavior more closely and is named _to_ rather than _from_ like other Trigger methods. - Simplify the validity checking and the index to bitmask resolution mechanism - This improves the bad input detection for arguments passed as noted in the specs

- Rename Month.months_from_bitfield to Month.bitmask_to_indexes so that the method name matches behavior more closely and is named _to_ rather than _from_ like other Trigger methods. - Simplify the bitmask to month index lookup given a hash already keeps track of this relationship - Add some basic input validation and tests to check values

- This code is not consumed by anything

- Move the days_from_bitfield method from the provider code to the Trigger helpers, specifically adding the helper to new Days class. - Move the bitfield_from_days method from the provider code to the Trigger helpers, specifically adding the helper to new Days class. - Change call to self.fail to raise ArgumentError.new so that tests can pass. New tests do basic input verification.

- Rename Days.bitfield_from_days to Days.indexes_to_bitmask so that the method name matches behavior more closely and is named _to_ rather than _from_ like other Trigger methods. - Simplify the code that maps various day values to a bitmask over for clarity / maintainability

- Rename Days.days_from_bitfield to Days.bitmask_to_indexes so that the method name matches behavior more closely and is named _to_ rather than _from_ like other Trigger methods. - Simplify the bitmask to day index lookup code so the mapping is a 1:1 for the first 31 days and 'last' for 32

- Refactor Occurrence helper to map constants for the Trigger to its own class. - Replace class to occurrence_constant_to_name with PuppetX::PuppetLabs::ScheduledTask::Trigger::Occurrence.constant_to_name - Replace class to occurrence_name_to_constant with PuppetX::PuppetLabs::ScheduledTask::Trigger::Occurrence.name_to_constant

- Reimplement normalized_date and normalized_time to be easier to understand by passing actual param values and: * using the initializer for Date * building string in the normalized_time method - Note that to maintain parity with prior code the output of normalized_date uses %-m for months without leading 0s and %-d for days without leading 0s. - Temporarily guard existing tests against old provider until these methods are moved. NOTE: the original tests for the old provider validated that a string like '8:37 PM' would be properly parsed into '20:37'. However, note that the actual code never called the method in such a way as the string being created and passed to Date.parse was always setup like "hour:minute" and never included AM or PM

- normalized_date and normalized_time belong with the Trigger code so move them there - Note that the old tests had an assertion like normalized_time('8:37 PM').to eq('20:37'), however there were no callers that used a format that added PM. Therefore it's not important to observe that same behavior

- Simple method extraction - Modify tests to use new helper

- Rename V1.dummy_time_trigger to V1.time_trigger_once_now - Remove unnecessary usage of Win32 constant

- Move this helper to PuppetX::PuppetLabs::ScheduledTask::Trigger::V1 from the provider. This will later be refactored further for cleanliness - To keep tests passing, change self.fail to raise Puppet::Error.new to match previous behavior. These tests will be refactored in a subsequent commit. - Instead of keeping the translate_hash_to_trigger method available on the provider, update callsites to call the new helper. This requires updating the test suite, which currently understands *both* providers, to vary its behavior even further depending on which provider is the currently selected one. This is the point at which the new provider is not a 100% drop-in for the existing provider for *all* callers. In practical terms, the type only calls the providers validate_trigger method, so the provider is still compatible with the implementation inside of Puppet.

- Remove Win32 constants and replace with their symbol mappings For instance, the value of Win32::TaskScheduler::ONCE is :TASK_TIME_TRIGGER_ONCE - Remove PuppetX::PuppetLabs::ScheduledTask::Trigger::V1 namespace qualifications as they are no longer necessary - Extract array of allowed keys in a manifest trigger to the ValidManifestKeys constant, similar to how valid keys lists are maintained for the actual V1 trigger

- Create a Flag class to scope the values and use them internally within the V1::Trigger class

- Update the V1::Trigger method, renaming it from the less descriptive translate_hash_to_trigger to the more descriptive from_manifest_hash, given there are a number of Trigger representations in-use within the helpers, and it is getting confusing. - Update tests / call sites accordingly

- 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

- Create default triggers based on specific schedule types. This results in a reduction of code for mapping specific manifests into their corresponding internal trigger hash structures.

- Start with schedule type specific conversions, then move to more universal trigger settings - Remove unnecessary code so that its simpler / easier to understand

- There are no integration tests in this file, so relocate to unit

Iristyle · 2018-04-23T15:02:29Z

You must have edited your GH comment after initially leaving it @glennsarti? The email I got didn't have any of the info about commits or rebasing, but the initial one was enough to clue me in anyhow. I totally forgot this branch started before the second PR for 6526 landed, which had the acceptance fix. After realizing I needed a rebase, I rekicked adhoc and it has passed at https://jenkins-master-prod-1.delivery.puppetlabs.net/view/All/job/forge-windows_puppetlabs-scheduled_task_i18n-ruby_adhoc/4/

But yeah, the merge would have passed anyhow, but at least we get a quick turnaround on verification this way.

Thanks!

Iristyle changed the title ~~(MODULES-6895) Add Trigger::V1::Day class~~ (WIP)(MODULES-6895) Add Trigger::V1::Day class Mar 22, 2018

Iristyle changed the title ~~(WIP)(MODULES-6895) Add Trigger::V1::Day class~~ (WIP)(MODULES-6895) Refactor provider code to Trigger Mar 22, 2018

Iristyle requested a review from michaeltlombardi March 22, 2018 18:41

Iristyle force-pushed the MODULES-6895-refactor-provider-helpers-to-Trigger branch from 2808fb8 to 0a1b1e0 Compare March 22, 2018 23:59

Iristyle force-pushed the MODULES-6895-refactor-provider-helpers-to-Trigger branch 5 times, most recently from 0ef2369 to 5f01088 Compare April 17, 2018 18:53

michaeltlombardi reviewed Apr 17, 2018

View reviewed changes

glennsarti reviewed Apr 18, 2018

View reviewed changes

Iristyle force-pushed the MODULES-6895-refactor-provider-helpers-to-Trigger branch from 860b590 to 5614fda Compare April 19, 2018 23:43

Iristyle commented Apr 20, 2018

View reviewed changes

Iristyle force-pushed the MODULES-6895-refactor-provider-helpers-to-Trigger branch 2 times, most recently from 116e3ef to 8aceb93 Compare April 20, 2018 22:28

Iristyle changed the title ~~(WIP)(MODULES-6895) Refactor provider code to Trigger~~ (MODULES-6895) Refactor provider code to Trigger Apr 20, 2018

michaeltlombardi approved these changes Apr 20, 2018

View reviewed changes

glennsarti approved these changes Apr 23, 2018

View reviewed changes

Iristyle added 10 commits April 23, 2018 07:48

(MODULES-6895) V1::Day helpers for names / vals

73a3d31

- Move constant arrays from provider code to the V1::Day class as they're not used outside of the class, and the logic for setting up the internal V1 trigger struct can be simplified.

(MODULES-6895) Day#days_of_week_from_bitfield

ea09582

- Move days_of_week_from_bitfield from taskscheduler_api2 to V1::Day. Remove unnecessary day_of_week_constant_to_name method

(MODULES-6895) Remove dead scheduler_occurrences

b005159

- This code is not consumed by anything

Iristyle added 15 commits April 23, 2018 07:48

(MODULES-6895) dummy_time_trigger -> V1::Trigger

9789196

- Simple method extraction - Modify tests to use new helper

(MODULES-6895) Rename V1.dummy_time_trigger

1e357db

- Rename V1.dummy_time_trigger to V1.time_trigger_once_now - Remove unnecessary usage of Win32 constant

(MODULES-6895) Remove Win32 constants from Trigger

6010752

- Create a Flag class to scope the values and use them internally within the V1::Trigger class

(MODULES-6895) time_trigger_once_now -> default_trigger_settings_for

1d2939a

- Create default triggers based on specific schedule types. This results in a reduction of code for mapping specific manifests into their corresponding internal trigger hash structures.

(MODULES-6895) Tighten up from_manifest_hash

cd3d3c0

- Start with schedule type specific conversions, then move to more universal trigger settings - Remove unnecessary code so that its simpler / easier to understand

(MODULES-6895) Move trigger specs to unit

99f4d67

- There are no integration tests in this file, so relocate to unit

Iristyle force-pushed the MODULES-6895-refactor-provider-helpers-to-Trigger branch from f04194f to 99f4d67 Compare April 23, 2018 14:49

RandomNoun7 merged commit 24a0691 into puppetlabs:master Apr 23, 2018

Iristyle deleted the MODULES-6895-refactor-provider-helpers-to-Trigger branch April 23, 2018 17:21

florindragos added the enhancement label Aug 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-6895) Refactor provider code to Trigger #21

(MODULES-6895) Refactor provider code to Trigger #21

Iristyle commented Mar 22, 2018 •

edited

Loading

michaeltlombardi Apr 17, 2018

glennsarti Apr 18, 2018

Iristyle Apr 18, 2018

glennsarti left a comment

glennsarti Apr 18, 2018

glennsarti Apr 18, 2018

Iristyle Apr 18, 2018

glennsarti Apr 18, 2018

Iristyle Apr 18, 2018

glennsarti Apr 18, 2018

Iristyle Apr 18, 2018

Iristyle Apr 18, 2018 •

edited

Loading

michaeltlombardi Apr 18, 2018

glennsarti Apr 18, 2018

Iristyle Apr 20, 2018

glennsarti Apr 20, 2018

michaeltlombardi left a comment

Iristyle commented Apr 23, 2018

glennsarti commented Apr 23, 2018 •

edited

Loading

glennsarti left a comment

Iristyle commented Apr 23, 2018

(MODULES-6895) Refactor provider code to Trigger #21

(MODULES-6895) Refactor provider code to Trigger #21

Conversation

Iristyle commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iristyle Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeltlombardi left a comment

Choose a reason for hiding this comment

Iristyle commented Apr 23, 2018

glennsarti commented Apr 23, 2018 • edited Loading

glennsarti left a comment

Choose a reason for hiding this comment

Iristyle commented Apr 23, 2018

Iristyle commented Mar 22, 2018 •

edited

Loading

Iristyle Apr 18, 2018 •

edited

Loading

glennsarti commented Apr 23, 2018 •

edited

Loading