ScheduledTask: Removing helper function Disable-ScheduledTask #228
Conversation
A built in function with the same name exists that works fine on all modern versions of Windwos (2012R2 and never). This helper function does not work with all scheduled tasks.
|
|
Codecov Report
@@ Coverage Diff @@
## dev #228 +/- ##
===================================
- Coverage 88% 87% -1%
===================================
Files 12 12
Lines 1398 1395 -3
===================================
- Hits 1231 1226 -5
- Misses 167 169 +2 |
Removing tests that are no longer needed after Disable-ScheduledTask helper function was removed.
One more test removed relating to Disable-ScheduledTask
Updated CHANGELOG.md
Correcting errors in CHANGELOG
Adding fixed issue reference
Fixing errors in changelog
Correcting the errors in changelog again
EDockus
changed the title
Trying to fix whatever is wrong with formating in changelog.md
|
Hi @EDockus - I'll review this once the tests are passing. Looks like there are some long lines in the CHANGELOG.MD: Lines 6/7: https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/25584560?fullLog=true#L813 Suggest using Visual Studio Code and adding the markdownlint extension - it'll highlight any lines that violate markdown rules. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 2 files at r2, 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EDockus)
DSCResources/MSFT_ScheduledTask/MSFT_ScheduledTask.psm1, line 2076 at r4 (raw file):
} <#
FYI, I'm not sure why I bothered to add this function in the first place?
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 168 at r4 (raw file):
} It 'Should remove the scheduled task in the set method' {
Why is this assertion being removed?
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 213 at r4 (raw file):
} It 'Should remove the scheduled task in the set method' {
Why is this assertion being removed?
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1547 at r4 (raw file):
} It 'Should disable the scheduled task in the set method' {
This assertion should be to Disable-ScheduledTask rather than being removed completely as we're no longer executing the test at all.
PlagueHO
added
waiting for code fix
Trying once again to fix the errros in the changelog file.
|
Can you rebase this one @EDockus to resolve conflicts and we'll work on getting it through. |
Hi, I've done that now. I'm not exactly sure how to update the unit tests, I assumed those particular ones were no longer needed as functionality was moved off the helper function onto the built-in function which has it's own tests. |
|
Hi @EDockus - sorry about this, but I missed your comments. I'll take a look at the current state and advise what needs to be done. Can you resolve the conflicts? |
There was a problem hiding this comment.
Hi @EDockus - I think you might need to rebase against the PowerShell/ComputerManagementDsc Dev branch as there seems to be some corruption in the CHANGELOG.MD. Could you rebase onto Dev rather than merge (see https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts).
Can you also have a look at my comments below? 😀
Reviewed 1 of 1 files at r6, 1 of 1 files at r8.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @EDockus)
CHANGELOG.md, line 9 at r8 (raw file):
of the built-in scheduled tasks. Removing the function means that built-in one is used instead which works as expected - Fixes [Issue #137](https://github.com/PowerShell/ComputerManagementDsc/issues/137). - xComputer:
I think this line and the next 2 should be removed.
CHANGELOG.md, line 10 at r8 (raw file):
- Fix for 'directory service is busy' error when joining a domain and renaming a computer when JoinOU is specified - Fixes [Issue #221](https://github.com/PowerShell/ComputerManagementDsc/issues/221). - Added new resource SmbShare
I think this section has been removed accidentally.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9.
Reviewable status:complete! all files reviewed, all discussions resolved
CHANGELOG.md, line 10 at r8 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this section has been removed accidentally.
It looks like this is still missing.
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 168 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Why is this assertion being removed?
It looks like this is still missing.
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 213 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Why is this assertion being removed?
It looks like this is still missing.
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1547 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This assertion should be to Disable-ScheduledTask rather than being removed completely as we're no longer executing the test at all.
It looks like this is still missing.
|
Sorry @EDockus - more conflicts. Can you fix and I'll try and get this merged ASAP! Sorry about that. |
There was a problem hiding this comment.
Just some really minor tweaks @EDockus
Reviewed 1 of 1 files at r11, 2 of 2 files at r12.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EDockus)
CHANGELOG.md, line 61 at r12 (raw file):
linked to it from README.MD instead. - Updated test header for all unit tests to version 1.2.4. - Updated test header for all imtegration to version 1.3.3.
One minor thing - can you correct the spelling here?
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 168 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It looks like this is still missing.
Can we add these assertions back in?
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 213 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It looks like this is still missing.
Can we add these assertions back in?
Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1547 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It looks like this is still missing.
Can we add these assertions back in?
|
Hi @EDockus - sorry about the delay on this one - are you still working on it? |
Pull Request (PR) description
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