search

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
300 stars 83 forks source link

ScheduledTask: Add 'StopExisting' to valid values for MultipleInstances parameter #335

Closed Rob-S closed 4 years ago

Rob-S commented 4 years ago

Pull Request (PR) description

Add 'StopExisting' to valid values for ScheduledTask MultipleInstances parameter - fixes Issue #333

This Pull Request (PR) fixes the following issues

Fixes Issue #333

Task list

This change is Reviewable

Rob-S commented 4 years ago

The HQRM tests are failing with the error below for all the examples for every resource, saying Use Import-DSCResource to import the resource. However, the examples do import the resources using the module name: Import-DscResource -ModuleName ComputerManagementDsc. Has something gone awry with the tests?

    Context ScheduledTask\1-ScheduledTask_CreateScheduledTaskOnce_Config.ps1
      [-] Should compile MOFs for example correctly 86ms
        Expected no exception to be thrown, but an exception "At D:\a\1\s\source\Examples\Resources\ScheduledTask\1-ScheduledTask_CreateScheduledTaskOnce_Config.ps1:35 char:9
        +         ScheduledTask ScheduledTaskOnceAdd
        +         ~~~~~~~~~~~~~
        Undefined DSC resource 'ScheduledTask'. Use Import-DSCResource to import the resource." was thrown from D:\a\1\s\source\Examples\Resources\ScheduledTask\1-ScheduledTask_CreateScheduledTaskOnce_Config.ps1:35 char:9
            +         ScheduledTask ScheduledTaskOnceAdd
            +         ~~~~~~~~~~~~~.
        141:                     } | Should -Not -Throw
        at <ScriptBlock>, D:\a\1\s\output\RequiredModules\DscResource.Test\0.13.0\Tests\QA\ExampleFiles.common.Tests.ps1: line 27
PlagueHO commented 4 years ago

Hi @Rob-S - this is due to a change to recent ModuleBuilder change. I've got a PR #334 open that will resolve it. Once that is trough the build should work properly again.

PlagueHO commented 4 years ago

You should be able to rebase and resolve conflicts and tests should pass.

Rob-S commented 4 years ago

I had resolved the conflict in the CHANGELOG.md file. Does anything else need done on this?

PlagueHO commented 4 years ago

Hi @Rob-S - sorry, this is on me. I'll review this weekend (this week is a bit crazy with MS Build and so none of my evenings are free).

Rob-S commented 4 years ago

I did try adding a unit test (I took a look at the integration tests, but I'm not really familiar with how those work yet). In the pipeline run, it looks like it's not even trying to run the tests, saying "Cannot bind argument to parameter 'Path' because it is null." although I do see a "Project Path" parameter is set.

PlagueHO commented 4 years ago

Hi @Rob-S - the build failure is caused by a recent release of Module Builder. I've fixed the issue in this PR https://github.com/dsccommunity/ComputerManagementDsc/pull/334 - if you rebase onto master again the build should pass - then we can take a look at the integration tests.

PlagueHO commented 4 years ago

Hi @Rob-S - Sorry, correction, the issue you're seeing is because of the Pester v5.x release. I'm going to pin this module to remain on Pester 4.10.1.

PlagueHO commented 4 years ago

Once this PR https://github.com/dsccommunity/ComputerManagementDsc/pull/337 is through you can rebase and the tests will hopefully pass.

PlagueHO commented 4 years ago

@Rob-S - can you try rebasing your branch onto master again to see if that fixes the build?

PlagueHO commented 4 years ago

Hi @Rob-S - looks like the tests are failing. I think this may be because:https://dev.azure.com/dsccommunity/ComputerManagementDsc/_build/results?buildId=2305&view=logs&j=158455ef-2db5-56fd-6022-83ded799e050&t=b737ddad-5217-50db-260a-b6e0dd548ea1&l=933

Rob-S commented 4 years ago

@PlagueHO Yes, I had fixed the tests around the same time you entered that comment.

Rob-S commented 4 years ago

I think what we need is a unit test that tests when MultipleInstances is set to StopExisting. I could be wrong, but I didn't see one. Is it easy to add that?

No. It's easy to add it for the other values, but not for StopExisting, since the Enum does not support that. See the comments for my commits when I attempted to do that. The issue is that you have to go to the SettingsSet level and if you try to mock that function it fails miserably elsewhere -- the "real" SettingsSet functions must be used.