(MODULES-7150) Refactor Adapter class state management #35

Iristyle · 2018-05-04T22:50:20Z

Builds on #27, which should be merged prior

Old code would insert a dummy trigger into brand new tasks
created for the first time.

Since they are always immediately removed, and since there is no
need to insert a dummy trigger, remove that from the code.

Iristyle · 2018-05-09T00:20:00Z

lib/puppet_x/puppetlabs/scheduled_task/taskscheduler2_task.rb

@@ -51,7 +51,7 @@ def activate(task_name)
    @definition = TaskScheduler2.task_definition(@task)
    @task_password = nil

-    @task


Still need to run some manifests are this one, and make sure there are no callers of activate using the old result.

Only callers here are tests, and they can be modified at will.

Iristyle · 2018-05-09T00:20:05Z

lib/puppet_x/puppetlabs/scheduled_task/taskscheduler2_v1task.rb

@@ -49,7 +49,7 @@ def activate(task_name)
    @definition = TaskScheduler2.task_definition(@task)
    @task_password = nil

-    @task


Still need to run some manifests are this one, and make sure there are no callers of activate using the old result.

Only callers here are tests, and they can be modified at will.

- Old code would insert a dummy trigger into brand new tasks created for the first time. Since they are always immediately removed, and since there is no need to insert a dummy trigger, remove that from the code. - Update tests, which were the only callers for this workflow

- work_item is a completely non-descriptive name, and matches up with the task_name parameter of #new_work_item, so rename it to task_name

- There is no reason for the #tasks method to be require a task instance be created before use. Instead, make the method class level, so that instead of accessing it like TaskScheduler2.new.tasks It can be accessed like: TaskScheduler.tasks

- There is no reason for the #exists? method to require a task instance be created before use. Instead, make the method class level, so that instead of accessing it like TaskScheduler2.new.exists?('foo') It can be accessed like: TaskScheduler.exists?('foo') - Add comments to tests where stubbing exists? is necessary to allow specs to run in non-Windows environments - Remove cases where stubbing :exists? is not needed or where it is otherwise implemented automatically by the Mocha method_missing handler

- There is no reason for the #normalize_task_name method to require a task instance be created before use. Instead, make the method class level, so that instead of accessing it like TaskScheduler2.new.normalize_task_name('foo') It can be accessed like: TaskScheduler.normalize_task_name('foo') - This is required to be able to make the #delete method class level as well

- There is no reason for the #delete method to require a task instance be created before use. Instead, make the method class level, so that instead of accessing it like TaskScheduler2.new.delete('foo') It can be accessed like: TaskScheduler.delete('foo')

- Since @tasksched was not holding an actual instance, there was no reason to store off a reference to the PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2 module. @tasksched was mostly being used as an alias to shorten the calls to the methods. - Since TaskScheduler2 is in the same namespace already as these adapters / helpers, there is no reason to fully qualify the name, so `@tasksched` can be changed simply to `TaskScheduler2`. Further, PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2 can be written simply as TaskScheduler2

- Presently the #activate method in the TaskScheduler2V1Task and the TaskScheduler2Task classes returns the result of the TaskScheduler2.task(task_path) helper, which is returned by calls to GetFolder, then GetTask. This is an instance of a COM IRegisteredTask interface, documented https://msdn.microsoft.com/en-us/library/windows/desktop/aa381353(v=vs.85).aspx - Change the semantics behind this method to instead return an instance of the adapter class. This will eventually pave the way for removing the strange workflow in use with the #activate method.

- Test subjects have previously been an instance of an adapter, which then requires referring to class methods by subject.class.foo. Instead, make the subject the adapter class itself, so that its more clear when actual instances are being requested and created. - The intent will be to remove some of the `new` calls during additional refactoring

@task

- The clear_task helper is only used internally, and doesn't perform much in the way of utility. It simply resets internal tracking of the @task and @triggers objects, which make the provider behavior slightly counterintuitive Provider instances should always be mapped to a single instance of a resource on the system, and should not be fungible anyhow. Therefore, remove the method and inline the code into the #create method of the provider. - Move tests for #clear_task into the #create method instead, and simplify them in the process to use default trigger creation code, instead of inlining trigger definitions. It's possible these tests may be removed in a future iteration.

@Definition

- Instead of returning a COM TaskDefinition object from the #new_work_item method, return an instance of the corresponding TaskScheduler2Task or TaskScheduler2V1Task. - Since #new_work_item method is only ever called by the initializer, and the initializer will *always* return an instance of its type, there is no need to provide accessibility to the @Definition value representing the TaskDefinition object. It can remain an internal implementation detail.

- #new_work_item and #new_task are aliases for the same thing. Remove references to #new_work_item / replace with the more descriptive #new_task. This factory method will later be moved to the class, but keep it on the instance for now.

- A simpler form for existing code exists, so use it

- Calling #activate against an instance of TaskScheduler2Task or TaskScheduler2V1Task is a workflow that doesn't make a lot of sense. Instead, treat #activate as a factory method, so that it may be called like: ScheduledTask::TaskScheduler2Task.activate('foo') Instead of the alternative that requires an instance first: ScheduledTask::TaskScheduler2Task.new.activate('foo') - This is another step toward fully splitting the state management code inside of the adapter classes - Update all specs accordingly - Note that because the current initializer for the adapter classes only accepts a name, and not all necessary instance variables, that a temporary setup is put in place to pass the variable data into the newly created instance. This will be changed in a subsequent commit that improves the design of the class.

glennsarti

Makes sense so far.

glennsarti · 2018-05-10T02:48:46Z

lib/puppet/provider/scheduled_task/taskscheduler_api2.rb

@@ -175,7 +170,7 @@ def user=(value)
  end

  def create
-    clear_task
+    @triggers   = nil


nit: double space.

- A factory method is unnecessary as the initializer for the adapter types TaskScheduler2Task and TaskScheduler2V1Task are sufficient based on code that exists in this module. There is never a case where a trigger is passed to the ::new_task or ::new methods, so remove support for that. - This should make it easier to change the semantics of ::new so that in addition to creating tasks that don't yet exist, it will also be capable of finding / loading an existing task based on name

- There is very little difference between creating a new adapter instance and finding an existing adapter instance given a name. The ::activate method currently differs by throwing an Error if the name cannot be found, but that is not a behavior that is relied upon, and therefore can be changed. ::activate also uses instance_variable_set on an adapter instance, which is a clunky method for adding state to an instance. - Instead, merge the functionality of creating a new task OR finding an existing task into ::new so that for instance: TaskScheduler2Task.new('foo') Will try to find a task by the name of 'foo', and will create one (but not save it) if it doesn't already exist. While this might normally be considered seaprate concerns that should be separated, in practical terms there are so few callers that it doesn't really matter. It turns out to be simpler for new to find OR create new, as it saves callers an existence check (which in reality is a race condition). - This also simplifies tests / test code

- Move all class methods next to one another

- Rename confusingly named TaskScheduler2V1Task class / file to the simpler V1Adapter with file v1adapter.rb

- Rename confusingly named TaskScheduler2Task class / file to the simpler V2Adapter with file v2adapter.rb

Iristyle · 2018-05-10T23:35:57Z

ok @glennsarti this is ready to go now

glennsarti · 2018-05-11T05:47:06Z

lib/puppet/provider/scheduled_task/taskscheduler_api2.rb

@@ -14,10 +14,9 @@

  def self.instances
    PuppetX::PuppetLabs::ScheduledTask::V2Adapter.tasks.collect do |job_file|


Can probably rename the iterator here as task as job_file has no meaning in the V2 land

glennsarti · 2018-05-11T05:47:23Z

lib/puppet/provider/scheduled_task/win32_taskscheduler.rb

@@ -11,10 +11,9 @@

  def self.instances
    PuppetX::PuppetLabs::ScheduledTask::V1Adapter.tasks.collect do |job_file|


Can probably rename the iterator here as task as job_file has no meaning in the V2 land

glennsarti · 2018-05-11T05:48:29Z

spec/unit/puppet/provider/scheduled_task/win32_taskscheduler_spec.rb

@@ -655,11 +655,9 @@ def time_component

  describe '.instances' do
    it 'should use the list of .job files to construct the list of scheduled_tasks' do


Test name should be modified as .job is no longer part of the test

glennsarti

Only 3 minor renames and I'm good for merge!

- When the V2 API returns task information, it omits the .job bit. There is no compatibility reason to keep this around, as the .job extension is actively added / removed or otherwise ignored. - Further the adapter classes have #save methods which accept a file parameter, but never use it. Update the method to remove the argument, and removes docs referencing anything about file extensions - Tech debt cleanup, aisle 3

- For the sake of tests, the V1Adapter.new and V2Adapter.new methods did not require a task_name to be specified. Given Mocha has a responds_like_instance_of helper that can be used in lieu of the responds_like helper, which doesn't require creating an instance without a name, support for that code path in the initializer can be removed. This simplifies adapter object construction.

- Previously, if there was an error trying to find a task given a specific name, TaskScheduler2.task would throw a Win32OLERuntimeError corresponding to "not found". Change the behavior slightly so that the "not found" is handled and instead a `nil` value is returned. - This avoids having to enumerate all the tasks to find one by name, which has demonstrated itself to be problematic when rapidly creating / modifying many tasks in tests. It seems that multiple instances of the COM Schedule.Service cache data and are not always in sync with one another. Calling GetFolder then GetTasks may return instances that have been deleted from other instances of Schedule.Service. This may reduce the risk of encountering such a problem

- Caching instances of Schedule.Service may lead it to be out of sync with the state of tasks on the system. For instance, tests may enumerate over tasks that have already been deleted, yielding to intermittent test failures. - While this may be more expensive, it should be safer.

Iristyle · 2018-05-11T23:53:48Z

Ok @glennsarti / @michaeltlombardi ... I think we're ready to go now. I think the secret sauce here was 989c01e.

I have a few other improvement ideas, but we can wait until later on those.

- Instead of creating new tasks for each type of trigger, create a task once and reuse it for each trigger type.

- Add support for compatibility level 5, which was previously thought to not exist. It was the original compat level introduced in Windows 10, but was updated to 6 in later versions of Windows 10. - The Windows SDK, in taskschd.h, names the compatiblity level constants differently: TASK_COMPATIBILITY_V3 -> TASK_COMPATIBILITY_V2_1 TASK_COMPATIBILITY_V4 -> TASK_COMPATIBILITY_V2_2 TASK_COMPATIBILITY_V6 -> TASK_COMPATIBILITY_V2_4 - Match them up internally with the source of truth that is the Windows SDK - Note that v8.1A of the SDK includes values up through TASK_COMPATIBILITY_V2_2 TASK_COMPATIBILITY_V2_3 and TASK_COMPATIBILITY_V2_4 were added to different versions of the Windows 10 SDK. It's unclear when they were added, but V2_3 is present in SDK 10.0.10586.0. V2_4 is not in SDK 10.0.14393.0, but is in 10.0.17134.0

glennsarti · 2018-05-14T02:30:03Z

Passed through adhoc. Merging.

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch 3 times, most recently from f33f27c to 561d3dd Compare May 8, 2018 22:02

Iristyle commented May 9, 2018

View reviewed changes

Iristyle changed the title ~~(maint) remove unnecessary trigger injection~~ (MODULES-7150) Refactor Adapter class state management May 9, 2018

Iristyle added 12 commits May 9, 2018 14:16

(MODULES-7150) Rename adapter initializer params

4877c4c

- work_item is a completely non-descriptive name, and matches up with the task_name parameter of #new_work_item, so rename it to task_name

(MODULES-7150) Remove adapter #new_work_item

da9fa05

- #new_work_item and #new_task are aliases for the same thing. Remove references to #new_work_item / replace with the more descriptive #new_task. This factory method will later be moved to the class, but keep it on the instance for now.

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 387094b to da9fa05 Compare May 9, 2018 21:24

Iristyle added 2 commits May 9, 2018 14:32

(MODULES-7150) Simplify adapter #task method

df1ae13

- A simpler form for existing code exists, so use it

glennsarti reviewed May 10, 2018

View reviewed changes

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from cdc7503 to 77d2d4b Compare May 10, 2018 18:43

Iristyle added 4 commits May 10, 2018 13:50

(MODULES-7150) Reorganize adapter code

698d7b9

- Move all class methods next to one another

(MODULES-7150) TaskScheduler2V1Task -> V1Adapter

0402b65

- Rename confusingly named TaskScheduler2V1Task class / file to the simpler V1Adapter with file v1adapter.rb

(MODULES-7150) TaskScheduler2Task -> V2Adapter

0560e39

- Rename confusingly named TaskScheduler2Task class / file to the simpler V2Adapter with file v2adapter.rb

michaeltlombardi approved these changes May 10, 2018

View reviewed changes

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 6d34071 to a0b3221 Compare May 10, 2018 23:32

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from a0b3221 to 5899b22 Compare May 10, 2018 23:35

glennsarti reviewed May 11, 2018

View reviewed changes

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 5899b22 to 7805eee Compare May 11, 2018 14:58

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 68fc488 to 39bf3c6 Compare May 11, 2018 21:12

Iristyle added 2 commits May 11, 2018 16:46

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 78dcf5b to 989c01e Compare May 11, 2018 23:48

(MODULES-7150) Rewrite trigger integration test

55bd06a

- Instead of creating new tasks for each type of trigger, create a task once and reuse it for each trigger type.

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 93c9d1e to 55bd06a Compare May 13, 2018 15:56

Iristyle force-pushed the maint-remove-unnecessary-trigger-population branch from 5a86e00 to beff755 Compare May 13, 2018 21:26

glennsarti merged commit e466fbe into puppetlabs:master May 14, 2018

Iristyle deleted the maint-remove-unnecessary-trigger-population branch May 14, 2018 04:50

florindragos added the maintenance label Aug 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-7150) Refactor Adapter class state management #35

(MODULES-7150) Refactor Adapter class state management #35

Iristyle commented May 4, 2018 •

edited

Loading

Iristyle May 9, 2018

Iristyle May 9, 2018

Iristyle May 9, 2018

Iristyle May 9, 2018

glennsarti left a comment

glennsarti May 10, 2018

Iristyle commented May 10, 2018

glennsarti May 11, 2018

Iristyle May 11, 2018

glennsarti May 11, 2018

Iristyle May 11, 2018

glennsarti May 11, 2018 •

edited

Loading

Iristyle May 11, 2018

glennsarti left a comment

Iristyle commented May 11, 2018

glennsarti commented May 14, 2018

		@@ -14,10 +14,9 @@

		def self.instances
		PuppetX::PuppetLabs::ScheduledTask::V2Adapter.tasks.collect do \|job_file\|

		@@ -11,10 +11,9 @@

		def self.instances
		PuppetX::PuppetLabs::ScheduledTask::V1Adapter.tasks.collect do \|job_file\|

		@@ -655,11 +655,9 @@ def time_component

		describe '.instances' do
		it 'should use the list of .job files to construct the list of scheduled_tasks' do

(MODULES-7150) Refactor Adapter class state management #35

(MODULES-7150) Refactor Adapter class state management #35

Conversation

Iristyle commented May 4, 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

glennsarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iristyle commented May 10, 2018

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

glennsarti May 11, 2018 • edited Loading

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

Iristyle commented May 11, 2018

glennsarti commented May 14, 2018

Iristyle commented May 4, 2018 •

edited

Loading

glennsarti May 11, 2018 •

edited

Loading