dsccommunity / ComputerManagementDsc

DSC resources for for configuration of a Windows computer. These DSC resources allow you to perform computer management tasks, such as renaming the computer, joining a domain and scheduling tasks as well as configuring items such as virtual memory, event logs, time zones and power settings.
https://dsccommunity.org
MIT License
309 stars 82 forks source link

BREAKING CHANGE: ScheduledTask: Update and fix existing task trigger parameters #437

Closed Borgquite closed 1 month ago

Borgquite commented 1 month ago

Pull Request (PR) description

Updates and fixes various issues with task trigger parameters: In theory we are only adding new interfaces but due to general ScheduledTask flakiness this is marked as a BREAKING CHANGE.

This Pull Request (PR) fixes the following issues

Task list


This change is Reviewable

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89%. Comparing base (62ba944) to head (7d0df88). Report is 1 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/437/graphs/tree.svg?width=650&height=150&src=pr&token=n7Sx5K7YGT&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity)](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) ```diff @@ Coverage Diff @@ ## main #437 +/- ## =================================== Coverage 88% 89% =================================== Files 19 19 Lines 1797 1831 +34 =================================== + Hits 1598 1642 +44 + Misses 199 189 -10 ``` | [Flag](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/437/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/437?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [...Resources/DSC\_ScheduledTask/DSC\_ScheduledTask.psm1](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/437?src=pr&el=tree&filepath=source%2FDSCResources%2FDSC_ScheduledTask%2FDSC_ScheduledTask.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL0RTQ1Jlc291cmNlcy9EU0NfU2NoZWR1bGVkVGFzay9EU0NfU2NoZWR1bGVkVGFzay5wc20x) | `91% <100%> (+3%)` | :arrow_up: |
Borgquite commented 1 month ago

Still working on this - will hope to get more examples & sort tests imminently.

PlagueHO commented 1 month ago

@dan-hughes - It'll be a BREAKING CHANGE if the interface changes would break existing configurations. Otherwise, it would just be a minor number increase. Looking through the change (briefly) the interface changes seemed to be mostly additive - but adding new parameters can still cause configs to break (or change behaviour).

My recommendation for this one though is to make it a BREAKING CHANGE. ScheduleTask is one of the most nitpicky resources (because of the different ways the Windows team implemented it - check the backlog of issues down to different implementations in Windows). It has a tendency to have various side effects and undocumented behaviors -especially between versions of the OS. So, I'd probably err on the side of caution for this one and just make it a BREAKING CHANGE. Rather be safe than sorry :grinning:

If I have time to review (and it's still open), come the weekend I'll have time.

Borgquite commented 1 month ago

@PlagueHO That makes sense. I have noted how weird the implementation seems to be. If I were starting from scratch it might have been better to implement the whole thing using Register-ScheduledTask and New-CimInstance (e.g. my example here) https://stackoverflow.com/a/78703118/12872270 as I'm guessing doing it at a low-level WMI/CIM objects might have circumvented the various weirdness and missing features in the abstraction layer the PowerShell New-ScheduledTask* cmdlets add. Of course that wouldn't avoid any OS-level issues. Nevermind. At least we're getting near to feature completeness now.

dan-hughes commented 1 month ago

Reviewable status: 15 of 17 files reviewed, 2 unresolved discussions (waiting on @dan-hughes)

_source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1 line 2125 at r3 (raw file):_

Previously, dan-hughes (Daniel Hughes) wrote… Have fully qualified - but if I remove [System.String], it stops Test-TargetResource returning true when resource is in the desired state (which is why it was in there in the first place) ? https://dev.azure.com/dsccommunity/ComputerManagementDsc/_build/results?buildId=9571&view=logs&j=7d3af221-dd98-540f-35f6-de201108da76&t=0a82f033-7b8c-5c12-0fd5-dbd7a46d75ab

Probably due to the schema being [System.String]. If it's needed then so be it.

_source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1 line 23 at r4 (raw file):_

Previously, dan-hughes (Daniel Hughes) wrote… Done. (And glad you asked me to do it - made me see that StateChange has a couple of outlying values at the end!) :) https://learn.microsoft.com/en-us/windows/win32/taskschd/sessionstatechangetrigger-statechange

😊

dan-hughes commented 1 month ago

@johlju, @PlagueHO, review complete.

Borgquite commented 1 month ago

(Fixing this uncovered another bug in my integration tests which was being masked by the previous behaviour so there's a change in those too)

johlju commented 1 month ago

Merging this. As it is a breaking change, any other issues can be resolved in other PRs 🙂