Proposed fix for Issue #413

[MKletz]

Pull Request (PR) description

Adjusted logic for NetBios resource WMI filter to correctly accept * as the help implies.

This Pull Request (PR) fixes the following issues

Fixes #413

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in README.md in resource folder.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[msftclas]

All CLA requirements met.

[codecov-io]

Codecov Report

Merging #414 into dev will decrease coverage by <1%.
The diff coverage is 80%.

@@         Coverage Diff         @@
##            dev   #414   +/-   ##
===================================
- Coverage    95%    95%   -1%     
===================================
  Files        28     28           
  Lines      2272   2279    +7     
===================================
+ Hits       2172   2177    +5     
- Misses      100    102    +2

[PlagueHO]

Hi @MKletz - thanks for submitting this! Keen to get it in.

Can you make the following changes:

1. Add an entry into the # Unreleased section of the CHANGELOG.MD (use the same style as other entries)
2. Add some unit tests to cover these scenarios
3. Add an integration test to cover the scenario (if possible)
4. Add a new example to the examples folder showing the use of this pattern.
5. Add something to the description in the MOF and comment based help to indicate that the parameter supports wildcards.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @MKletz)

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 59 at r2 (raw file):

    Write-Verbose -Message ($script:localizedData.GettingNetBiosSettingMessage -f $InterfaceAlias)
    if( $InterfaceAlias.contains('*') )

Can you add a blank line before this as well as removing spaces inside of braces and adding a space after the if:
if ($InterfaceAlias.contains('*'))

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 59 at r2 (raw file):

    Write-Verbose -Message ($script:localizedData.GettingNetBiosSettingMessage -f $InterfaceAlias)
    if( $InterfaceAlias.contains('*') )

I also notice that there is a lot of code duplication (violation of DRY) now. Can we move this code into a new function:

<#
    .SYNOPSIS
        Get the CIM instance network adapters by name.

    .PARAMETER InterfaceAlias
       The name of the interface alias to return. May also include
       a * to return multiple matching adapters.
#>
function Get-NetAdapterCimInstance
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [System.String]
        $InterfaceAlias
    )

    if ($InterfaceAlias.contains('*'))
    {
        $InterfaceAlias = $InterfaceAlias.replace('*','%')
        $operator = " LIKE "
    }
    else
    {
        $operator = "="
    }

    return Get-CimInstance `
        -ClassName Win32_NetworkAdapter `
        -Filter ('NetConnectionID{0}"{1}"' -f $Operator,$InterfaceAlias)
}

You'd need to ensure unit tests are created to cover this function, but this would be pretty easy.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 62 at r2 (raw file):

    {
        $InterfaceAlias = $InterfaceAlias.replace('*','%')
        $Operator = " LIKE "

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 66 at r2 (raw file):

    else
    {
        $Operator = "="

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 71 at r2 (raw file):

    $netAdapter = Get-CimInstance `
        -ClassName Win32_NetworkAdapter `
        -Filter ('NetConnectionID{0}"{1}"' -f $Operator,$InterfaceAlias)

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 134 at r2 (raw file):

    Write-Verbose -Message ($script:localizedData.SettingNetBiosSettingMessage -f $InterfaceAlias)

    if( $InterfaceAlias.contains('*') )

Can you remove spaces inside of braces and adding a space after the if:
if ($InterfaceAlias.contains('*'))

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 137 at r2 (raw file):

    {
        $InterfaceAlias = $InterfaceAlias.replace('*','%')
        $Operator = " LIKE "

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 141 at r2 (raw file):

    else
    {
        $Operator = "="

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 146 at r2 (raw file):

    $netAdapter = Get-CimInstance `
        -ClassName Win32_NetworkAdapter `
        -Filter ('NetConnectionID{0}"{1}"' -f $Operator,$InterfaceAlias)

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 223 at r2 (raw file):

    Write-Verbose -Message ($script:localizedData.TestingNetBiosSettingMessage -f $InterfaceAlias)

    if( $InterfaceAlias.contains('*') )

Can you remove spaces inside of braces and adding a space after the if:
if ($InterfaceAlias.contains('*'))

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 226 at r2 (raw file):

    {
        $InterfaceAlias = $InterfaceAlias.replace('*','%')
        $Operator = " LIKE "

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 230 at r2 (raw file):

    else
    {
        $Operator = "="

Can you use lowercase O for $Operator and use single quotes around string.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 235 at r2 (raw file):

    $netAdapter = Get-CimInstance `
        -ClassName Win32_NetworkAdapter `
        -Filter ('NetConnectionID{0}"{1}"' -f $Operator,$InterfaceAlias)

Can you use lowercase O for $Operator and use single quotes around string.

[MKletz]

Thank you for the feedback working on these now. Do you think the function should go in common for use by other resources? It's fairly generic.

[MKletz]

@PlagueHO I believe I have your comments addressed now. This is my first open source contribution so I apologize if I'm missing obvious things standards wise.

[MKletz]

Reviewable status: 0 of 6 files reviewed, 13 unresolved discussions (waiting on @PlagueHO)

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 59 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a blank line before this as well as removing spaces inside of braces and adding a space after the if:
if ($InterfaceAlias.contains('*'))

Moved to common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 59 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I also notice that there is a lot of code duplication (violation of DRY) now. Can we move this code into a new function:

<#
    .SYNOPSIS
        Get the CIM instance network adapters by name.

    .PARAMETER InterfaceAlias
       The name of the interface alias to return. May also include
       a * to return multiple matching adapters.
#>
function Get-NetAdapterCimInstance
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [System.String]
        $InterfaceAlias
    )

    if ($InterfaceAlias.contains('*'))
    {
        $InterfaceAlias = $InterfaceAlias.replace('*','%')
        $operator = " LIKE "
    }
    else
    {
        $operator = "="
    }

    return Get-CimInstance `
        -ClassName Win32_NetworkAdapter `
        -Filter ('NetConnectionID{0}"{1}"' -f $Operator,$InterfaceAlias)
}

You'd need to ensure unit tests are created to cover this function, but this would be pretty easy.

Moved to common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 62 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 66 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 71 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 134 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove spaces inside of braces and adding a space after the if:
if ($InterfaceAlias.contains('*'))

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 137 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 141 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 146 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 223 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove spaces inside of braces and adding a space after the if:
if ($InterfaceAlias.contains('*'))

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 226 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 230 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 235 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lowercase O for $Operator and use single quotes around string.

Fixed in common function

[PlagueHO]

Cool! Thanks @MKletz - I'll get onto the review tonight or tomorrow night! Thank you heaps for this!

[PlagueHO]

Reviewed 1 of 3 files at r3, 2 of 4 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @MKletz)

---

CHANGELOG.md, line 7 at r5 (raw file):

- Added Comment Based Help for `New-NotImplementedException` common
  function - fixes [Issue #411](https://github.com/PowerShell/NetworkingDsc/issues/411).

Can you remove the blank lines between these entries?

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 60 at r5 (raw file):

    Write-Verbose -Message ($script:localizedData.GettingNetBiosSettingMessage -f $InterfaceAlias)

    $Win32NetworkADapterFilter = Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $InterfaceAlias

Can you use lower case first letter for local variables?

Also, it looks like the "AD" in Adapter should be lower case D. E.g. Format-Win32NetworkAdapterFilterByNetConnectionID

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 127 at r5 (raw file):

    Write-Verbose -Message ($script:localizedData.SettingNetBiosSettingMessage -f $InterfaceAlias)

    $Win32NetworkADapterFilter = Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $InterfaceAlias

Can you use lower case first letter for local variables?

Also, it looks like the "AD" in Adapter should be lower case D. E.g. Format-Win32NetworkAdapterFilterByNetConnectionID

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 208 at r5 (raw file):

    Write-Verbose -Message ($script:localizedData.TestingNetBiosSettingMessage -f $InterfaceAlias)

    $Win32NetworkADapterFilter = Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $InterfaceAlias

Can you use lower case first letter for local variables?

Also, it looks like the "AD" in Adapter should be lower case D. E.g. Format-Win32NetworkAdapterFilterByNetConnectionID

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1370 at r5 (raw file):

<#
.SYNOPSIS
    Returns a filter string for the net adapter CIM instances. Will detect wild cards and replace them and the operator accordingly.

Can you reduce the line length to below 80 chars (makes future reviewing easier).

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1375 at r5 (raw file):

    Specifies the alias of a network interface. Supports the use of '*' or '%'.
#>
function Format-Win32NetworkADapterFilterByNetConnectionID

Looks like the 'D' in Adapter should be changed to a lower case 'd'.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1377 at r5 (raw file):

function Format-Win32NetworkADapterFilterByNetConnectionID
{
[CmdletBinding()]

Can you indent attribute?

Also, can you add an [OutputType([System.String])] attribute after the CmdletBinding?

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1385 at r5 (raw file):

    )

    process

Is process block required? We don't typically include it unless we're using pipeline variables.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1387 at r5 (raw file):

    process
    {
        if($InterfaceAlias.contains('*'))

Can you use space after if?

Also, should be capital 'C' for Contains method.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1389 at r5 (raw file):

        if($InterfaceAlias.contains('*'))
        {
            $InterfaceAlias = $InterfaceAlias.replace('*','%')

Should be capital 'R' for Replace method.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1392 at r5 (raw file):

        }

        if($InterfaceAlias.contains('%'))

Can you use space after if?

Also, should be capital 'C' for Contains method.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1401 at r5 (raw file):

        }

        'NetConnectionID{0}"{1}"' -f $operator,$InterfaceAlias

Can you prepend with return to indicate it is the return variable?

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1429 at r5 (raw file):

    'ConvertTo-HashTable',
    'ConvertTo-CimInstance',
    'Format-Win32NetworkADapterFilterByNetConnectionID'

Looks like the 'D' in Adapter should be changed to a lower case 'd'.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2245 at r5 (raw file):

        Describe 'NetworkingDsc.Common\Format-Win32NetworkADapterFilterByNetConnectionID'{

Can you remove blank line here and the one inside Context block? And throughout. This is just to adopt consistent style. E.g. we don't include a blank line straight after the beginning of a block.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2248 at r5 (raw file):

            Context 'When interface alias has an ''*''' {

                $interfaceAlias = "Ether*"

Can you use single quotes?

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2251 at r5 (raw file):

                It 'Should convert the ''*'' to a ''%''' {
                    (Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $interfaceAlias).contains('%') -eq $True -and

Could these tests be simplified with something like:

Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $interfaceAlias | Should -BeExactly 'NetConnectionID LIKE Ether*'

This would then provide more explicit tests. It looks like the tests might still pass in situations that they shouldn't.

The same applies to the next Context block.

It might actually be simpler to use the It -TestCases to provide a set of input/outputs to validate.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2259 at r5 (raw file):

                }

                It 'Looks like a usable filter' {

'It' message should start with 'Should' - e.g. It 'Should look like a usable filter'.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2262 at r5 (raw file):

                    Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $interfaceAlias | Should -Be 'NetConnectionID LIKE "Ether%"'
                }

Can you remove this blank line? We don't include blank lines after block closure if followed by another block closure (sorry about style nitpicks 😁 )

[PlagueHO]

Hi @MKletz - let me know when you're good for me to review! Really keen to get this awesome addition into the kit.

[MKletz]

@PlagueHO sorry for the delay, missed the email for your last comment. I believe I've addressed your new comments

[MKletz]

Reviewed 1 of 3 files at r3, 1 of 4 files at r4, 3 of 4 files at r6, 1 of 1 files at r7.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @PlagueHO)

---

CHANGELOG.md, line 7 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove the blank lines between these entries?

Done.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 60 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lower case first letter for local variables?

Also, it looks like the "AD" in Adapter should be lower case D. E.g. Format-Win32NetworkAdapterFilterByNetConnectionID

Done.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 127 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lower case first letter for local variables?

Also, it looks like the "AD" in Adapter should be lower case D. E.g. Format-Win32NetworkAdapterFilterByNetConnectionID

Done.

---

DSCResources/MSFT_NetBios/MSFT_NetBios.psm1, line 208 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use lower case first letter for local variables?

Also, it looks like the "AD" in Adapter should be lower case D. E.g. Format-Win32NetworkAdapterFilterByNetConnectionID

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1370 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you reduce the line length to below 80 chars (makes future reviewing easier).

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1375 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Looks like the 'D' in Adapter should be changed to a lower case 'd'.

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1377 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you indent attribute?

Also, can you add an [OutputType([System.String])] attribute after the CmdletBinding?

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1385 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is process block required? We don't typically include it unless we're using pipeline variables.

force of habbit, I can remove.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1387 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use space after if?

Also, should be capital 'C' for Contains method.

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1389 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should be capital 'R' for Replace method.

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1392 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use space after if?

Also, should be capital 'C' for Contains method.

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1401 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you prepend with return to indicate it is the return variable?

Done.

---

Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 1429 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Looks like the 'D' in Adapter should be changed to a lower case 'd'.

Done.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2245 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove blank line here and the one inside Context block? And throughout. This is just to adopt consistent style. E.g. we don't include a blank line straight after the beginning of a block.

Done.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2248 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use single quotes?

Done.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2251 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could these tests be simplified with something like:

Format-Win32NetworkADapterFilterByNetConnectionID -InterfaceAlias $interfaceAlias | Should -BeExactly 'NetConnectionID LIKE Ether*'

This would then provide more explicit tests. It looks like the tests might still pass in situations that they shouldn't.

The same applies to the next Context block.

It might actually be simpler to use the It -TestCases to provide a set of input/outputs to validate.

Done.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2259 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

'It' message should start with 'Should' - e.g. It 'Should look like a usable filter'.

Done.

---

Tests/Unit/NetworkingDsc.Common.tests.ps1, line 2262 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove this blank line? We don't include blank lines after block closure if followed by another block closure (sorry about style nitpicks 😁 )

please nitpick away! This is my first of hopefully many contributions and need to be consistent

[PlagueHO]

No worries at all @MKletz- just great to have the contribution!

Reviewed 3 of 4 files at r6, 1 of 1 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved