PowerPlan: Powercfg.exe instead of WMI and allow GUID as power plan name

[J0F3]

Pull Request (PR) description

Changes the PowerPlan resource so that is uses powercfg.exe to get and set power plans instead of the WMI method which is not available or working on all Windows version. This would fix the problems on Windows 10, Windows Server 2019 and Nano Server.
Furthermore the resource accepts now also a GUID as the power plan "name" so the desired plan can be als specified as a GUID when using it on a non-english Windows.

This Pull Request (PR) fixes the following issues

• Fixes MSFT_xPowerPlan should use power plan guid #59
• Fixes xPowerPlan: Trying to use it on Windows 10 fails with no Activate method error #155
• Fixes MSFT_xPowerPlan Support for Nano server #65

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• [ N/A] Resource documentation added/updated in README.md in the resource folder.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• [N/A ] Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov-io]

Codecov Report

Merging #202 into dev will decrease coverage by <1%.
The diff coverage is 100%.

@@        Coverage Diff         @@
##           dev   #202   +/-   ##
==================================
- Coverage   90%    90%   -1%     
==================================
  Files        7      7           
  Lines      735    724   -11     
==================================
- Hits       667    656   -11     
  Misses      68     68

[PlagueHO]

Really awesome stuff @J0F3 - just a few queries.

I really think we need to get some integration tests in for this resource - even if they can't be run in AppVeyor, because I think we should make it easy for people to validate this resource in a few different OS versions (incl. Nano Server). I've got a lab here with various different OS's that I use to run special checks with.

Also, because we're calling out to an external command we can sometimes have issues with localized versions (e.g. if we're looking for certain messages in the output then they may be different for other languages). So we might need to validate on a different OS.

An alternate approach is to allow for a fallback to calling out to PowerCfg.exe only if more "PowerShelly" methods like CIM aren't available on the specific OS - this could all be done within the "Get-PowerPlan" and "Set-PowerPlan" functions to keep the workings abstracted from the Resource? Although, I'd be happy to only look at doing this if we do run into problems using PowerCfg.exe

Reviewed 6 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @J0F3)

---

CHANGELOG.md, line 9 at r3 (raw file):

    Fixes [Issue #59](https://github.com/PowerShell/ComputerManagementDsc/issues/59)
  - Changed the resource so it uses powercfg.exe instead of WMI/CIM.
  (Workaround fo rServer 2012R2 Core, Nano Server, Server 2019 and Windows 10)

The indent here seems odd- I think these three lines need to be indented by 4 spaces each so that they display correctly in the MD.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 49 at r3 (raw file):

    )

    $plan = Get-PowerPlans | Where-Object{($_.Name -eq $Name) -or ($_.Guid -eq $Name)}

Can you use layout?

Where-Object -FilterScript {
   ....
}

To reduce code duplication, is it worth changing Get-PowerPlans to a function that takes a $Name parameter and applies the Where-Object filter for us?

Minor nitpick: we should avoid using plural on function names -e.g. prefer Get-PowerPlan over Get-PowerPlans.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 113 at r3 (raw file):

        try
        {
            Invoke-Expression -Command "powercfg.exe /S $($plan.Guid)" -ErrorVariable powerCfgError

Could you add a comment above this line just mentioning that we're using PowerCfg instead of CIM because of...

Just in case someone goes to refactor it back! 😁

I'm also thinking that it is worth abstracting the entire call out to setting the configuration into a separate function as well: Set-PowerPlan ...

Then we won't have multiple levels of abstraction within the same function and should also make testing slightly simpler.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 114 at r3 (raw file):

        {
            Invoke-Expression -Command "powercfg.exe /S $($plan.Guid)" -ErrorVariable powerCfgError
            if ($powerCfgError)

Can you add a blank line above this one?

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 116 at r3 (raw file):

            if ($powerCfgError)
            {
                Throw $powerCfgError

Can you use the New-InvalidOperationException function to throw the exception? You can just pass in the -ErrorRecord parameter

e.g.

New-InvalidOperationException -ErrorRecord $powerCfgError

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 128 at r3 (raw file):

    {
        New-InvalidOperationException `
        -Message ($script:localizedData.PowerPlanNotFound -f $Name)

Can you correct the indent on this line for me?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 496 at r3 (raw file):

<#
    .SYNOPSIS
        This function gets all power plans/schemes available on the machine using the powercfg.exe utility and returns a custom object for each plan

Could you reduce the width of these lines to around 80 chars? Otherwise they wrap when doing review.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 497 at r3 (raw file):

    .SYNOPSIS
        This function gets all power plans/schemes available on the machine using the powercfg.exe utility and returns a custom object for each plan
        It exists because the WMI classes are not available on all platforms (e.g on Server 2012 R2 core or nano server)

Can you end with a full stop?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 500 at r3 (raw file):

    .NOTES
        This function is used by the PowerPlan resource

Can you end with a full stop?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 502 at r3 (raw file):

        This function is used by the PowerPlan resource
#>
function Get-PowerPlans {

Could you use Get-PowerPlan instead?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 505 at r3 (raw file):

    [CmdletBinding()]
    param
    (

Suggest adding an optional name parameter in here that will allow you to apply the Where-Object filter on name/guid.

If the function is called without the parameter then it would behave exactly as it does now, but if passed it could apply the filter.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 508 at r3 (raw file):

    )

    $powercfgOutPut = Invoke-Expression -Command 'powercfg.exe /l'

Can we add some pre-validation checking on the output here? E.g. if $powercfgOutPut is empty then it should probably throw an exception.

This is one of the biggest challenges with calling out to external "non-PowerShell" methods to do the work for us: We can't make assumptions about what we get back and therefore have to do a bunch more checking.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 512 at r3 (raw file):

    $allPlans = @()

    foreach($line in $powercfgOutPut)

Can you add space after foreach and if on the following lines?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 529 at r3 (raw file):

            $allPlans += $plan
        }
    }

Are we able to add some sort of validation check here? E.g. if we weren't able to get any Power Plans at all could this indicate a problem?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 533 at r3 (raw file):

    return $allPlans
}

Can you remove blank line here?

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 708 at r3 (raw file):

                It 'Should not throw exception' {
                    {Get-PowerPlans} | Should -Not -Throw

You can assign the result of the Get-PowerPlans call to a script variable and then rather than recalling Get-PowerPlans for every assertion you can just check the variable. Not a major thing though- can just result in faster tests (although you're mocking the call to PowerCfg so not much faster).

e.g.

{ $script:powerPlans = Get-PowerPlans } | Should -Not -Throw

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 740 at r3 (raw file):

                        )
                    } `
                    -ParameterFilter {$Command -eq  'powercfg.exe /l'}

Can you put use format:

{
    $Command -eq ..
}

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 755 at r3 (raw file):

                It 'Should be only have one plan marked as active' {
                    Get-PowerPlans | Where-Object{$_.IsActive -eq $true} | Should -HaveCount 1

Can you use -FilterScript parameter and also we want FilterScript block like:

Get-PowerPlans | Where-Object -FilterScript {
  $_.IsActive -eq $true
} | Should -HaveCount 1

Same thing in the next It block.

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 761 at r3 (raw file):

                    (Get-PowerPlans | Where-Object{$_.IsActive -eq $true}).Name  | Should -Be 'Balanced'
                }
            }

I think we should also probably test the actual names/guids are correct that are being returned. We want to be able to ensure we're picking up if any unexpected layout changes occur in the output of PowerCfg.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 36 at r3 (raw file):

    $testCases =@(
        #Power plan as name specified

Can you add a space after the comment #

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 42 at r3 (raw file):

        },

        #Power plan as Guid specified

Can you add a space after the comment

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 119 at r3 (raw file):

            It 'Should return an inactive plan (power plan specified as <Type>)' -TestCases $testCases {

Can you remove blank line here?

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 120 at r3 (raw file):

            It 'Should return an inactive plan (power plan specified as <Type>)' -TestCases $testCases {

                Param(

Can you use lower case P in param and put ( on the next line?

And same throughout.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 192 at r3 (raw file):

            Mock `
            -CommandName Invoke-Expression `
            -ParameterFilter {$Command -eq 'powercfg.exe /S 8c5e7fda-e8bf-4a96-9a85-a6e23a8c635c'} `

Can you use {

[J0F3]

Reviewable status: 3 of 10 files reviewed, 24 unresolved discussions (waiting on @J0F3 and @PlagueHO)

---

CHANGELOG.md, line 9 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

The indent here seems odd- I think these three lines need to be indented by 4 spaces each so that they display correctly in the MD.

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 49 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use layout?

Where-Object -FilterScript {
   ....
}

To reduce code duplication, is it worth changing Get-PowerPlans to a function that takes a $Name parameter and applies the Where-Object filter for us?

Minor nitpick: we should avoid using plural on function names -e.g. prefer Get-PowerPlan over Get-PowerPlans.

Good idea. 👍

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 113 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you add a comment above this line just mentioning that we're using PowerCfg instead of CIM because of...

Just in case someone goes to refactor it back! 😁

I'm also thinking that it is worth abstracting the entire call out to setting the configuration into a separate function as well: Set-PowerPlan ...

Then we won't have multiple levels of abstraction within the same function and should also make testing slightly simpler.

Great idea. I have just added a Set-PowerPlan funciton.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 114 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a blank line above this one?

Done with the Set-PowerPlan function

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 116 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use the New-InvalidOperationException function to throw the exception? You can just pass in the -ErrorRecord parameter

e.g.

New-InvalidOperationException -ErrorRecord $powerCfgError

Done with the Set-PowerPlan function

---

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 128 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you correct the indent on this line for me?

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 496 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you reduce the width of these lines to around 80 chars? Otherwise they wrap when doing review.

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 497 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you end with a full stop?

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 500 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you end with a full stop?

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 502 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you use Get-PowerPlan instead?

yep 😃

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 505 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Suggest adding an optional name parameter in here that will allow you to apply the Where-Object filter on name/guid.

If the function is called without the parameter then it would behave exactly as it does now, but if passed it could apply the filter.

I think to make the name parameter mandatory is enough here. (While doing the filtering directly in the function we do not need any function to get all power plans...)

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 508 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we add some pre-validation checking on the output here? E.g. if $powercfgOutPut is empty then it should probably throw an exception.

This is one of the biggest challenges with calling out to external "non-PowerShell" methods to do the work for us: We can't make assumptions about what we get back and therefore have to do a bunch more checking.

Good point.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 512 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add space after foreach and if on the following lines?

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 529 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Are we able to add some sort of validation check here? E.g. if we weren't able to get any Power Plans at all could this indicate a problem?

While doing now the filtering in the function returning nothing simply means the desired power plan does not exist on the system and is then catched in the Get-TargetResource function.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 533 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove blank line here?

Done.

[J0F3]

Well I have changed the things now a little. I changed the Get-PowerPlans function to Get-PowerPlan with a -Name parameter and added a new Set-PowerPlan function.

However in the meantime I have also made some tests with using the native Win32 API for power management (which probably also the powercfg.exe uses). I am now able to get and set power plans using the win32 API and P/Invoke.

I think this would be actually the most robust approach and I will create probably an second PR with this P/Invoke approach. Or should I just update this one?

[J0F3]

Reviewable status: 3 of 10 files reviewed, 24 unresolved discussions (waiting on @PlagueHO)

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 708 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

You can assign the result of the Get-PowerPlans call to a script variable and then rather than recalling Get-PowerPlans for every assertion you can just check the variable. Not a major thing though- can just result in faster tests (although you're mocking the call to PowerCfg so not much faster).

e.g.

{ $script:powerPlans = Get-PowerPlans } | Should -Not -Throw

Done.

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 740 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you put use format:

{
    $Command -eq ..
}

Done.

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 755 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use -FilterScript parameter and also we want FilterScript block like:

Get-PowerPlans | Where-Object -FilterScript {
  $_.IsActive -eq $true
} | Should -HaveCount 1

Same thing in the next It block.

Done.

---

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 761 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I think we should also probably test the actual names/guids are correct that are being returned. We want to be able to ensure we're picking up if any unexpected layout changes occur in the output of PowerCfg.

Done.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 36 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the comment #

Done.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 42 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space after the comment

Done.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 119 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove blank line here?

Done.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 120 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lower case P in param and put ( on the next line?

And same throughout.

Done.

---

Tests/Unit/MSFT_PowerPlan.Tests.ps1, line 192 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use {

Done.

[J0F3]

All right I just created an other PR with the approach to use the Windows APIs through some new functions. Actually just to have both variants here to compare and then just close one and go further with the one we prefer. -> #203

I personally prefer the variant with the Windows APIs but to be honest I have currently no idea how to write proper unit tests for the functions which call the Windows native APIs trough P/Invoke. 🙄

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.