Skip to content

Conversation

@glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Jan 4, 2018

This PR adds a new class and provider which has the same interface as the older V1
Scheduled Task. This adapter class will implement the V1 methods but using the V2 API calls.

This PR also adds tests to ensure that the adapter behaves the same as the V1 puppet type

This PR also adds some basic integration tests, which may subsume the current acceptance tests

@glennsarti
Copy link
Contributor Author

Can't repro the failures locally so will need to break into an appveyor build insitu.

@Iristyle
Copy link
Contributor

Iristyle commented Jan 4, 2018

This is the artist formerly known as PUP-7894 - originally proposed in PR at puppetlabs/puppet#6382

@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch 2 times, most recently from 1599bd2 to 9e3bc8f Compare January 5, 2018 03:09
@glennsarti
Copy link
Contributor Author

@clairecadman When you get a moment, can you give the README a once-over. It's a completely new README so feel free to comment on anything in it, not just my changes.

@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch from 9e3bc8f to ca0d1f8 Compare January 5, 2018 04:59
@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch 2 times, most recently from 609596a to 3feec81 Compare January 5, 2018 07:40
@clairecadman
Copy link

@glennsarti Added a couple of comments.

@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch from 3feec81 to bbab9f6 Compare January 8, 2018 00:27
@glennsarti
Copy link
Contributor Author

@clairecadman I've updated the README.

@@ -0,0 +1,279 @@
# This class is used to manage V1 compatible tasks using the Task Scheduler V2 API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - this is not a Facade, it's an Adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll reword. Aren't metaphors wonderful! People can have different interpretations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded.

# Execute the current task.
#
def run
raise NotImplementedError
Copy link
Contributor

@Iristyle Iristyle Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these raise NotImplementedError can be deleted as they're dead code without any callers. Don't need to handle in this PR, can defer to next release we're cutting...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... this was a tricky one in my head. Do I only implement the minimum methods or make this object a direct replacement for the V1 object. I landed with this so at least if anyone was using those methods, they'd throw Not Implemented as opposed to Method Missing.

But you're right this is moot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know we talked about this before, and then the plan changed a bit along the way. The thing is, to use these methods users would have to know they can swap this adapter in for the old class, which no one can possibly be aware of yet ;0

YAGNI and all that...

This commit adds a new class which has the same interface as the older V1
Scheduled Tasks (puppet/util/windows/taskscheduler.rb).  This adapter class will
implement the V1 methods but using the V2 API calls.  This commit only adds the
minimum required implementations, and the rest with NotImplementedErrors.  These
will be populated in later commits.
This commit adds a non-default provider for the Scheduled Task type but instead
uses the V2 adapter class.  This provider is in essence a verbatim copy of the
original provider (win32_taskscheduler) but changes the helper class to the
V2 API adapter object.  This maintains backwards compatilbity and will allow the
older V1 implementation to be deprecated and then removed easily.
This commit changes the unit tests for the scheduled_task provider by running
the tests against both providers (win32_taskscheduler and
taskscheduler_api2). This ensures that any behaviors are consistent in both
providers.
@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch from bbab9f6 to c06853e Compare January 9, 2018 05:34
@glennsarti glennsarti changed the title (MODULES-6264)(MODULES-6266) Create V2 API and V1 facade (MODULES-6264)(MODULES-6266) Create V2 API and V1 adapter Jan 9, 2018
klass_list.each do |concrete_klass|

if concrete_klass == PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2V1Task
task_provider = :taskscheduler_api2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one thing I'm not totally keen on is that the constants used in this class are still scoped to Win32::TaskScheduler:: rather than concrete_klass::, so we have a bunch of leakage from v1 provider into v2 provider.

Given what this PR is for now, should be OK for the moment... but we should probably bookmark this as something to loop back on / clean up later. I understand why you want the constants to be the same values for the sake of API compat...

FWIW, the names of the providers as old Win32::TaskScheduler and new PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2V1Task don't make it clear that they're related to one another either, but not sure that's something we can or want to change really.

Copy link
Contributor Author

@glennsarti glennsarti Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a TODO: comment on those

We certainly can't change the 'Win32::TaskScheduler' and that's a hangover of removing the win32-taskscheduler gem. I strongly feel that should not changethe name PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2V1Task, as that is definitely where it should be, rather than say 'Win32::TaskScheduler2' or something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we would want to do is move / rename constants into the new world, and provide aliases from old world to new world for backwards compat, with some timeline on removing the old things (i.e. Puppet 6). It's unlikely that anyone outside of Pupet is using the old Win32::TaskScheduler namespace (we can scan Forge to be absolutely certain though).


module TaskScheduler2
# The error class raised if any task scheduler specific calls fail.
# class Error < Puppet::Util::Windows::Error; end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, we've got a clinger!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clinger?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. commented out code.


task_folder = task_service.GetFolder(folder_path)
task_folder.GetTasks(TASK_ENUM_HIDDEN).each do |task|
included = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to simplify how this reads... how about something like?

filter_compatibility = !options[:include_compatibility].empty?
task_folder.GetTasks(TASK_ENUM_HIDDEN).each do |task|
  next if filter_compatibility && !options[:include_compatibility].include?(task.Definition.Settings.Compatibility)
  array << task.Path
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# Private methods
def self.task_service
if @@service_object.nil?
@@service_object = WIN32OLE.new('Schedule.Service')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will an effective singleton here cause us any problems? Are these expensive to create?

Copy link
Contributor Author

@glennsarti glennsarti Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is in a module I'm not sure why I used @@. Should probably just use @

Creating OLE objects is "expensive" but relative to a puppet run, probably not. It just seemed a waste to continuously create and destroy that object.

As a quick trial, I changed the method to always create a new object.
Using shared - Spec tests took 12.00, 11.86, 12.06
Using new object - Specs took 12.09, 12.21, 12.97

On average the specs were 4% slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I've changed it to @

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you started with @@ when this code was a class / before changing to module?

I was mostly just concerned with understanding the appropriate usage pattern for that COM component.... For instance, I was wondering if it's possible to get the object into a state where a failure in one resource ends up putting that object into a state that makes it unable to service other resources in the same run, etc. I would hope that would not be the case, but given the v1 weirdness, thought it was worth asking the question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory yes. It could not recover if the object was in a bad state, whereas if I created and connected on every invocation then it would auto-correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However that service object shouldn't really store state between calls, it's all about;

  1. call get task
  2. modify the object returned using its methods etc.
  3. call set task

I'm happy to change it to create a new object on each call. I suspect the actual time difference is minimal as specs use the code in a different way than a puppet resource.

end
return array unless options[:include_child_folders]

task_folder.GetFolders(0).each do |child_folder|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a tough time figuring out what the 0 meant... probably because this is a terrible API.

Could we at least name the 0 as reserved_flag or add a comment or something?
https://msdn.microsoft.com/en-us/library/windows/desktop/aa381350(v=vs.85).aspx

Otherwise this looks like some sort of count of folders to return or something, which it definitely isn't!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a const called RESERVED_FOR_FUTURE_USE

task_object
end

def self.definition(task_object = nil)
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it too late to name this something else?

I'm concerned that this method is named definition, but that we also have parameters passed to a number of functions that are also named definition. Based on scoping rules, we pick up the arguments in those other functions, but it would be pretty easy to accidentally mess this up in the future. I think no name collision would be preferable to protect ourselves from ourselves...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly naming a variable inside a method the same name as the method is usually not preferred for the same reasons (sometimes leads to stack overflows).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Changed method name to task_definition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprised rubocop didn't complain....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have it turned on? I was actually going to suggest enabling it on this project as a starter to see what happens...

I don't see it in the release_checks task at https://github.com/puppetlabs/puppetlabs_spec_helper/blob/2403747514bc1aecddccde203e79680f8067b9eb/lib/puppetlabs_spec_helper/rake_tasks.rb#L614-L624

end

def self.definition(task_object = nil)
definition = task_service.NewTask(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another weird reserved flag?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more comment on this method... I see it's basically designed to give you either a new definition or to clone an existing definition if you pass a task object. Wondering if it might be better to separate the two into something like empty_definition and task_definition(task) or something like that?

I understand what you were going for... could probably go either way.

Is only existing usage in specs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. Agreed. I broke the (S)olid :-) . Changed to new_task_definition and task_definition(task)

definition.Principal
end

def self.set_principal(definition, user)
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why set_principal rather than principal=?

Ah, nevermind.. this makes sense in the context of a purely functional class method. Comment rescinded.

raise TypeError unless task_path.is_a?(String)
task_folder = task_service.GetFolder(folder_path_from_task_path(task_path))

result = task_folder.DeleteTask(task_name_from_task_path(task_path),0)
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COM wrappers automatically raised for HRESULT < 0 failures on method calls like this (like https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/taskscheduler.rb#L265) with a Puppet::Util::Windows::Error.
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/com.rb#L141-L143

I think we should probably continue to do the same here?

For this one, we should probably file a new ticket / address in the future as it may be a little more involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah...as these are WIN32OLERuntimeError not COM errors things are not as clear cut. The WIN32OLERuntimeError stuff is horrible to work with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I now recall our discussion about the WIN32OLERuntimeError business and the lack of error codes as an attribute of the error instance... so that's a problem for sure.

But in particular I was looking at the == Puppet::Util::Windows::COM::S_OK, which I assumed indicated you got back a HRESULT... However, looking at the docs now, I don't think that that's right - DeleteTask says this method does not return a value, so I think if nothing is raised you just return true at the end:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa382566%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396

This is confirmed by the COM interface docs as well https://msdn.microsoft.com/en-us/library/windows/desktop/aa381345(v=vs.85).aspx

Note: All COM interface methods return a HRESULT status code by convention, and if they have any return values from the method they're actually [out] parameters to pointer-pointers in the IDL. I assume WIN32OLERuntimeError probably already does the FAILED check under the hood for us, so my original point is moot I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional note: I am now a little concerned that this method is always returning false, yet we have no test failures to show for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. competing docs -
https://msdn.microsoft.com/en-us/library/windows/desktop/aa381345(v=vs.85).aspx

Return value
If this method succeeds, it returns S_OK. Otherwise, it returns an HRESULT error code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified delete method

  def self.delete(task_path)
    raise TypeError unless task_path.is_a?(String)
    task_folder = task_service.GetFolder(folder_path_from_task_path(task_path))

    task_folder.DeleteTask(task_name_from_task_path(task_path),0)
  end

and tests

    it 'should delete a task that exists' do
...
    it 'should raise an error for a task that does not exist' do
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the sake of anyone else following the thread, the docs are not competing, just targeted at different consumers. COM methods all returns status HRESULTs as mentioned in #6 (comment)

For COM functions to return, for instance, new objects, they have additional [out] parameters that are passed into the function as pointer pointers... For instance,

HRESULT GetFolder(
  [in]  BSTR        path,
  [out] ITaskFolder **ppFolder
);

Essentially you pass the address of your ITaskFolder * (interface pointer), so that the calling function can write the actual pointer to the memory address you've given when it creates the output object.

Because throwing C++ exceptions out of COM functions is not supported (what would catching a C++ exception in Visual Basic even mean?), status is conveyed by the convention of always returning a HRESULT value for success / fail.

When we use WIN32OLE in Ruby, we're using the scripting compatible interfaces, which support late-binding and going through the IDispatch interface... so when a function has no [out] params in the actual IDL, that means it's basically a void. But the easiest thing to do is just look at the "scripting" docs at https://msdn.microsoft.com/en-us/library/windows/desktop/aa383607(v=vs.85).aspx rather than the C++ docs.

# and nS is the number of seconds (for example, PT5M specifies 5 minutes and P1M4DT2H5M specifies one month,
# four days, two hours, and five minutes)
def self.duration_to_hash(duration)
regex = /^P((?'year'\d+)Y)?((?'month'\d+)M)?((?'day'\d+)D)?(T((?'hour'\d+)H)?((?'minute'\d+)M)?((?'second'\d+)S)?)?$/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I suspect we don't have to worry about this because we always get back the format that we want, with everything specified... but... it does look like something like PT36H is valid... which will cause some of the matches to be nil

[5] pry(main)> regex = /^P((?'year'\d+)Y)?((?'month'\d+)M)?((?'day'\d+)D)?(T((?'hour'\d+)H)?((?'minute'\d+)M)?((?'second'\d+)S)?)?$/
=> /^P((?'year'\d+)Y)?((?'month'\d+)M)?((?'day'\d+)D)?(T((?'hour'\d+)H)?((?'minute'\d+)M)?((?'second'\d+)S)?)?$/
[6] pry(main)> matches = regex.match('PT36H')
=> #<MatchData "PT36H" year:nil month:nil day:nil hour:"36" minute:nil second:nil>
[7] pry(main)> matches['month']
=> nil

I didn't see any problems around this when testing so I think we're OK?

Copy link
Contributor Author

@glennsarti glennsarti Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I'm converting this to a hash, the missing months would be a nil anyway e.g.

xxxxx.duration_to_hash('PT36H')[:months] => nil

So either way, they'll be nil.

Currently the V1 API uses milliseconds not the duration format so the tests are safe. Perhaps when we use the V2 API 'for realz' things may be tripped up e.g.

'PT2M' == 'PT120S' (2 minutes equals 120 seconds)

def activate(task_name)
raise TypeError unless task_name.is_a?(String)
normal_task_name = normalize_task_name(task_name)
raise Error.new(_("Scheduled Task %{task_name} does not exist") % { task_name: normal_task_name }) unless exists?(normal_task_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's already guarded in the new provider
lib/puppet/provider/scheduled_task/taskscheduler_api2.rb:32
@task.activate(resource[:name] + '.job') if exists?

and old provider
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb#L34

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not.. I just noticed that the logic here was a little different that previous helper, especially wrt normalization.

action = nil
action_index += 1
end
return action unless action.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if action.nil? && create_if_missing
  action = @tasksched.create_action(@definition, PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2::TASK_ACTION_EXEC)  
end

action

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

def default_action(create_if_missing = false)
action = nil
action_index = 1
while action_index <= @tasksched.action_count(@definition) do
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little verbose, but a little better...

action = nil

(1..@tasksched.action_count(@definition)) do |i|
  index_action = @tasksched.action(@definition, i)
  action = index_action if index_action.Type == PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2::TASK_ACTION_EXEC
  break if action
end

But I think we can do one better by using lazy here to only create items in the map when we don't get matches - here's the entire method (verified locally but only with an action_count of 1):

def default_action(create_if_missing: false)
  action = (1..@tasksched.action_count(@definition)).lazy.map do |i|
    @tasksched.action(@definition, i)
  end.find do |a|
    a.Type == PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2::TASK_ACTION_EXEC
  end

  if action.nil? && create_if_missing
    action = @tasksched.create_action(@definition, PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2::TASK_ACTION_EXEC)
  end

  action
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the first one. The lazy code looks harder to grok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lazy can be removed in the above btw -- that was an optimization specifically to not fill the map prior to finding the first matching item (which is closer to the behavior of the prior example).

end

# Find the first TASK_ACTION_EXEC action
def default_action(create_if_missing = false)
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def default_action(create_if_missing = false)

Would be clearer at callsites using keyword args like

def default_action(create_if_missing: false)

Then callers use

foo = default_action(create_if_missing: true)

Unnamed bools as params are hard to understand later.

@@ -0,0 +1,9 @@
scheduled_task { 'Run Notepad':
ensure => present,
command => 'C:\Windows\System32\notepad.exe',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just what every system should do at noon every day... launch a notepad!

### Provider

* If you are using Puppet Strings code comments, this Reference section should include Strings information so that your users know how to access your documentation.
* taskscheduler_api2: Adapts the Puppet scheduled_task resource to use the modern Version 2 API.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type has backticks, but this provider does not...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

let(:subject) { PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2 }

describe '#enum_task_names' do
before(:all) do
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to do :all or :each here?

Since these are all reads, :all is fine...

end

describe '#delete' do
before(:all) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably change this from :all to :each in the event more tests are added to this describe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

end

describe 'modify a task' do
before(:all) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for :each instead of :all ... if more tests are added, you end up sharing the task which is probably undesirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

let(:subjectv2) { PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2V1Task.new() }

let(:command) { 'cmd.exe' }
let(:arguments_before) { '/c exit 0' }
Copy link
Contributor

@Iristyle Iristyle Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:command and :arguments_before are never used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah...that's because I moved them to before(:all) and let variables are not available there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


it 'should have same properties in the V1 API' do
subjectv2.activate(@task_name)
subjectv2.parameters = arguments_after
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arguments_after only used here.. probably don't need to be in the let above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch from c06853e to d29cf28 Compare January 11, 2018 06:07
@glennsarti
Copy link
Contributor Author

@Iristyle Comments addressed.

#
def application_name
action = default_action(create_if_missing: false)
action.nil? ? nil : action.Path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be nice when we can use the safe navigation operator!

@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch from d29cf28 to 38b411b Compare January 11, 2018 07:05
Previously Puppet used the V1 API for scheduled tasks however this has been
deprecated in favor of the V2 Win32OLE objects.  This commit adds a module which
can query and modify tasks with the V2 API.  This module only implements the
required methods for the existing Scheduled Task providers, for example it does
not implement methods to run, terminate tasks or get last run information or
exit codes.
Previously a basic implementation of the V2 adapter class was introduced.  This
commit actually implements the required methods for the Scheduled Task
providers.  Note that many methods still raise Not Implemented errors as these
are not required by the Scheduled Task providers to manage state.
Previously the Scheduled Task API helpers did not have any tests to confirm
their behaviour.  This commit adds some basic tests to the Scheduled Task V2
module to ensure it can list, create, delete, and modify tasks on a Windows
system.  This commit also adds integration tests for the V2 facade class to
confirm that tasks created in the V1 API appear in the V2 facade and vice versa.
This commit updates the README for the new taskscheduler_api2 provider.  This
commit also adds example manifests in the examples directory.
@glennsarti glennsarti force-pushed the MODULES-6264-add-tasksched-v2-api branch from 38b411b to 59a0c15 Compare January 11, 2018 07:10
@Iristyle Iristyle merged commit 437a9b9 into puppetlabs:master Jan 11, 2018
Iristyle added a commit to Iristyle/puppetlabs-scheduled_task that referenced this pull request Aug 8, 2018
 - When loading a task by path / name, the code was previously
   creating a new ITaskDefinition instance and copying everything
   from the IRegisteredTask that was found.

   Instead of performing this copy, return the ITaskDefinition
   that is attached to the found task.

 - This code was introduced in 437a9b9
   but is unclear from the PR history why it was needed
   puppetlabs#6

   Current theory is that it may have been an accident
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants