xVHD - Add support for setting disk type (Fixed, Dynamic, Differencing) #77

johnf · 2017-01-08T04:32:08Z

Add the ability to create Fixed disks

Fixes #72

This change is

msftclas · 2017-01-08T04:32:14Z

Hi @johnf, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

PlagueHO · 2017-01-08T07:44:27Z

Hi @johnf - are you able to fix the problem in the MOF schema as reported by the AppVeyor tests failing?

johnf · 2017-01-08T11:29:57Z

Fixed. Dumb typo

PlagueHO · 2017-01-09T06:34:27Z

@johnf - awesome job! Thanks for addressing this.

Could you please add an entry to the #Unreleased section in the Readme.md listing the change?

Could you also add this to this parameter to the Examples/Sample_xVHD_DiffVHD.ps1 (and to the same place in the Examples section of the Readme.md).

It might be worth also creating an Examples/Sample_xVHD_FixedVHD.ps1 showing how the resource is used to create a fixed disk (and copy the example to the Readme.md)

Unfortunately this resource doesn't contain any unit tests for xVHD yet so we can't add tests for this change.

Thanks again for all your work on this! It is greatly appreciated.

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

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 65 at r1 (raw file):

        # Type of Vhd to be created
        [ValidateSet("Dynamic","Fixed","Differencing")]
        [String]$Type = "Dynamic",

One thing that may be confusing here is that it would be possible for someone to specify a $Type of 'Dynamic' or 'Fixed' and also a $ParentPath set. This doesn't look like it would cause any problems, but it might imply to users that this was a possible combination of parameters (e.g. a Dynamic Differencing Disk- which isn't of course possible).

It might be worth adding a parameter validation check into the Set-* and Test-* that would throw an exception if an invalid combination of parameters was requested.

Comments from Reviewable

johnf · 2017-06-12T03:05:51Z

I've updated my branch with your comments.

I added the new Example but in the README rather than add a whole new example I updated the first one to be Fixed.

bgelens · 2017-07-03T19:39:19Z

@johnf We have diverged a bit with the Readme since this PR was created. Examples are no longer part of it. Instead links are added to the example in the Examples folder. Probably a good idea to rebase and address the comments so we can take the PR.

Sorry to put you through this. Let me know if you are not comfortable with concepts as rebasing and such.

Reviewed 1 of 3 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 64 at r2 (raw file):

        # Type of Vhd to be created
        [ValidateSet("Dynamic","Fixed","Differencing")]

Could you add [Parameter()] on top of the ValidateSet and Place $Type on a new line below [String]

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 83 at r2 (raw file):

    # Check the module combinations make sense
    if($ParentPath -and $Type -ne "Differencing")

You'll also need something like if ($null -eq $parentpath -and $Type -eq 'Differencing'){throw error}

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 151 at r2 (raw file):

                if($vhd.Type -ne $Type)
                {
                    Throw "This module can't convert disk types yet"

Are you planning to implement this, if not, please remove yet 😄

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 205 at r2 (raw file):

        # Type of Vhd to be created
        [ValidateSet("Dynamic","Fixed","Differencing")]

Same comments as with Set-TargetResource apply here

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 227 at r2 (raw file):

    # Check the module combinations make sense
    if($ParentPath -and $Type -ne "Differencing")

And here

Comments from Reviewable

codecov-io · 2017-07-04T08:45:19Z

Codecov Report

Merging #77 into dev will increase coverage by 7%.
The diff coverage is 94%.

@@         Coverage Diff         @@
##            dev    #77   +/-   ##
===================================
+ Coverage    63%    71%   +7%     
===================================
  Files         9      9           
  Lines      1022   1032   +10     
  Branches     12     12           
===================================
+ Hits        651    733   +82     
+ Misses      359    287   -72     
  Partials     12     12

johnf · 2017-07-04T09:32:08Z

I've rebased

bgelens

Reviewable didn't make any sense anymore so switched to GitHub review for now. Couple of points still. Would be greatly appreciated if you can address them.

bgelens · 2017-07-04T18:16:20Z

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1

@@ -75,6 +80,17 @@ function Set-TargetResource
        Throw "Please ensure that Hyper-V role is installed with its PowerShell module"
    }

+    # Check the module combinations make sense
+    if($ParentPath -and $Type -ne "Differencing")


Could you add a unit test for this resource. It doesn't have to cover the entire thing but at least validate these parameter conditions are working properly e.g. It 'Should throw when ParentPath defined and Type -ne Diff'

bgelens · 2017-07-04T18:19:33Z

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1

@@ -60,6 +60,11 @@ function Set-TargetResource
        # Size of Vhd to be created
        [Uint64]$MaximumSizeBytes,

+        # Type of Vhd to be created
+        [parameter()]


Should be:

[Parameter()] [ValidateSet('Dynamic', 'Fixed', 'Differencing')] [String] $Type = 'Dynamic',

bgelens · 2017-07-04T18:20:14Z

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1

+
+    if($null -eq $ParentPath -and $Type -eq "Differencing")
+    {
+        Throw "Differencing requires a parent path"


And It 'Should throw when ParentPath is not defined and type is Differencing'

bgelens · 2017-07-04T18:21:43Z

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1

@@ -181,6 +207,11 @@ function Test-TargetResource
        [ValidateSet("Vhd","Vhdx")]
        [String]$Generation = "Vhd",

+        # Type of Vhd to be created
+        [parameter()]


Same as with other comment

bgelens · 2017-07-04T18:23:55Z

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1

@@ -198,7 +229,18 @@ function Test-TargetResource
    {
       Throw "Either specify ParentPath or MaximumSizeBytes property." 
    }
-
+
+    # Check the module combinations make sense


Could you remove this comment or move it up (so the earlier validation is included under the comment as well) and change the wording so that it says something like, Validating Parameters

johnf · 2017-07-05T09:26:27Z

I written some tests which are close but aren't working quite right.
I'm not a powershell programmer and I don't have an environment at the moment where I can actually run the tests.
And the cycle of doing that via appveyor will take me hours :)

I've ticked allow edits from maintainers so you can commit to my branch if someone else can make them work

…f the styling and indentation issues. added comment based help

… check parameter conditions

bgelens · 2017-07-05T19:53:39Z

@johnf I've send in some commits to your repo to make some adjustments.
@PlagueHO could you review this PR (as I contributed the last commits, I would like some other eyes looking at the changes) The added unit tests are just an initial base set for parameter combination validation. Additional tests need to be added and I created an issue to increase code coverage already #119 )

Reviewed 2 of 3 files at r3, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

…on. Fixed throw messages

… process. Fixed a bug when MaximumSizeBytes was not specified, resize was wrongly initialized.

PlagueHO · 2017-07-07T20:21:37Z

Sorry about the minor nitpicks. Generally things look fine, but if you're going to do some rework on this module at some point then I've made some suggested changes. One other thing that you would want to address in any rework: Move strings to strings file.

Reviewed 1 of 4 files at r2, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 83 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

You'll also need something like if ($null -eq $parentpath -and $Type -eq 'Differencing'){throw error}

I'd generally recommend creating an Assert-ParametersValid function in a resource and using that to validate the parameter input - with throwing if they are invalid. But don't worry about doing that here. Just a suggestion for future.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 233 at r3 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Could you remove this comment or move it up (so the earlier validation is included under the comment as well) and change the wording so that it says something like, Validating Parameters

See my earlier comments about parameter validation in Assert-ParametersValid - again not at all required here. Just if anyone is doing rework on this module that would be the recommended pattern.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 37 at r6 (raw file):

    if(!(Get-Module -ListAvailable -Name Hyper-V))
    {
        Throw 'Please ensure that Hyper-V role is installed with its PowerShell module'

Not required here (so please don't worry about fixing in this PR), but just a suggestion: convert to standard way of throwing custom errors. E.g. New-InvalidOperationException.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 137 at r6 (raw file):

    # If vhd should be absent, delete it
    if($Ensure -eq 'Absent')

Nitpick: Need space between if and ( - just use global search/replace to fix them all.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 197 at r6 (raw file):

        # Vhd file is not present
        catch

Hope I'm not being too much of a pain, but won't this catch any kind of error - not just missing VHD files?

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 212 at r6 (raw file):

                }
                $null = New-VHD @params
            }

Please add blank line after end of code blocks.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 282 at r6 (raw file):

    # Check if Hyper-V module is present for Hyper-V cmdlets
    if(!(Get-Module -ListAvailable -Name Hyper-V))

Not required, please don't worry about doing this now, just a suggestion for future: The recommended way of doing this is to move it to an Assert-HyperVRequired function.

Examples/Sample_xVHD_FixedVHD.ps1, line 5 at r6 (raw file):

    param
    (
        [string[]]$NodeName = 'localhost',

Please convert parameter block to be standard format. E.g.

[Parameter()]
[String[]]
$NodeName = 'localhost',

[Parameter(Mandatory = $true)]
[String]
$Name

Tests/Unit/MSFT_xVHD.tests.ps1, line 47 at r6 (raw file):

                    return $false
                }
                It 'Should throw when the module is missing' {

Add blank line here.

Tests/Unit/MSFT_xVHD.tests.ps1, line 126 at r6 (raw file):

                Mock -CommandName Get-Module -ParameterFilter { ($Name -eq 'Hyper-V') -and ($ListAvailable -eq $true) } -MockWith {
                    return $false
                }

Please add blank line here.

Tests/Unit/MSFT_xVHD.tests.ps1, line 162 at r6 (raw file):

                }

                #"Generation $Generation should match ParentPath extension $($ParentPath.Split('.')[-1])"

Space after #

Tests/Unit/MSFT_xVHD.tests.ps1, line 164 at r6 (raw file):

                #"Generation $Generation should match ParentPath extension $($ParentPath.Split('.')[-1])"
                Mock -CommandName Test-Path -MockWith { $true }
                It 'Should throw when file extension and generation have a mismatch' {

Please add blank line here.

Comments from Reviewable

bgelens · 2017-07-08T08:26:04Z

No worries, keep up with the nitpicks!

I've made some changes per your findings. Please approve these if they are to your liking 😄

I'm not going to improve / enhance this resource further within the scope of this PR. The additions I made to it, felt needed to have this PR merged (most important: adding the test cases which resulted in some of the code being changed due to bug findings 😉 )
So further enhancements need to be addressed in a new PR. I've logged an issue #121 which should kick start the work on this.

Reviewed 2 of 2 files at r6.
Review status: 3 of 7 files reviewed at latest revision, 12 unresolved discussions.

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 137 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Nitpick: Need space between if and ( - just use global search/replace to fix them all.

Done

DSCResources/MSFT_xVHD/MSFT_xVHD.psm1, line 197 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Hope I'm not being too much of a pain, but won't this catch any kind of error - not just missing VHD files?

You are not, it is true and I was not happy with this myself. I already had made some changes to this PR original contributors code as this was actually the case with the type mismatch .
I'm not planning to reevaluate this resource in this PR but I've logged an issue #121

Examples/Sample_xVHD_FixedVHD.ps1, line 5 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please convert parameter block to be standard format. E.g.
[Parameter()]
[String[]]
$NodeName = 'localhost',

[Parameter(Mandatory = $true)]
[String]
$Name

Done

Tests/Unit/MSFT_xVHD.tests.ps1, line 47 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line here.

Done

Tests/Unit/MSFT_xVHD.tests.ps1, line 126 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please add blank line here.

Done

Tests/Unit/MSFT_xVHD.tests.ps1, line 162 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Space after #

Done

Tests/Unit/MSFT_xVHD.tests.ps1, line 164 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please add blank line here.

Done

Comments from Reviewable

PlagueHO · 2017-07-08T20:19:14Z

Thanks for all your work on getting this PR out 😁 Nearly there! You're doing great stuff.

Reviewed 1 of 4 files at r2, 4 of 4 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Examples/Sample_xVHD_DiffVHD.ps1, line 19 at r7 (raw file):

        [string]$Generation = "Vhd",

        [ValidateSet("Dynamic","Fixed","Differencing")]

This file looks like it still needs it's parameter style fixed.

Comments from Reviewable

bgelens · 2017-07-08T21:24:13Z

OK please check again 😉

Reviewed 4 of 4 files at r7, 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Examples/Sample_xVHD_DiffVHD.ps1, line 19 at r7 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This file looks like it still needs it's parameter style fixed.

Done plus all other xVHD examples

Comments from Reviewable

PlagueHO · 2017-07-08T22:12:36Z

Great job and done!

Reviewed 4 of 4 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

msftclas added the cla-already-signed label Jan 8, 2017

johnf mentioned this pull request Jan 8, 2017

Add support disk Type #72

Closed

kwirkykat added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Jan 20, 2017

bgelens added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jun 28, 2017

bgelens added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Jul 3, 2017

bgelens changed the title ~~Support setting disk type~~ xVHD - Add support for setting disk type (Fixed, Dynamic, Differencing) Jul 3, 2017

bgelens requested changes Jul 4, 2017

View reviewed changes

Support setting disk type

2f72559

bgelens added 3 commits July 5, 2017 21:27

fixed parameter validation logic (and removed from Set). fixed some o…

36332f4

…f the styling and indentation issues. added comment based help

removed parameter validation which couldn't be reached anymore

5a56a9f

changed to make use of unit test template 1.2.0 and adjusted tests to…

0e5da37

… check parameter conditions

bgelens approved these changes Jul 5, 2017

View reviewed changes

bgelens added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jul 5, 2017

bgelens added 2 commits July 6, 2017 22:44

added test for coverage of Test-TargetResource and GetNameWithExtensi…

cf3f1d7

…on. Fixed throw messages

added testcases for Get-TargetResource

84c4c0c

bgelens mentioned this pull request Jul 6, 2017

xHyper-V - Fix issues flagged by scriptanalyzer #117

Closed

3 tasks

fixed bug where incorrect type threw an error instead of stopping the…

5fd5a00

… process. Fixed a bug when MaximumSizeBytes was not specified, resize was wrongly initialized.

added Set test cases

bb5ac47

fixes on reviewable comments @PlagueHO

1da8a25

bgelens mentioned this pull request Jul 8, 2017

Vhd: Set-TargetResource uses try catch mechanism as logic to adjust or create a VHD #121

Open

fixed style from xVHD examples

1326858

bgelens merged commit c044b73 into dsccommunity:dev Jul 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xVHD - Add support for setting disk type (Fixed, Dynamic, Differencing) #77

xVHD - Add support for setting disk type (Fixed, Dynamic, Differencing) #77

johnf commented Jan 8, 2017 •

edited by bgelens

Loading

msftclas commented Jan 8, 2017

PlagueHO commented Jan 8, 2017

johnf commented Jan 8, 2017

PlagueHO commented Jan 9, 2017

johnf commented Jun 12, 2017

bgelens commented Jul 3, 2017

codecov-io commented Jul 4, 2017 •

edited

Loading

johnf commented Jul 4, 2017

bgelens left a comment

bgelens Jul 4, 2017

bgelens Jul 4, 2017

bgelens Jul 4, 2017

bgelens Jul 4, 2017

bgelens Jul 4, 2017

johnf commented Jul 5, 2017

bgelens commented Jul 5, 2017

PlagueHO commented Jul 7, 2017

bgelens commented Jul 8, 2017

PlagueHO commented Jul 8, 2017

bgelens commented Jul 8, 2017

PlagueHO commented Jul 8, 2017

xVHD - Add support for setting disk type (Fixed, Dynamic, Differencing) #77

xVHD - Add support for setting disk type (Fixed, Dynamic, Differencing) #77

Conversation

johnf commented Jan 8, 2017 • edited by bgelens Loading

msftclas commented Jan 8, 2017

PlagueHO commented Jan 8, 2017

johnf commented Jan 8, 2017

PlagueHO commented Jan 9, 2017

johnf commented Jun 12, 2017

bgelens commented Jul 3, 2017

codecov-io commented Jul 4, 2017 • edited Loading

Codecov Report

johnf commented Jul 4, 2017

bgelens left a comment

Choose a reason for hiding this comment

bgelens Jul 4, 2017

Choose a reason for hiding this comment

bgelens Jul 4, 2017

Choose a reason for hiding this comment

bgelens Jul 4, 2017

Choose a reason for hiding this comment

bgelens Jul 4, 2017

Choose a reason for hiding this comment

bgelens Jul 4, 2017

Choose a reason for hiding this comment

johnf commented Jul 5, 2017

bgelens commented Jul 5, 2017

PlagueHO commented Jul 7, 2017

bgelens commented Jul 8, 2017

PlagueHO commented Jul 8, 2017

bgelens commented Jul 8, 2017

PlagueHO commented Jul 8, 2017

johnf commented Jan 8, 2017 •

edited by bgelens

Loading

codecov-io commented Jul 4, 2017 •

edited

Loading