BREAKING CHANGE: Used ScopeId as a key property for xDhcpServerScope resource.

[bielawb]

This PR implements a change mentioned in #48.

On top of that I probably fixed few minor bugs in this resource (mainly in Get-TargetResource) and tried to update tests to use TestCases more, where it looked like an obvious choice.

I also added key property to the output of verbose messages in multiple places to make verbose message more useful.

Finally I added validations to Set-TargetResource - resources are used also outside of LCM/DSC scope (think Invoke-DscResource) so throwing from Test is not sufficient, when input data makes no sense it's important to not attempt to run a Set with it - it creates broken scopes.

Fixes #48

---

This change is 

[codecov-io]

Codecov Report

Merging #49 into dev will increase coverage by 3.19%. The diff coverage is 88.88%.

@@            Coverage Diff            @@
##              dev     #49      +/-   ##
=========================================
+ Coverage   66.61%   69.8%   +3.19%     
=========================================
  Files          11      11              
  Lines         608     659      +51     
  Branches        4       4              
=========================================
+ Hits          405     460      +55     
+ Misses        199     195       -4     
  Partials        4       4

Impacted Files	Coverage Δ
...s/MSFT_xDhcpServerClass/MSFT_xDhcpServerClass.psm1	73.17% <ø> (ø)	:arrow_up:
DSCResources/Helper.psm1	82% <78.12%> (+22%)	:arrow_up:
...s/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1	91.46% <94.82%> (+2.74%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 26bb39f...4713705. Read the comment docs.

[johlju]

Reviewed 5 of 5 files at r2. Review status: all files reviewed, 23 unresolved discussions (waiting on @bielawb)

---

a discussion (no related file): Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.

* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)

---

DSCResources/Helper.psm1, line 40 at r2 (raw file):

ipaddress

Can we change to System.Net.IPAddress?

---

DSCResources/Helper.psm1, line 97 at r2 (raw file):

}

# Internal function to assert if values of ScopeId/SubnetMask/IPStartRange/IPEndRange make sense. Implementation for IPv4.

We should have comment-based help on functions. https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions-have-comment-based-help

---

DSCResources/Helper.psm1, line 99 at r2 (raw file):

# Internal function to assert if values of ScopeId/SubnetMask/IPStartRange/IPEndRange make sense. Implementation for IPv4.

function Assert-ScopeParameter

Could we please add unit tests that tests this helper function individually (in a new Helper.Tests.ps1 file)? See other comment.

---

DSCResources/Helper.psm1, line 104 at r2 (raw file):

    param
    (
        [String]$ScopeId,

We should write parameters according to the style guideline: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-type-on-line-above

---

DSCResources/Helper.psm1, line 112 at r2 (raw file):

-ipString

-IpString (upper-case 'I'). Throughout.

---

DSCResources/Helper.psm1, line 112 at r2 (raw file):

-parameterName

-ParameterName (upper-case 'P'). Throughout.

---

DSCResources/Helper.psm1, line 127 at r2 (raw file):

-errorId

-ErrorId (upper 'E'). Same for the other named parameters on this row.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 22 at r2 (raw file):

}

function Get-TargetResource

We should have comment-based help on functions. Would you mind adding that throughout?

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 28 at r2 (raw file):

Mandatory

Mandatory = $true. If you would like, then please also do that throughout.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 29 at r2 (raw file):

    (
        [parameter(Mandatory)]
        [String]$ScopeId,

We should write parameters according to the style guideline: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-type-on-line-above

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 73 at r2 (raw file):

    {
        $ensure = 'Absent'
        $leaseDuration = ''

Curious. Is this meant to (must) return empty string and not $null?

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 125 at r2 (raw file):

    )

#region Input Validation

Please indent this.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 140 at r2 (raw file):

    Assert-ScopeParameter @ipAddressesAssertionParameters

#endregion Input Validation

Please indent this.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.schema.mof, line 4 at r2 (raw file):

class MSFT_xDhcpServerScope : OMI_BaseResource
{
    [Key, Description("ScopeId for the given scope")] String ScopeId;

We should add this parameter to the README.md too.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 77 at r2 (raw file):

It 'Throws an exception with information about incorrect <Parameter> (<Value>)' {

If we move this test to 'Helper.Tests.ps1' we don't need to test all aspects here? We should test that it throws though, but maybe it sufficient with one test if all the other variants are validated in a Helper.Tests.ps1?

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 77 at r2 (raw file):

It 'Throws an exception with information about incorrect <Parameter> (<Value>)' {

We should start It-blocks with 'Should...'. Also, maybe we can use 'Should throws the correct exception when...'

Throughout.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 85 at r2 (raw file):

                $brokenTestParams = $testParams.Clone()
                $brokenTestParams[$Parameter] = $Value
                try {

Instead of the try-catch-block we should use | Should -Throw $ErrorPattern because we want to test that it throws the correct error, not test string evaluation. Pester will correctly report that it expected a throw if the code does not throw when using Should -Throw. $ErrorPattern' should be changed to$ErrorMessage`.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 116 at r2 (raw file):

            It 'Returns a "System.Collections.Hashtable" object type' {
                Mock Get-DhcpServerv4Scope { return $fakeDhcpServerv4Scope; }
                $result = Get-TargetResource @testParams;

Can we tests so the Get-TargetResource returns the correct values?

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 120 at r2 (raw file):

                $result -is [System.Collections.Hashtable] | Should Be $true;
            }
        }

Can we also add a tests that tests so that the Get-TargetResource returns correct blank values when the scope does not exist?

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 236 at r2 (raw file):

Assert-MockCalled Add-DhcpServerv4Scope -Scope It;

Can we verify that this mock is called with the expected paramaters (using Assert-MockCalled -ParameterFilter). Also we should add -Exactly -Times 1 since we only want this to be called once.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 245 at r2 (raw file):

Assert-MockCalled Remove-DhcpServerv4Scope -Scope It;

Same as previous comment.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 254 at r2 (raw file):

Assert-MockCalled Set-DhcpServerv4Scope -Scope It

Same as previous comment.

---

Comments from Reviewable

[johlju]

Review status: all files reviewed, 24 unresolved discussions (waiting on @bielawb)

---

a discussion (no related file): Could you please add an example or two showing this new functionality? :slightly_smiling_face:

---

Comments from Reviewable

[johlju]

Review status: all files reviewed, 24 unresolved discussions (waiting on @bielawb)

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add an example or two showing this new functionality? :slightly_smiling_face:

Please update existing examples that using this resource.

---

Comments from Reviewable

[johlju]

@bielawb Great work on this! Thank you! 😃

[bielawb]

Thanks for comments, @johlju - I plan to address comments today. But before I start - what should I do with casing of parameters in Helper.psm1? I'm using them as they are defined, so I'd rather fix both call and definition.

On the other hand - if I do that I'm afraid next step will be a request to fix calls all over the place.. ;) ...and add help for changed commands ...and have 'correct' parameter definitions (in quotes, cause I disagree on [Parameter(Mandatory)] being incorrect, but I can close my eyes and write = $true if you insist). With some Select-String magic I can do that, but it sounds a lot like opening Pandora box...

[johlju]

Sorry for making a lot of style comments, I understand and saw that you followed how the previous code was written. Much of the existing code in the DSC Resource Kit are so far from the style guideline, so I try to at least get new changes to adhere to the style guideline. 🙂 I won't expect you to fix style in code that you haven't changed, but would be much appreciated for any style changes that help get the code closer to the style guideline. I won't ask for comment-based help, etc. for functions you fix style in (outside of your code changes). But if I do go overboard with the style comments, please comment that you think it should be done in another PR. 😃

The code will of course work with [Parameter(Mandatory)], it's just not following the style guideline. 🙂

Casing should be PascalCase for function names (and using verb-noun), PascalCase for parameter names and named parameters on function calls. For variables it should be camelCase. https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#function-names-use-pascal-case https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-names-use-pascal-case https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#variable-names-use-camel-case

[bielawb]

OK, one more thing... I noticed you used v4 Pester syntax in one of the comments - I was not aware that v4 is used for testing already. Would it make sense to add new tests using v4 syntax (with -Operators instead of Operators)?

[johlju]

We change to Pester v4 syntax where we can, but there is no hard rule. See https://github.com/PowerShell/DscResource.Tests/issues/199 for a gist that can be used to change this fast.

[bielawb]

Review status: 2 of 7 files reviewed, 24 unresolved discussions (waiting on @johlju and @bielawb)

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry. ``` * Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account) ```

Done.

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Please update existing examples that using this resource.

Done.

---

DSCResources/Helper.psm1, line 40 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > ipaddress > ``` Can we change to `System.Net.IPAddress`?

Done.

---

DSCResources/Helper.psm1, line 97 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have comment-based help on functions. https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions-have-comment-based-help

Done.

---

DSCResources/Helper.psm1, line 99 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we please add unit tests that tests this helper function individually (in a new Helper.Tests.ps1 file)? See other comment.

Done.

---

DSCResources/Helper.psm1, line 104 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should write parameters according to the style guideline: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-type-on-line-above

Done.

---

DSCResources/Helper.psm1, line 112 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > -ipString > ``` `-IpString` (upper-case 'I'). Throughout.

Done.

---

DSCResources/Helper.psm1, line 112 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > -parameterName > ``` `-ParameterName` (upper-case 'P'). Throughout.

Done.

---

DSCResources/Helper.psm1, line 127 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > -errorId > ``` `-ErrorId` (upper 'E'). Same for the other named parameters on this row.

Done.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 22 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have comment-based help on functions. Would you mind adding that throughout?

So it is required to have help for Get(), Test(), and Set() ? No problem - I can add it.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 28 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > Mandatory > ``` `Mandatory = $true`. If you would like, then please also do that throughout.

I don't like it, but guideline is a guideline... ;)

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 29 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should write parameters according to the style guideline: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-type-on-line-above

Done.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 73 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Curious. Is this meant to (must) return empty string and not $null?

Property is a string and [string]$null -> empty string... so result will be more or less the same. But I'm fine with either.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 125 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please indent this.

Done.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 140 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please indent this.

Done.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.schema.mof, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add this parameter to the README.md too.

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 77 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > It 'Throws an exception with information about incorrect ()' { > ``` If we move this test to 'Helper.Tests.ps1' we don't need to test all aspects here? We should test that it throws though, but maybe it sufficient with one test if all the other variants are validated in a Helper.Tests.ps1?

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 77 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > It 'Throws an exception with information about incorrect ()' { > ``` We should start It-blocks with 'Should...'. Also, maybe we can use `'Should throws the correct exception when...'` Throughout.

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 85 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instead of the try-catch-block we should use `| Should -Throw $ErrorPattern` because we want to test that it throws the correct error, not test string evaluation. Pester will correctly report that it expected a throw if the code does not throw when using `Should -Throw`. `$ErrorPattern' should be changed to `$ErrorMessage`.

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 116 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we tests so the Get-TargetResource returns the correct values?

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 120 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we also add a tests that tests so that the Get-TargetResource returns correct blank values when the scope does not exist?

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 236 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > Assert-MockCalled Add-DhcpServerv4Scope -Scope It; > ``` Can we verify that this mock is called with the expected paramaters (using `Assert-MockCalled -ParameterFilter`). Also we should add `-Exactly -Times 1` since we only want this to be called once.

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 245 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > Assert-MockCalled Remove-DhcpServerv4Scope -Scope It; > ``` Same as previous comment.

Done.

---

Tests/Unit/MSFT_xDhcpServerScope.Tests.ps1, line 254 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > Assert-MockCalled Set-DhcpServerv4Scope -Scope It > ``` Same as previous comment.

Done.

---

Comments from Reviewable

[johlju]

Reviewed 5 of 5 files at r4. Review status: all files reviewed, 2 unresolved discussions (waiting on @johlju and @bielawb)

---

DSCResources/Helper.psm1, line 104 at r2 (raw file):

Previously, bielawb (Bartek Bielawski) wrote…

Done.

Could we move the parameter name to a separate row (as per guideline)? Only need to do that for this helper function.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 22 at r2 (raw file):

Previously, bielawb (Bartek Bielawski) wrote…

So it is required to have help for Get(), Test(), and Set() ? No problem - I can add it.

Yes, to follow the guideline we should have comment-based helper for all functions, including these. Much appreciated if you want to add it, but leaving it as non-blocking.

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 73 at r2 (raw file):

Previously, bielawb (Bartek Bielawski) wrote…

Property is a string and [string]$null -> empty string... so result will be more or less the same. But I'm fine with either.

I'm good with it as -is. :slightly_smiling_face:

---

Tests/Unit/Helper.Tests.ps1, line 31 at r4 (raw file):

"$($script:ModuleName)\Assert-ScopeParameter"

$($script:ModuleName) won't work here since it declared outside of the InModuleScope-block. See https://ci.appveyor.com/project/PowerShell/xdhcpserver/build/1.5.127.0#L845 It's okay just to write 'Helper\Assert-ScopeParameter'.

---

Comments from Reviewable

[johlju]

@bielawb Awesome work on this! Thanks for fixing so much style issues! 😃

[bielawb]

Looks like I can't open reviewable from here - code should be ready for review though... :)

[johlju]

---

Reviewed 3 of 3 files at r5. Review status: :shipit: complete! all files reviewed, all discussions resolved

---

Comments from Reviewable

[johlju]

Review status: all files reviewed, 1 unresolved discussion (waiting on @bielawb)

---

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

### Unreleased
* Switch to ScopeId as a key property for xDhcpServerScope ([issue #43](https://github.com/PowerShell/xDhcpServer/issues/48). [Bartek Bielawski (@bielawb)](https://github.com/bielawb)

Forgot that this was a breaking change. We should prefix this row with

* BREAKING CHANGE: Switch ...

---

Comments from Reviewable

[bielawb]

Updated README with information about it being breaking change.

[bielawb]

Review status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

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

Previously, johlju (Johan Ljunggren) wrote…

Forgot that this was a breaking change. We should prefix this row with ``` * BREAKING CHANGE: Switch ... ```

Done.

---

DSCResources/Helper.psm1, line 104 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we move the parameter name to a separate row (as per guideline)? Only need to do that for this helper function.

Missed that part - moved variable part to the line below the type definition

---

DSCResources/MSFT_xDhcpServerScope/MSFT_xDhcpServerScope.psm1, line 22 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Yes, to follow the guideline we should have comment-based helper for all functions, including these. Much appreciated if you want to add it, but leaving it as non-blocking.

Help added

---

Tests/Unit/Helper.Tests.ps1, line 31 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

> ``` > "$($script:ModuleName)\Assert-ScopeParameter" > ``` `$($script:ModuleName)` won't work here since it declared outside of the `InModuleScope`-block. See https://ci.appveyor.com/project/PowerShell/xdhcpserver/build/1.5.127.0#L845 It's okay just to write `'Helper\Assert-ScopeParameter'`.

Tnx, missed that in output - hardcoded module name.

---

Comments from Reviewable

[johlju]

---

Reviewed 1 of 1 files at r6. Review status: :shipit: complete! all files reviewed, all discussions resolved

---

Comments from Reviewable

[johlju]

@bielawb Thanks for adding this! Awesome work! Thank you!