dsccommunity / StorageDsc

DSC resource module is used to manage storage on Windows Servers.
https://dsccommunity.org
MIT License
71 stars 52 forks source link

Fix bug in xDiskAccessPath and xMountImage and enabled Integration Tests - Fixes #102, #105 #106

Closed PlagueHO closed 7 years ago

PlagueHO commented 7 years ago

This PR primarily fixes #102.

However during investigation of #102, I found a way to enable the integration tests to run in AppVeyor by using DISKPART to create a new VHD. I decided to enable these to ensure that this module is being effectively tested on Windows Server 2012 R2.

This highlighted issue #105 as the integration tests failed on the AppVeyor build agent (but passed on Windows 10 and Windows Server 2016). I identified the cause of the problem and fixed it as well so that I could commit this fix.

The PR changes are as per:


This change is Reviewable

codecov-io commented 7 years ago

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #106    +/-   ##
===================================
+ Coverage   92%    92%   +<1%     
===================================
  Files        5      5            
  Lines      645    650     +5     
===================================
+ Hits       594    599     +5     
  Misses      51     51
bgelens commented 7 years ago

Great reasoning to make use of DiskPart to enable the integration tests in appveyor!!


Reviewed 14 of 14 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.


Modules/xStorage/DSCResources/MSFT_xMountImage/MSFT_xMountImage.psm1, line 80 at r1 (raw file):

            # Find the first 'Basic' partition
            $partitions = $disk | Get-Partition
            $partition = $partitions | Where-Object -Property Type -EQ 'Basic'

it's -eg lowercase.

Why the breakup and not?

$partition = $disk | Get-Partition | Where-Object -Property Type -eq 'Basic'

Or

$partition = $disk | Get-Partition |
    Where-Object -Property Type -eq 'Basic'

Modules/xStorage/DSCResources/MSFT_xMountImage/MSFT_xMountImage.psm1, line 558 at r1 (raw file):

        # Find the first 'Basic' partition to mount
        $partitions = $disk | Get-Partition
        $partition = $partitions | Where-Object -Property Type -EQ 'Basic'

same comments as before


Tests/TestHelpers/CommonTestHelper.psm1, line 105 at r1 (raw file):

    $tempScriptPath = Join-Path -Path $ENV:Temp -ChildPath 'DiskPartVdiskScript.txt'
    Write-Verbose -Message ('Creating DISKPART script {0}' -f $tempScriptPath)
    Set-Content -Path $tempScriptPath -Value "create vdisk file=`"$Path`" type=expandable maximum=$SizeInMB"

From experience, this file needs to have Ascii encoding. Recommend adding -Encoding Ascii The text is also expected to be uppercase if I remember correctly. create vdisk FILE="" TYPE=EXPANDABLE MAXIMUM=1024



---

*Comments from [Reviewable](https://reviewable.io:443/reviews/powershell/xstorage/106#-:-KnOPzFkJm0IHZDegcLq:bfqnwry)*
<!-- Sent from Reviewable.io -->
PlagueHO commented 7 years ago

Thanks for reviewing @bgelens ! Appreciate the work and detail as always! Changes made and questions answered.


Review status: 13 of 14 files reviewed at latest revision, 3 unresolved discussions.


Modules/xStorage/DSCResources/MSFT_xMountImage/MSFT_xMountImage.psm1, line 80 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…
it's -eg lowercase. Why the breakup and not? ``` $partition = $disk | Get-Partition | Where-Object -Property Type -eq 'Basic' ``` Or ``` $partition = $disk | Get-Partition | Where-Object -Property Type -eq 'Basic' ```

Good question. A couple of reasons:

  1. When I was debugging this problem I found it much easier to track down the problem by splitting up statements so I could find out and compare the different outputs on the different OS's.
  2. This best practice https://github.com/PowerShell/DscResources/blob/master/BestPractices.md#use-variables-rather-than-extensive-piping

However, I realize this wouldn't really be excessive piping. So I'm OK to reassemble if you'd prefer - as hopefully I'll not need to do any further debugging on this (but never say never). Your call :grin:


Modules/xStorage/DSCResources/MSFT_xMountImage/MSFT_xMountImage.psm1, line 558 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…
same comments as before

See above.


Tests/TestHelpers/CommonTestHelper.psm1, line 105 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…
From experience, this file needs to have Ascii encoding. Recommend adding ```-Encoding Ascii``` The text is also expected to be uppercase if I remember correctly. ```create vdisk FILE="" TYPE=EXPANDABLE MAXIMUM=1024```

Good catch. Done.


Comments from Reviewable

bgelens commented 7 years ago

:lgtm: No need to change further :smile:


Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

PlagueHO commented 7 years ago

Thanks @bgelens - really appreciate your help! Things are really beginning to move around the repos now!