-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix error when KeyValuePair file is empty or does not exist - Fixes #34 #35
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #35 +/- ##
==================================
+ Coverage 68% 69% +<1%
==================================
Files 4 4
Lines 443 452 +9
==================================
+ Hits 304 314 +10
+ Misses 139 138 -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, two non-blocking comments.
Reviewed 5 of 5 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved
DSCResources/DSR_KeyValuePairFile/DSR_KeyValuePairFile.psm1, line 44 at r1 (raw file):
$fileEncoding = 'ASCII'
We must return 'ASCII'
here because it is a validate set for that property? Returning $null
will fail? Would have been better to return $null
since we don't know the encoding if the file does not exist. But might fail with Get-DscConfiguration
or Invoke-DscResource -Method Get
. 🤔
Tests/Unit/DSR_KeyValuePairFile.Tests.ps1, line 148 at r1 (raw file):
Assert-MockCalled -CommandName Get-Content ` -Exactly -Times 0
Maybe add Scope
to all of these? You can add that in another PR if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johlju - did all the fixes 😁 could you sign off again for me when you have a moment?
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @johlju)
DSCResources/DSR_KeyValuePairFile/DSR_KeyValuePairFile.psm1, line 44 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$fileEncoding = 'ASCII'
We must return
'ASCII'
here because it is a validate set for that property? Returning$null
will fail? Would have been better to return$null
since we don't know the encoding if the file does not exist. But might fail with Get-DscConfiguration
orInvoke-DscResource -Method Get
. 🤔
Fixed!
Tests/Unit/DSR_KeyValuePairFile.Tests.ps1, line 148 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Assert-MockCalled -CommandName Get-Content ` -Exactly -Times 0
Maybe add
Scope
to all of these? You can add that in another PR if you like.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
Pull Request (PR) description
This PR fixes an error that occurs when the KeyValuePair file is empty or does not exist.
It also cleans up the unit tests to make them less fragile.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
@johlju - would you mind reviewing this when you have a moment? Sorry about the length of the change in the unit tests - I decided to refactor them to make them less brittle.
This change is