ScheduledTask: Resolved inconsistent treatment of BuiltInAccount property. #313
Conversation
Added missing "NT Authority\" prefix to BuiltInAccount property in the resource Test method. Added test cases using the ServiceAccount logonType and BuiltInAccount properties.
PlagueHO
added
needs review
|
Hi @wm-wilson - thanks for submitting this! Great submission. It looks like the tests are failing. Can you take a look at this? |
Fixed unit test 'When a scheduled task is created using a Built In Service Account' - the Get-ScheduledTask mock needed updating to match the current observed behaviour.
|
@PlagueHO - sorry about that, fixed the unit test to match the current observed behaviour from Get-ScheduledTask. |
|
Apologies if I've missed some protocol here. I submitted the test fix 12 days ago but the pull request is still labelled as "waiting for codefix". Is there something else I need to do to indicate that this is ready for further review? |
PlagueHO
removed
the
waiting for code fix
|
Hi @wm-wilson - sorry about the delay. It is waiting on me to complete the review. I'll get onto this over the weekend. Been a bit snowed under with projects. |
There was a problem hiding this comment.
Looking great @wm-wilson - just one minor tweak and we're good to go. Thanks for submitting this.
Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wm-wilson)
CHANGELOG.md, line 8 at r2 (raw file):
## [Unreleased] - ScheduledTask:
Can you add a ### Added (or ### Fixed) header above this?
CHANGELOG.md, line 9 at r2 (raw file):
- ScheduledTask: - Added missing 'NT Authority\' domain prefix when testing tasks that use the BuiltInAccount property.
Could you also add: - Fixes [Issue #317](https://github.com/dsccommunity/ComputerManagementDsc/issues/317). to the end of the change here? See
Moved changelog entry from the [Unreleased] to the Fixed heading. Linked to the issue fixed by this change.
|
@PlagueHO - Hopefully I got that right. No worries re. the delay - I suspect we've all got lots of plates spinning right now! |
|
PEBKAC |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @wm-wilson)
CHANGELOG.md, line 48 at r3 (raw file):
error in `Test-TargetResource` - Fixes [Issue #297](https://github.com/dsccommunity/ComputerManagementDsc/issues/297). - ScheduledTask: - Added missing 'NT Authority\' domain prefix when testing tasks that use the BuiltInAccount property.
Nitpick: Can you append the - Fixes onto the end of the line above (e.g. so it is clear it is related). E.g.
- Updated to use continuous delivery pattern using Azure DevOps - Fixes
[Issue #295](https://github.com/dsccommunity/ComputerManagementDsc/issues/295).
There was a problem hiding this comment.
I suspect we've all got lots of plates spinning right now!
Haha yes, indeed we do! But we'll get there. 😁 just one tweak and I'll merge!
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @wm-wilson)
Moved line break in Changelog entry to make it clearer that the fix and issue are related.
|
Changelog corrected. Thanks! |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @wm-wilson)
CHANGELOG.md, line 47 at r4 (raw file):
- Fix `A parameter cannot be found that matches parameter name 'Ensure'.` error in `Test-TargetResource` - Fixes [Issue #297](https://github.com/dsccommunity/ComputerManagementDsc/issues/297). - ScheduledTask:
Strange - this seems to have moved down to the [8.0.0] section rather than under unreleased? Can you move it up? Sorry about the pain -not sure what happened there...?
Moved WindowsCapability fix back to [Unreleased] at the request of PlagueHO
|
Done! |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @wm-wilson)
CHANGELOG.md, line 8 at r5 (raw file):
## [Unreleased] - WindowsCapability:
Doh! Sorry @wm-wilson - it needs to be under a section called ### Fixed
E.g. what we should have is:
## [Unreleased]
### Fixed
- Fix `A parameter cannot be found that matches parameter name 'Ensure'.`
error in `Test-TargetResource` - Fixes [Issue #297](https://github.com/dsccommunity/ComputerManagementDsc/issues/297).
## [8.0.0] - 2020-02-14
The reason for all this is that the automated deployment code uses this specific structure to generate the release notes.
There was a problem hiding this comment.
Hi @wm-wilson - I merged another PR and so it might have changed my example below, so you should end up with:
## [Unreleased]
### Added
- ComputerManagementDsc
- Added build task `Generate_Conceptual_Help` to generate conceptual help
for the DSC resource.
- Added build task `Generate_Wiki_Content` to generate the wiki content
that can be used to update the GitHub Wiki.
### Changed
- ComputerManagementDsc
- Updated CI pipeline files.
- No longer run integration tests when running the build task `test`, e.g.
`.\build.ps1 -Task test`. To manually run integration tests, run the
following:
```powershell
.\build.ps1 -Tasks test -PesterScript 'tests/Integration' -(needs review)
Fixed
- Updated to use continuous delivery pattern using Azure DevOps - Fixes
Issue #295.
[8.0.0] - 2020-02-14
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @wm-wilson)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status:complete! all files reviewed, all discussions resolved
Pull Request (PR) description
Fixed inconsistent treatment of the BuiltInAccount property for the ScheduledTask resource.
The Set method prefixes the "SYSTEM", "LOCAL SERVICE", "NETWORK SERVICE" account names with the "NT Authority" domain.
This results in a scheduled task that runs, but the DSC resource test will always fail on the BuiltInAccount property check. For example:
Expected = 'SYSTEM'
Actual = 'NT Authority\SYSTEM.'
Added integration test cases for the ServiceAccount Logontype/BuiltInAccount property, demonstrating the above.
This Pull Request (PR) fixes the following issues
None
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is