Skip to content

ScheduledTask: Add support for event based triggers#169

Merged
PlagueHO merged 5 commits intodsccommunity:devfrom
danielboth:ScheduledTaskOnEventTrigger
Jul 22, 2018
Merged

ScheduledTask: Add support for event based triggers#169
PlagueHO merged 5 commits intodsccommunity:devfrom
danielboth:ScheduledTaskOnEventTrigger

Conversation

@danielboth
Copy link
Copy Markdown
Member

@danielboth danielboth commented Jul 3, 2018

Pull Request (PR) description
Adding the OnEvent ScheduleType to ScheduledTask to enable the use of event based triggers in the ScheduledTask resource.

This Pull Request (PR) fixes the following issues:
ScheduledTask: Add support to trigger on an event Fixes #167

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 3, 2018

Codecov Report

Merging #169 into dev will decrease coverage by <1%.
The diff coverage is 89%.

Impacted file tree graph

@@        Coverage Diff         @@
##           dev   #169   +/-   ##
==================================
- Coverage   90%    90%   -1%     
==================================
  Files        7      7           
  Lines      691    707   +16     
==================================
+ Hits       623    637   +14     
- Misses      68     70    +2

@johlju johlju requested a review from PlagueHO July 4, 2018 04:02
@johlju johlju added the needs review The pull request needs a code review. label Jul 4, 2018
@PlagueHO
Copy link
Copy Markdown
Member

HI @danielboth - sorry for taking so long! I'll have time tomorrow night to get through this. Sorry again, and thanks for submitting this!

@PlagueHO
Copy link
Copy Markdown
Member

Sorry for not getting to this @danielboth - I haven't forgotten I promise. I've cleared tomorrow night after work to complete this review. Sorry again.

Copy link
Copy Markdown
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

This is really great stuff @danielboth - just some minor tweaks. Is it also possible to create an integration test for the new trigger type?

Reviewed 4 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @danielboth)


CHANGELOG.md, line 13 at r2 (raw file):

- Changed the scope from Global to Script ComputerManagementDsc.Common.Tests.ps1
- ScheduledTask:
  - Added support for event based triggers, implemented using the ScheduleType OnEvent

Could you please add a link to the issue that this fixes (e.g. see the format below - fixes [Issue #xxx](https://github.com/PowerShell/ComputerManagementDsc/issues/xxx)


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 207 at r2 (raw file):

    .PARAMETER EventSubscription
        The event subscription in a string that can be parsed as valid XML. This parameter is only
        valid in combination with the OnEvent Schedule Type.

Is there a link to a Microsoft document we could provide here describing the schema of this XML?


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 695 at r2 (raw file):

    .PARAMETER EventSubscription
        The event subscription in a string that can be parsed as valid XML. This parameter is only
        valid in combination with the OnEvent Schedule Type.

Is there a link to a Microsoft document we could provide here describing the schema of this XML?


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 945 at r2 (raw file):

        }

        if ($ScheduleType -eq 'OnEvent' -and -not([xml]$EventSubscription))

Can you add a space after the -not and before the (


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1458 at r2 (raw file):

    .PARAMETER EventSubscription
        The event subscription in a string that can be parsed as valid XML. This parameter is only
        valid in combination with the OnEvent Schedule Type.

Is there a link to a Microsoft document we could provide here describing the schema of this XML?


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1661 at r2 (raw file):

    if ($PSBoundParameters.ContainsKey('RandomDelay'))
    {
        if($ScheduleType -eq 'OnEvent')

Can you add a space after the if and before the open bracket?


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1664 at r2 (raw file):

        {
            # A random delay is not supported when the ScheduleType is set to OnEvent.
            $null = $PSBoundParameters.Remove('RandomDelay')

Can we add a Write-Verbose message here indicating that the RandomDelay parameter was ignored because it is not supported in when OnEvent is used?


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.schema.mof, line 45 at r2 (raw file):

    [Write, Description("Specifies the level of user rights that Task Scheduler uses to run the tasks that are associated with the principal. Defaults to 'Limited'."), ValueMap{"Limited","Highest"}, Values{"Limited","Highest"}] String RunLevel;
    [Write, Description("Specifies the security logon method that Task Scheduler uses to run the tasks that are associated with the principal."), ValueMap{"Group","Interactive","InteractiveOrPassword","None","Password","S4U","ServiceAccount"}, Values{"Group","Interactive","InteractiveOrPassword","None","Password","S4U","ServiceAccount"}] String LogonType;
    [Write, Description("Specifies the EventSubscription in XML. This can be easily generated using the Windows Eventlog Viewer. Can only be used in combination with ScheduleType OnEvent")] String EventSubscription;

Is there a link to a Microsoft document we could provide here describing the schema of this XML?


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/en-US/MSFT_ScheduledTask.strings.psd1, line 24 at r2 (raw file):

    RemovePreviousScheduledTaskMessage = Removing previous scheduled task '{0}' from '{1}'.
    CreateNewScheduledTaskMessage = Creating new scheduled task '{0}' in '{1}'.
    ConfigureTaskEventTrigger = Setting up an event based trigger on task {0}

Can you add a full stop to the end of this message?


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1559 at r2 (raw file):

                    EventSubscription = '<QueryList><Query Id="0" Path="System"><Select Path="System">*[System[Provider[@Name=''User32''] and EventID=1600]]</Select></Query></QueryList>'
                    Delay             = '00:01:00'
                    Enable            = $True

Can you change to lower case for $true - and throughout.


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1597 at r2 (raw file):

            }

            Context 'When a scheduled task with an OnEvent scheduletype is needs to be created' {

Can you remove the is?

Copy link
Copy Markdown
Member Author

@danielboth danielboth left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 11 unresolved discussions (waiting on @PlagueHO)


CHANGELOG.md, line 13 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you please add a link to the issue that this fixes (e.g. see the format below - fixes [Issue #xxx](https://github.com/PowerShell/ComputerManagementDsc/issues/xxx)

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 207 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is there a link to a Microsoft document we could provide here describing the schema of this XML?

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 695 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is there a link to a Microsoft document we could provide here describing the schema of this XML?

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 945 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the -not and before the (

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1458 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is there a link to a Microsoft document we could provide here describing the schema of this XML?

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1661 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the if and before the open bracket?

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1664 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we add a Write-Verbose message here indicating that the RandomDelay parameter was ignored because it is not supported in when OnEvent is used?

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.schema.mof, line 45 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is there a link to a Microsoft document we could provide here describing the schema of this XML?

Done.


Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/en-US/MSFT_ScheduledTask.strings.psd1, line 24 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a full stop to the end of this message?

Done.


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1559 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you change to lower case for $true - and throughout.

Done.


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1597 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove the is?

Done.

@danielboth danielboth force-pushed the ScheduledTaskOnEventTrigger branch from 97f6f1b to 4d5ad64 Compare July 20, 2018 17:43
Copy link
Copy Markdown
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @danielboth)


appveyor.yml, line 7 at r3 (raw file):

init:
  - ps: iex ((new-object net.webclient).DownloadString('https://raw.githubusercontent.com/appveyor/ci/master/scripts/enable-rdp.ps1'))

Is this required (RDP) or was this something used in debugging/diagnostics? And line below? If it is required for diagnostics of the resource, it might be worth adding a comment detailing this.

@danielboth
Copy link
Copy Markdown
Member Author

Hi @PlagueHO, while setting up the integration tests I discovered that the output of New-ScheduledTaskTrigger is different between W2012R2 and W2016. On W2016 the output of the delay is a time formatted string (like PT30S) but on W2012R2 this is a timespan. I build the trigger around the W2016 behaviour so need to add some code to fix this on W2012R2.

… way to get a DateInterval string to use System.Xml.XmlConvert
@danielboth danielboth force-pushed the ScheduledTaskOnEventTrigger branch from 691661c to ad01864 Compare July 21, 2018 22:23
Copy link
Copy Markdown
Member Author

@danielboth danielboth left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @PlagueHO and @danielboth)


appveyor.yml, line 7 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is this required (RDP) or was this something used in debugging/diagnostics? And line below? If it is required for diagnostics of the resource, it might be worth adding a comment detailing this.

This was just for debugging purposes, I already rolled back to the original version. The problem with converting to a dateinterval string (like PT30S) was solved by using System.Xml.XmlConvert method instead.

Copy link
Copy Markdown
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Awesome stuff @danielboth

:lgtm:

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@PlagueHO PlagueHO merged commit 550074c into dsccommunity:dev Jul 22, 2018
@johlju johlju removed the needs review The pull request needs a code review. label Jul 22, 2018
@danielboth danielboth deleted the ScheduledTaskOnEventTrigger branch July 22, 2018 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ScheduledTask: Add support to trigger on an event

4 participants