ScheduledTask: Added BuiltInUser (Issue #130) & Fixed IdleWaitTimeout… #195
Conversation
|
The appveyor build seems to be broken / hanging. When I run it manually I get: I get the same error even when building from an un-modified fork so I don't think it is anything I did. |
|
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again. |
gaelcolas
added
needs review
Codecov Report
@@ Coverage Diff @@
## dev #195 +/- ##
===================================
+ Coverage 90% 90% +<1%
===================================
Files 8 8
Lines 836 857 +21
===================================
+ Hits 754 774 +20
- Misses 82 83 +1 |
Codecov Report
@@ Coverage Diff @@
## dev #195 +/- ##
===================================
+ Coverage 90% 90% +<1%
===================================
Files 8 8
Lines 836 858 +22
===================================
+ Hits 754 775 +21
- Misses 82 83 +1 |
|
Seems to be working now @djwork - I'll review today. |
There was a problem hiding this comment.
Sorry about the terribly long delay @djwork
Just some minor tweaks and queries.
Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @djwork)
CHANGELOG.md, line 35 at r2 (raw file):
- ScheduledTask: - IdleWaitTimeout returned from Get-TargetResource always null - Fixes [Issue #186](https://github.com/PowerShell/ComputerManagementDsc/issues/186).
Can you move this up to the # Unreleased section?
CHANGELOG.md, line 35 at r2 (raw file):
- ScheduledTask: - IdleWaitTimeout returned from Get-TargetResource always null - Fixes [Issue #186](https://github.com/PowerShell/ComputerManagementDsc/issues/186).
I couldn't find where this change was made?
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 284 at r2 (raw file):
$Enable = $true, [ValidateSet('SYSTEM', 'LOCAL SERVICE', 'NETWORK SERVICE')]
Can you add a [Parameter()] attribute to this parameter.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 808 at r2 (raw file):
$Enable = $true, [ValidateSet('SYSTEM', 'LOCAL SERVICE', 'NETWORK SERVICE')]
Can you add a [Parameter()] attribute to this parameter.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1271 at r2 (raw file):
# Prepare the register arguments $registerArguments = @{}
Can you remove this blank line.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1272 at r2 (raw file):
$registerArguments = @{} $username = $null
Can you add a blank line after this one?
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1657 at r2 (raw file):
$Enable = $true, [ValidateSet('SYSTEM', 'LOCAL SERVICE', 'NETWORK SERVICE')]
Can you add a [Parameter()] attribute to this parameter.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1900 at r2 (raw file):
} if ($PSBoundParameters.ContainsKey('WeeksInterval') -and ((-not $currentValues.ContainsKey('WeeksInterval')) -or ($null -eq $currentValues['WeeksInterval'])))
This line is a bit long - can you split it using backticks?
E.g.
if ($PSBoundParameters.ContainsKey('WeeksInterval') `
-and ((-not $currentValues.ContainsKey('WeeksInterval')) -or ($null -eq $currentValues['WeeksInterval'])))Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1903 at r2 (raw file):
{ <# The WeeksInterval parameter is defaulted to 1, even when the
This comment isn't that clear - are you able to reword it? Also it doesn't appear in CHANGELOG.MD?
It isn't completely clear what the purpose of this change is.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1928 at r2 (raw file):
$desiredValues = $PSBoundParameters $desiredValues.TaskPath = $TaskPath if ($desiredValues.ContainsKey('Verbose'))
Can you add a blank line before this one?
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1928 at r2 (raw file):
$desiredValues = $PSBoundParameters $desiredValues.TaskPath = $TaskPath if ($desiredValues.ContainsKey('Verbose'))
I get why this is being done, but what if a Debug parameter is used? Then it'll also have the same problem. Is there a different way of solving this issue?
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 1 at r2 (raw file):
<#
Can you update the example format and name to match the new format?
The name of the example should be 16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1
The header should match as well (except with a new GUID).
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 13 at r2 (raw file):
Configuration Example { param
Can you remove this param block?
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 22 at r2 (raw file):
Import-DscResource -ModuleName ComputerManagementDsc Node $NodeName
Should be Node localhost
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1748 at r2 (raw file):
Set-TargetResource @testParameters Assert-MockCalled -CommandName Register-ScheduledTask -Times 1 -Scope It -ParameterFilter {
Can you remove blank line
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1790 at r2 (raw file):
$testParameters.LogonType = 'Password' It 'Should return true when BuiltInAccount set even if LogonType parameter different' {
Can you add a blank line before this one?
|
CHANGELOG.md, line 35 at r2 (raw file):
Line 549 of MSFT_ScheduledTask.psm1 |
|
CHANGELOG.md, line 35 at r2 (raw file): Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
You have already merged the change in to the dev branch, is it correct to move to unreleased? |
|
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1928 at r2 (raw file):
Can we cross that bridge when we get to it? Otherwise the different way to solve is a smarter Test-DscParameterState function but that is a much more invasive change |
There was a problem hiding this comment.
Reviewable status: 2 of 6 files reviewed, 18 unresolved discussions (waiting on @djwork and @PlagueHO)
CHANGELOG.md, line 41 at r3 (raw file):
aitTimeou
CHANGELOG.md, line 41 at r3 (raw file):
Timeout
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 284 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a [Parameter()] attribute to this parameter.
Done.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 808 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a [Parameter()] attribute to this parameter.
Done.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1271 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove this blank line.
Done.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1272 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line after this one?
Done.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1657 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a [Parameter()] attribute to this parameter.
Done.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1900 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This line is a bit long - can you split it using backticks?
E.g.
if ($PSBoundParameters.ContainsKey('WeeksInterval') ` -and ((-not $currentValues.ContainsKey('WeeksInterval')) -or ($null -eq $currentValues['WeeksInterval'])))
Done.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1903 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This comment isn't that clear - are you able to reword it? Also it doesn't appear in CHANGELOG.MD?
It isn't completely clear what the purpose of this change is.
Done.
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 1 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you update the example format and name to match the new format?
The name of the example should be 16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1
The header should match as well (except with a new GUID).
Done.
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 13 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove this param block?
Done.
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-CreateScheduledTasksAsBuiltinServiceAccount.ps1, line 22 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should be
Node localhost
Done.
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1748 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove blank line
Done.
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1790 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line before this one?
Done.
There was a problem hiding this comment.
Reviewable status: 2 of 6 files reviewed, 18 unresolved discussions (waiting on @PlagueHO)
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1928 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line before this one?
Done.
There was a problem hiding this comment.
I think the strangeness in the CHANGELOG.MD is because you need to rebase onto Dev again. Are you able to do this? That should correct the what I'm seeing that is odd in the Changelog.md. Should then be ready to merge.
Reviewed 3 of 4 files at r4.
Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on @djwork and @PlagueHO)
CHANGELOG.md, line 35 at r2 (raw file):
Previously, djwork wrote…
You have already merged the change in to the dev branch, is it correct to move to unreleased?
Actually no, you're right. Leave this change where it is - I think this got messed up in a previous merge. That is why I couldn't figure out where the change was made.
Modules/ComputerManagementDsc/DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 1928 at r2 (raw file):
Previously, djwork wrote…
$desiredValues.TaskPath = $TaskPathCan we cross that bridge when we get to it? Otherwise the different way to solve is a smarter Test-DscParameterState function but that is a much more invasive change
I'd rather avoid having to mess with too much. I'm ok to leave it as is for now and "cross that bridge" another time.
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1, line 30 at r4 (raw file):
The contents of c:\temp\seeme.txt should be "NETWORK SERVICE". #> Configuration Example
Can you change the name of the sample to match the filename of the example file without the number. E.g. ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, 1 of 4 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @djwork)
…Config -> ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config as requested
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @djwork and @PlagueHO)
Modules/ComputerManagementDsc/Examples/Resources/ScheduledTask/16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1, line 30 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you change the name of the sample to match the filename of the example file without the number. E.g.
ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @djwork)
CHANGELOG.md, line 42 at r5 (raw file):
- ScheduledTask: - IdleWaitTimeout returned from Get-TargetResource always null - Fixes [Issue #186](https://github.com/PowerShell/ComputerManagementDsc/issues/186). - Added BuiltInAccount Property to allow running task as one of the build in
I think this is still in the wrong place - needs to be moved up to # Unreleased.
Once that is sorted I reckon we are ready to go.
Releasing version 6.3.0.0
|
CHANGELOG.md, line 42 at r5 (raw file): Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
rebased my fork from dev does that fix the problem? |
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @djwork and @PlagueHO)
CHANGELOG.md, line 42 at r5 (raw file):
Previously, djwork wrote…
rebased my fork from dev does that fix the problem?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @djwork)
CHANGELOG.md, line 42 at r5 (raw file):
Previously, djwork wrote…
Done.
This is strange- the lines are still showing up under the # 6.2.0.0 section instead of # Unreleased. I wonder if this is because Katie just released the new version to the PS Gallery. Maybe another rebase. Sorry about this!
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @djwork and @PlagueHO)
CHANGELOG.md, line 42 at r5 (raw file):
IdleWaitTimeout
My Fork is currently ahead of dev branch, manually compared the code base for the releases 6.0.0.0 - 6.3.0.0 and the code for BuiltInAccount & IdleWaitTimeout Fix was absent. Therefore moved these entries to ##Unreleased
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)
a discussion (no related file):
previously you asked me to rename .\Examples\Resources\ScheduledTask\16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1 to .\Examples\Resources\ScheduledTask\ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1
I did the rename, but I noticed even after the rebase all the other examples in ".\Examples\Resources" still have the number prefix, ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config is now the odd man out
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @djwork)
a discussion (no related file):
Previously, djwork wrote…
previously you asked me to rename .\Examples\Resources\ScheduledTask\16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1 to .\Examples\Resources\ScheduledTask\ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1
I did the rename, but I noticed even after the rebase all the other examples in ".\Examples\Resources" still have the number prefix, ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config is now the odd man out
@djwork - Almost there.
The filename should have the number prefix: 16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1
But the Configuration name inside the file should not have the number.
E.g.
configuration ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_ConfigDoes that make sense?
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
a discussion (no related file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
@djwork - Almost there.
The filename should have the number prefix: 16-ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_Config.ps1
But the Configuration name inside the file should not have the number.
E.g.configuration ScheduledTask_CreateScheduledTasksAsBuiltInServiceAccount_ConfigDoes that make sense?
Does now, done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @djwork)
a discussion (no related file):
Previously, djwork wrote…
Does now, done
Yep! Good now!
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @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
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is