ScheduledTask: Added BuiltInUser (Issue #130) & Fixed IdleWaitTimeout…

[djwork]

Pull Request (PR) description

ScheduledTask:
Fix to IdleWaitTimeout returned from Get-TargetResource always null
Added BuiltInAccount property to support running task as a builtin account

This Pull Request (PR) fixes the following issues

- Fixes #186 
- Fixes #130 

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in README.md in the resource folder.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov-io]

Codecov Report

Merging #192 into dev will decrease coverage by <1%.
The diff coverage is 68%.

@@        Coverage Diff         @@
##           dev   #192   +/-   ##
==================================
- Coverage   90%    89%   -1%     
==================================
  Files        7      7           
  Lines      739    758   +19     
==================================
+ Hits       671    681   +10     
- Misses      68     77    +9

[PlagueHO]

Looks much better, thanks @djwork

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @djwork)

---

CHANGELOG.md, line 6 at r1 (raw file):

- ScheduledTask:
  - IdleWaitTimeout returned from Get-TargetResource always null - See [Issue #186](https://github.com/PowerShell/ComputerManagementDsc/issues/186).

FYI, This is great stuff! Ideally though we want to separate issues into different PR's so that it makes it easier to review and also if one PR gets held up the other one can still proceed 😁 No worries for this one though- just for future to help my bad memory 😆

---

CHANGELOG.md, line 8 at r1 (raw file):

  - IdleWaitTimeout returned from Get-TargetResource always null - See [Issue #186](https://github.com/PowerShell/ComputerManagementDsc/issues/186).
  - Added Property BuiltInAccount - See [Issue #130](https://github.com/PowerShell/ComputerManagementDsc/issues/130).
    Used for running the Scheduled Task as one of the built in

Great level of detail. but we usually don't include that much in the CHANGELOG.MD. Are you able to reduce it down to something like Added BuiltInAccount Property to allow running task as one of the build in service accounts - Fixes ...

The rest of the text can be removed.

However, you could add some of the detail into the README.MD specific to ScheduledTask so that it ends up in the Wiki. You could also add the limitations around parameters the parameter description itself (in the MOF and the parameter help).

Nitpick, for consistency can use use - Fixes instead of - See?

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 80 at r1 (raw file):

    .PARAMETER BuiltInAccount
        Run the task as  one of the built in service accounts ('SYSTEM', 'LOCAL SERVICE', 'NETWORK SERVICE').

Can you remove the extra space between as and one?

Can you end in full stop.

Can you remove the list of service accounts as they will automatically be generated in the Wiki.

And through out.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 285 at r1 (raw file):

        [ValidateSet('SYSTEM', 'LOCAL SERVICE', 'NETWORK SERVICE')]
        [String]

Can you use [System.String] for consistency (not using type accelerators).

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 549 at r1 (raw file):

            Hidden                          = $settings.Hidden
            RunOnlyIfIdle                   = $settings.RunOnlyIfIdle
            #$settings.IdleSettings.IdleWaitTimeout is always null, changed to WaitTimeout property to avoid Test-TargetResource returning a spurious value

Can you remove this line as we don't need it 😁

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 550 at r1 (raw file):

            RunOnlyIfIdle                   = $settings.RunOnlyIfIdle
            #$settings.IdleSettings.IdleWaitTimeout is always null, changed to WaitTimeout property to avoid Test-TargetResource returning a spurious value
            #IdleWaitTimeout                 = ConvertTo-TimeSpanStringFromScheduledTaskString -TimeSpan $settings.IdleSettings.IdleWaitTimeout

Can you delete this commented out code.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1276 at r1 (raw file):

        if ($PSBoundParameters.ContainsKey('BuiltInAccount'))
        {
            #the validateset on BuiltInAccount has already checked the non null value to be 'LOCAL SERVICE', 'NETWORK SERVICE' or 'SYSTEM'

Can you add a space after the # and start with a capital letter and reduce the line length of the comment to around 80 chars or less (easier to review)

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1305 at r1 (raw file):

        else
        {
            #'NT AUTHORITY\SYSTEM' basically gives the schedule task admin privileges, should we default to 'NT AUTHORITY\LOCAL SERVICE' instead?

Can you add a space after the # and start with a capital letter and reduce the line length of the comment to around 80 chars or less (easier to review)

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1886 at r1 (raw file):

    else
    {
        # must be running as System, login type is ServiceAccount

Can you start comment with capital letter?

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1893 at r1 (raw file):

    if ($PSBoundParameters.ContainsKey('WeeksInterval') -and ((-not $currentValues.ContainsKey('WeeksInterval')) -or ($null -eq $currentValues['WeeksInterval'])))
    {
        #The WeeksInterval parameter is defaulted to 1, even when the property is unset/undefined for the current task returned from Get-TargetResouce

Can you add a space after the # and reduce the line length of the comment to around 80 chars or less (easier to review)

Can you also use comment block for multi line comment?

<#
bla
#>

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1917 at r1 (raw file):

    if ($desiredValues.ContainsKey('Verbose'))
    {
        #initialise a missing or null Verbose to spurious calls to Set-TargetResouce

Can you start comment with capital letter and add a space after #?

---

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

    [Write, Description("Present if the task should exist, Absent if it should be removed"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] string Ensure;
    [Write, Description("True if the task should be enabled, false if it should be disabled")] boolean Enable;
    [Write, Description("Run the task as  one of the built in service accounts ('SYSTEM', 'LOCAL SERVICE', 'NETWORK SERVICE'). When set -ExecuteAsCredential will be ignored and -LogonType will be set to 'SericeAccount'"), ValueMap{"SYSTEM", "LOCAL SERVICE", "NETWORK SERVICE"}, Values{"SYSTEM", "LOCAL SERVICE", "NETWORK SERVICE"}] string BuiltInAccount;

Can you remove the extra space and remove the ('SYSTEM', LOCAL SERVICE',' NETWORK SERIVICE') from the comment?

Can you remove - from beginning of -ExecuteAsCredential and -LogonType?

Can you also end comment in a full stop (I noticed that there are some other parameters that are missing full stop as well - so feel free to correct those 😁

---

Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 3 at r1 (raw file):

<#
    .EXAMPLE
    This example creates a scheduled task called 'TriggerOnServiceFailures' in the folder

This example text doesn't seem to match what is actually being demonstrated.

---

Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 24 at r1 (raw file):

        ScheduledTask ServiceEventManager
        {
            TaskName = 'TaskRunAsNetworkService'

Can you align the = signs (VSCode can easily do this for you using the autoformat).

---

Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1764 at r1 (raw file):

                It 'Should return an error when both the ExecuteAsGMSA an ExecuteAsCredential ar specified' {
                    try

Rather than doing this, can you use the method we use here:
https://github.com/PowerShell/ComputerManagementDsc/blob/dev/Tests/Unit/MSFT_OfflineDomainJoin.Tests.ps1#L70

[djwork]

@johlju @PlagueHO are we still waiting for code fix? I addressed all of the issues from the review last week, are there additional changes required?

[johlju]

@djwork If you go in an write 'Done' on each of the review comments in Reviewable, then it is easier for @PlagueHO to know that you are done and he can review again. It is also possible to resolve the review comments if you comment 'Done' on the comments (otherwise the review comment has to "retracted"). 🙂

Labeling this as 'needs review'.

[djwork]

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

---

CHANGELOG.md, line 8 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Great level of detail. but we usually don't include that much in the CHANGELOG.MD. Are you able to reduce it down to something like Added BuiltInAccount Property to allow running task as one of the build in service accounts - Fixes ...

The rest of the text can be removed.

However, you could add some of the detail into the README.MD specific to ScheduledTask so that it ends up in the Wiki. You could also add the limitations around parameters the parameter description itself (in the MOF and the parameter help).

Nitpick, for consistency can use use - Fixes instead of - See?

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 80 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove the extra space between as and one?

Can you end in full stop.

Can you remove the list of service accounts as they will automatically be generated in the Wiki.

And through out.

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 285 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use [System.String] for consistency (not using type accelerators).

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 549 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove this line as we don't need it 😁

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 550 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you delete this commented out code.

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1276 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the # and start with a capital letter and reduce the line length of the comment to around 80 chars or less (easier to review)

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1305 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the # and start with a capital letter and reduce the line length of the comment to around 80 chars or less (easier to review)

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1886 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you start comment with capital letter?

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1893 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the # and reduce the line length of the comment to around 80 chars or less (easier to review)

Can you also use comment block for multi line comment?

<#
bla
#>

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1917 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you start comment with capital letter and add a space after #?

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove the extra space and remove the ('SYSTEM', LOCAL SERVICE',' NETWORK SERIVICE') from the comment?

Can you remove - from beginning of -ExecuteAsCredential and -LogonType?

Can you also end comment in a full stop (I noticed that there are some other parameters that are missing full stop as well - so feel free to correct those 😁

Done.

---

Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 3 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This example text doesn't seem to match what is actually being demonstrated.

Done.

---

Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 24 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you align the = signs (VSCode can easily do this for you using the autoformat).

Done.

---

Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1764 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Rather than doing this, can you use the method we use here:
https://github.com/PowerShell/ComputerManagementDsc/blob/dev/Tests/Unit/MSFT_OfflineDomainJoin.Tests.ps1#L70

Done.

[PlagueHO]

Just some really minor things to tweak (my fault as I gave you a comment sample that wasn't indented). Then should be good to go.

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @djwork)

---

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

        {
            <#
            The validateset on BuiltInAccount has already checked the non-null

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

---

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

        {
            <#
            'NT AUTHORITY\SYSTEM' basically gives the schedule task admin

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

---

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

    {
        <#
        The WeeksInterval parameter is defaulted to 1, even when the property

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

---

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

    {
        <#
        Initialise a missing or null Verbose to avoid spurious

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

[djwork]

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

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you indent this comment? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-comments

Done.

[PlagueHO]

Code wise I think this is ready to merge, but the difference in code coverage is still being flagged (1% drop with only 68% of the diff covered). @djwork - do you think it is possible to hit some of these lines with some tests: https://codecov.io/gh/PowerShell/ComputerManagementDsc/compare/b41ef83d35bb36ed46d8787b2f3ab1597fd06c16...c6a684cbf16a1c04b7a06e6c1b2961e7f6c84f9e/diff#D6-1879

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[PlagueHO]

Hi @djwork - I think you've got a merge conflict here that needs to be fixed. Will review after that has been corrected.

[djwork]

Going to do the merge on a fresh fork and pull request to keep the commit history clean

[PlagueHO]

@djwork - the only problem with using a fresh PR is that all the review info gets lost and we need to start the review process again. What you can do is perform a squash on your commits. An interactive rebase (git rebase -i ...) is a great way to do this. But no worries for now -we'll just work on the new PR.

Also we can actually squash merge when we merge the PR to clean up the commit history.

[djwork]

Understood, I still need to up my GIT game.