Fix for issue #109

[p0w3rsh3ll]

This change is 

[codecov-io]

Codecov Report

Merging #110 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #110    +/-   ##
====================================
+ Coverage    83%    83%   +<1%     
====================================
  Files        19     19            
  Lines      2760   2765     +5     
  Branches      4      4            
====================================
+ Hits       2305   2310     +5     
  Misses      451    451            
  Partials      4      4

[johlju]

@kwirkykat need to approve this enhancement. But in the meantime please update the unit tests to test this change (make sure there are unit test that tests with and without passing LogPath), and also update the unreleased section under the Change log in the README.md.

[kwirkykat]

@p0w3rsh3ll Why are you removing the log path from the Windows Package Cab resource?

[kwirkykat]

@p0w3rsh3ll Sorry I see it now. You are making a change so that the LogPath parameter is optional as it is supposed to be. Change looks fine to me, but @johlju 's comment needs to be addressed before I can approve.

[johlju]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @p0w3rsh3ll and @kwirkykat)

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 74 at r2 (raw file):

 $null -eq (Get-TargetResource -Name $script:testPackageName @getTargetResourceCommonParams)['LogPath'] | Should be $true

Can we simplify this a bit? Maybe this reads a little better?

$getTargetResourceResult = Get-TargetResource -Name $script:testPackageName @getTargetResourceCommonParams
$getTargetResourceResult.LogPath | Should BeNullOrEmpty

[p0w3rsh3ll]

Sorry @johlju but your suggested code doesn't work for me (I'm using the inbox Pester module version 3.4.0).
I got a "PropertyNotFoundException: The property 'LogPath' cannot be found on this object" message in my branch and a validated test in your dev branch although it should have thrown an error.

[johlju]

@p0w3rsh3ll make sure LogPath is always returned in the hash table from the Get-TargetResource function. If it can't be evaluated (getting the default log path) then return $null. But issue #109 states that the documentation says that it defaults to %WINDIR%\Logs\Dism\dism.log? Maybe that is what we shall return if LogPath is not assigned in the configuration?

[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.

[p0w3rsh3ll]

"Abandoned"? What an awful experience :(

Sorry @johlju , I didn't reply to your comment because I understand it but don't get it.

All I can say is that the Get-TargetResource function is a "wrapper" of the native Get-WindowsPackage cmdlet. The native Get-WindowsPackage cmdlet returns an object that already has a LogPath property and so does the Get-TargetResource function .

The above commits aim to fix what was wrong with the LogPath parameter and ensure that it's not a mandatory parameter and defaults to %WINDIR%\Logs\Dism\dism.log if not set.

[johlju]

I appreciate you commenting on this issue, and also appreciate you expressing your dissatisfaction with how I handle this. I want every contributors experience contributing here the very best. I will try to do better!
I apologize if my comments didn't explain what I meant properly. I try to explain it better this time.

Also, sorry about the abandoned label, and that it made you feel as this is a bad experience. The Stale bot helps both me and contributors to remind about issues and pull requests. The Stale bot will automatically set the PR as abandoned after 14 days of inactivity when the PR is labeled with any 'Waiting...' label. It should have been removed as soon as you commented, but due to a (reported) bug in the Stale app it only removes on new pushes. I have now manually removed it.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @p0w3rsh3ll and @kwirkykat)

---

DscResources/MSFT_WindowsPackageCab/MSFT_WindowsPackageCab.psm1, line 70 at r2 (raw file):

    {
        $getWindowsPackageParams['LogPath'] = $LogPath
        $windowsPackageCab['LogPath'] = $LogPath

    $windowsPackageCab = @{
        Name = $Name
        Ensure = 'Present'
        SourcePath = $SourcePath
        LogPath = $LogPath
    }

This is how the hash table that is returned looked like on line 56 before this change. The property LogPath was always returned in the hash table $windowsPackageCab.

For this change, we added that LogPath should only be added to the hash table $windowsPackageCab when LogPath has been assigned in the configuration ($PSBoundParameters.ContainsKey('LogPath')).

What I'm looking for is that we should add LogPath to the hash table $windowsPackageCab as a $null value when LogPath has not been assigned in the configuration (-not $PSBoundParameters.ContainsKey('LogPath')).
That would again always return LogPath in the returned hash table $windowsPackageCab.

The cmdlet Get-WindowsPackage result is assigned to $windowsPackageInfo, but I can't see that result being used to update the returned hash table $windowsPackageCab, except for the Ensure property. Please correct me if I'm wrong.
If Get-WindowsPackage returns a LogPath in the variable $windowsPackageInfo, i.e $windowsPackageInfo.LogPath, then maybe it's more proper to always return that property into the returns hash table's LogPath property? But that wasn't done before this change either so that can be another PR.

So for this comment, I'm good with that we always return the LogPath property (with appropriate value, like $null) even when -not $PSBoundParameters.ContainsKey('LogPath'), and of course a unit test for that as well.
If this already works this way, then please correct me! 🙂

[p0w3rsh3ll]

@johlju Thanks for your explanations. Much appreciated :-)

I get it. Yes, you're absolutely right. I didn't realize the LogPath was missing from the returned hashtable by the Get-TargetResource function after my changes.
My bad. I'll start working on it and fix it.

[johlju]

@p0w3rsh3ll Thanks to you!

No worries. That is what reviews are for, another pair of eyes on the changes 🙂 I will review once your are done, and after that we ask Katie to approve and merge.

[johlju]

@p0w3rsh3ll Can you please look at the failing tests at https://ci.appveyor.com/project/PowerShell/psdscresources/build/2.4.254.0?fullLog=true#L7081

[p0w3rsh3ll]

@johlju Yes, I've seen that. I've planned to look at it as soon as I can.
There's an expected error due to my last change.
But there are also errors that are not expected that I've already reproduced on my laptop. I'm not sure why they occur. I still need to figure out why.

[johlju]

@p0w3rsh3ll Great that you are on it already. 🙂 No hurry, take your time.

[johlju]

@kwirkykat This one looks good to me now. Can you review when you get a chance?

Reviewed 1 of 2 files at r2, 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kwirkykat)

[johlju]

@kwirkykat Could you review since I think @p0w3rsh3ll has made the requested changes.

@p0w3rsh3ll could you please rebase this branch against dev?

[johlju]

@kwirkykat gentle reminder that this is ready for review again.

[PlagueHO]

@kwirkykat - is this something you could look at or @mgreenegit are you able to complete the review? It would be good to get this one in.

[PlagueHO]

Hi @p0w3rsh3ll - I'm picking up the review for this one and will try and get it through ASAP.

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kwirkykat and @p0w3rsh3ll)

---

README.md, line 581 at r5 (raw file):

* WindowsPackageCab
    * Made the LogPath parameter optional as it is supposed to be.

Can you just make a tweak to the wording here and link to the issue like this?

* Changed LogPath parameter to be optional (issue [#109](https://github.com/PowerShell/PSDscResources/issues/109)).

---

DscResources/MSFT_WindowsPackageCab/MSFT_WindowsPackageCab.psm1, line 71 at r4 (raw file):

Previously, kwirkykat (Katie Keim) wrote…

This line is not needed as LogPath is added in the declaration of $windowsPackageCab above.

I think this is OK because it is $getWindowsPackageParams that the LogPath is being added to.

---

DscResources/MSFT_WindowsPackageCab/MSFT_WindowsPackageCab.psm1, line 154 at r5 (raw file):

        $setTargetResourceParams['LogPath'] = $LogPath
    }
        

Can you trim this whitespace? VS Code will do this for you on Save if you configure it with the Trim Trailing Whitespace setting

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 73 at r4 (raw file):

Previously, kwirkykat (Katie Keim) wrote…

Please match the repeated format in the tests above:

$getTargetResourceResult = Get-TargetResource -Name $script:testPackageName @getTargetResourceCommonParams
$getTargetResourceResult.Ensure | Should Be 'Present'

This one doesn't look like it has been corrected fully. It needs the:

$getTargetResourceResult.Ensure | Should Be 'Present'

to be added.

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 107 at r4 (raw file):

Previously, kwirkykat (Katie Keim) wrote…

How are these two new tests any different from the two tests above?
If this scenario was failing wouldn't we see these tests failing already?
The parameter filter can be added to the tests above rather than adding new tests.

It 'Should call Add-WindowsPackage when Ensure is Present' {
     Set-TargetResource -Name $script:testPackageName -SourcePath $script:testSourcePath -Ensure 'Present'
     Assert-MockCalled -CommandName 'Dism\Add-WindowsPackage'
}
 	 
It 'Should call Remove-WindowsPackage when Ensure is Absent' {
    Set-TargetResource -Name $script:testPackageName -SourcePath $script:testSourcePath -Ensure 'Absent'
    Assert-MockCalled -CommandName 'Dism\Remove-WindowsPackage
}

I could be wrong, but I think this is OK. The tests do appear to cover the 4 possible behaviors: Absent with Log, Present with Log, Absent without Log and Present without Log.

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 73 at r5 (raw file):

                    Assert-MockCalled -CommandName 'Dism\Get-WindowsPackage' -ParameterFilter { $LogPath -eq $script:testLogPath }
                }
                It 'Should return an empty log path when it was not specified' {

Can you also add a linefeed above the It '...' {

[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.

[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.

[PlagueHO]

Can you resolve the conflicts @p0w3rsh3ll and tag me when you're done and I'll complete the review? Sorry about taking so long!

[p0w3rsh3ll]

@PlagueHO ping for review :-)

[PlagueHO]

Onto this tomorrow @p0w3rsh3ll ! Thank you 😁

[PlagueHO]

Hi @p0w3rsh3ll - can you click the Reviewable button above and click "Done" on all the issues you've resolved? This makes it easier for me to review properly 😁 Thank you!

[p0w3rsh3ll]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @johlju, @kwirkykat, and @PlagueHO)

---

README.md, line 581 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you just make a tweak to the wording here and link to the issue like this?

* Changed LogPath parameter to be optional (issue [#109](https://github.com/PowerShell/PSDscResources/issues/109)).

Done.

---

DscResources/MSFT_WindowsPackageCab/MSFT_WindowsPackageCab.psm1, line 71 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I think this is OK because it is $getWindowsPackageParams that the LogPath is being added to.

Done.

---

DscResources/MSFT_WindowsPackageCab/MSFT_WindowsPackageCab.psm1, line 154 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you trim this whitespace? VS Code will do this for you on Save if you configure it with the Trim Trailing Whitespace setting

Done.

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 73 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This one doesn't look like it has been corrected fully. It needs the:

$getTargetResourceResult.Ensure | Should Be 'Present'

to be added.

Done.

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 107 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I could be wrong, but I think this is OK. The tests do appear to cover the 4 possible behaviors: Absent with Log, Present with Log, Absent without Log and Present without Log.

Done.

---

Tests/Unit/MSFT_WindowsPackageCab.Tests.ps1, line 73 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you also add a linefeed above the It '...' {

Done.