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.

[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[bot]]

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.

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

[stale[bot]]

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[bot]]

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 :grin:

[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 :grin: Thank you!