dsccommunity / xPendingReboot

MIT License
31 stars 10 forks source link

Allow users to disable certain reboot checks #1

Closed qmfrederik closed 8 years ago

qmfrederik commented 9 years ago

I had an issue where my Azure VM went into a reboot loop after I applied a Powershell DSC script that included xPendingReboot.

Basically, there was a stale file rename operating pending, so every time the DSC script ran, it would detect that, reboot the VM, and we got in a loop.

The case is, I did not want to check for pending file rename operations - I was only looking for a reboot that could have been triggered by a Windows Update package I installed.

So I've created this pull request that allows the user to specify for which 'reboot sources' (s)he wants to check. For example, if you want to bypass the pending file rename operations, you could just set SkipPendingFileRename = $True.

msftclas commented 9 years ago

Hi @qmfrederik, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas commented 9 years ago

@qmfrederik, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, MSBOT;

TravisEz13 commented 8 years ago

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources Please work with us to come up with a plan to cover this code.

TravisEz13 commented 8 years ago

If there is no update of the pull request within 7 days, we will close the pull request. After that, if you have an update, feel free to reopen the pull request or a new pull request.

TravisEz13 commented 8 years ago

Please update the readme with the details of the changes and include a summary of changes in an unreleased version change per the contribution guidelines

TravisEz13 commented 8 years ago

Please fix the resource designer test failures. In the details

TravisEz13 commented 8 years ago

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources Please work with us to come up with a plan to cover this code.

Example, Please write a unit test that we test-targetresource actually returns false when you say to skip a reboot check, but the reboot check is positive.

qmfrederik commented 8 years ago

Hi @TravisEz13 ,

I think I addressed most of your feedback; let me know if something is missing.

Regarding the tests: I added a first test; however, it seems my mocks are not working (correctly). I have to admit I'm now to Pester and there were no tests in the repository which I could base my test off.

Would be great if you could provide some guidance here,

Thanks.

TravisEz13 commented 8 years ago

There are still 2 test failures. For the first issue, I have fixed this issue in the template, but it has not been merged

This error should read.

File C:\projects\xpendingreboot\DSCResources\MSFT_xPendingReboot\MSFT_xPendingReboot.schema.mof contains 0x00 bytes. It's probably uses Unicode and need to be converted to ASCII. Use Fixer 'Get-UnicodeFilesList $pwd | ConvertTo-ASCII'

You can find the updated code here: https://github.com/PowerShell/DscResource.Tests/tree/TravisEz13-patch-1

Update, I had a colleague review and merge the fix. I reran you last commit and have the corrected error message.

TravisEz13 commented 8 years ago

I've figured out this issue with the second error. I recommend this update to the test:

You are I should file an issue to refactor the resource not to use invoke-command and activate the test.

        Mock Invoke-WmiMethod {
            return New-Object PSObject -Property
                @{
                    ReturnValue = 0
                    IsHardRebootPending = $false
                    RebootPending = $true
                } 
        } -ModuleName MSFT_xPendingReboot 

        # The test is pending because you cannot mock into a command that is invoked by Invoke-Command
        It "A reboot should have been triggered" {
            $result = Test-TargetResource -Name "Test"

            $result | Should Be $false
            Assert-MockCalled -CommandName Invoke-WmiMethod -ModuleName MSFT_xPendingReboot
        } -Pending
qmfrederik commented 8 years ago

Hi @TravisEz13 ,

Have a look at the updated code:

Looks like something went wrong with the mocking of Get-WmiObject; it would be great if you could have a look at that and let me know if there's something I missed.

Cheers

TravisEz13 commented 8 years ago

For the failing test case, I don't think there is an easy fix. I recommend something like the example below. I cannot get a pester mock to work when the function being mocked is invoked inside invoke-command. I'd also file an issue that the resource should be refactored to use invoke-command less and remove this pending test case.

        Mock Invoke-WmiMethod {
            return New-Object PSObject -Property
                @{
                    ReturnValue = 0
                    IsHardRebootPending = $false
                    RebootPending = $true
                } 
        } -ModuleName MSFT_xPendingReboot 

        # The test is pending because you cannot mock into 
        # a command that is invoked by Invoke-Command
        It "A reboot should have been triggered" -Pending {
            $result = Test-TargetResource -Name "Test"

            $result | Should Be $false
            Assert-MockCalled -CommandName Invoke-WmiMethod -ModuleName MSFT_xPendingReboot
        } 
qmfrederik commented 8 years ago

Hi @TravisEz13 ,

Actually, I took care of refactoring the code so that Invoke-Method is no longer used by the PowerShell module.

The test now fail because it seems like I can't get Pester to mock Get-WmiObject.

Any thoughts?

TravisEz13 commented 8 years ago

I sync'ed your latest code and fixed all the test issues (described bellow.) I put the file in a Gist, here, as much of the file was changed.

Issue 1 - If you are calling the mocked function in a different module, you need to say what module you are calling the mock from when declaring it.

Issue 2 - It was difficult to debug the test case, because you combined many 'should's into one it. I separated them out so when something fails, you can tell what is failing.

Issue 3 - In you mocks for 'No Reboots Are Required' context. You were not returning an object and the product code was failing.

I believe this is all resolve in the GIST above.

I hope this helps.

Thanks, Travis

qmfrederik commented 8 years ago

Hi @TravisEz13 ,

Cool, thanks! Looks like we're all set now?

Frederik.

TravisEz13 commented 8 years ago

Thanks for your contribution and all your patients with the the feedback.