Add support for embedded teaming

[BrianFarnhill]

Fixes #95

Added support for the embedded teaming option that is new in server 2016.

@kwirkykat @mbreakey3 Do you guys know who is able to review stuff for this module? I don't see anyone on the maintainers list.

---

This change is 

[msftclas]

@BrianFarnhill,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

[iainbrighton]

@BrianFarnhill It looks like this functionality has been earmarked to get merged into the main xNetworking module from @rchaganti 's cHyper-V module. It's probably having a chat with Ravi and @PlagueHO to ensure that there's no duplication or additional waste of effort here.

[rchaganti]

I am waiting on completing all unit/integration tests. But, if this PR has all needed ingredients, we should be OK.

[rchaganti]

@BrianFarnhill if it's OK with you, I will merge my cVMSwitch as xVMSwitch here. My module has localized messages and other changes needed for making this a HQRM resource. I am trying to see if we can avoid any rework on this.

[BrianFarnhill]

@iainbrighton Why would we put this in to xNetworking though? This is the hyper-v specific implementation of the teaming which is different to the standard network teaming (well not under the hood, but in terms of how it's managed at least). Given the management of it is handled by hyper-v with the hyper-v commandlets when you do the teaming like this, I would argue that this belongs to the Hyper-V module. Would love to get @PlagueHO thoughts on this though.

@rchaganti Happy for you to put a pull request to my fork with the changes you have so they can get included in this PR. I was going to do another run over this whole module for the HQRM standards when I got some time as well so if you have some work already started there that would be great!

[rchaganti]

So, @BrianFarnhill I reviewed the differences (your PR vs what I have in cVMSwitch) in the module code and see that you have a subset of what I am doing. So, to do this the right way, let us get your updates merged here and I will open another PR with my updates. That way, you don't have to update this PR. Makes sense?

[BrianFarnhill]

@rchaganti Sure - so who is reviewing and merging my PR here then?

[johlju]

That would be @bgelens who is maintainer for this module. He will come around here soon I imagine.

[PlagueHO]

There is support in xNetworking for teaming adapters, but as @BrianFarnhill says, this is a different thing that enabling teams on a Hyper-V VM Switch. So it definitely makes more sense to be in here.

I think @bgelens is starting to catch up on all this stuff now.

[iainbrighton]

@PlagueHO I meant that @rchaganti had earmarked/mentioned that he "was looking to merge" his xVMSwitch implementation into the xNetworking module - not that this has been done. I just wanted to make sure there was no duplicated effort! It sounds like @BrianFarnhill and Ravi now have it under control... 👀

[PlagueHO]

@iainbrighton - Oh right! Doh! My lack of comprehension there. But yes, I don't think xNetworking is the place right for xVMSwitch - xHyper-V is definitely where people are going to expect to find it (at least that is where I'd look 😁 )

[bgelens]

@BrianFarnhill I'll look at this as soon as I can :) I just started maintaining a couple of the repos and am trying to get my grips on the backlog on all of them.

[bgelens]

@BrianFarnhill Thanks for your hard work! I finally had some time to review. So sorry for the delay (there is a lot of backlog to deal with in this repo).
Please review / address my comment. I am a bit worried about the resource itself though. It doesn't seem to be a solid reliable one and maybe needs an overhaul / redesign which is why I pinged some others in one of the comments to have some additional opinions.

---

Reviewed 2 of 4 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 20 unresolved discussions.

---

README.md, line 67 at r2 (raw file):

* **Name**: The desired VM Switch name
* **Type**: The desired type of switch: { External | Internal | Private }
* **NetAdapterName**: Network adapter name/s for external switch type

Could you change name/s to name(s)

---

README.md, line 71 at r2 (raw file):

* **EnableEmbeddedTeaming**: Should embedded NIC teaming be used (Windows Server 2016 only)
* **BandwidthReservationMode**: Specify the QoS mode used (options other than NA are only supported on Hyper-V 2012+): { Default | Weight | Absolute | None | NA }.
* **Ensure**: Ensures that the VM Switch is Present or Absent

Would you mind updating all of these under xVMSwitch so they state the data type and dsc property type (as with xVMDvdDrive)?
e.g:

• [Boolean] EnableEmbeddedTeaming (Write): Specifies switch embedded NIC teaming should be used (Windows Server 2016 only)

---

README.md, line 105 at r2 (raw file):

* Fix bug in xVMDvdDrive with hardcoded VM Name.
* Corrected Markdown rule violations in Readme.md.
* Added support for Embedded NIC teaming in Server 2016 to xVMSwitch

please rephrase:
MSFT_xVMHyperV: Added support for Switch Embedded Teaming (SET) in Server 2016

---

README.md, line 771 at r2 (raw file):

            Ensure = 'Present'
            Name   = 'Hyper-V'
        }

The Hyper-V PowerShell module needs to be installed as well or the xHyper-V resources won't work. Please add another WindowsFeature resource to install this

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 31 at r2 (raw file):

    {
        $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescriptions).Name
        $description = $switch.NetAdapterInterfaceDescriptions

I'm not too font on this construct as it's really hard to see the diff being a plural against a singular. Could you make this more elegant?

Isn't it so that the Plural property is only populated when it's a SET switch?
You can make it something like:

if ($switch.EmbeddedTeaminEnabled)
{
    # when SET, use NetAdapterInterfaceDescriptions (plural)
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescriptions).Name
    $description = $switch.NetAdapterInterfaceDescriptions
}
else
{
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescription -ErrorAction SilentlyContinue).Name
    $description = $switch.NetAdapterInterfaceDescription
}

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 46 at r2 (raw file):

    if($switch.BandwidthReservationMode -ne $null)
    {
        $returnValue['BandwidthReservationMode'] = $switch.BandwidthReservationMode

I know this is not code from your PR but please update so BandwidthReservationMode is always part of the hashtable. All properties in the schema should be accounted for and no more as defined in the schema may exist.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 91 at r2 (raw file):

    if ($EnableEmbeddedTeaming -eq $true -and (Get-OSVersion).Major -lt 10)
    {
        throw "Embedded teaming is only supported on Windows Server 2016"

Please make use of New-InvalidArgumentError from HyperVCommon instead of throw. Doing this makes sure we can add tests for the specific error.

For an example see: https://github.com/PowerShell/xHyper-V/blob/fabd28b7f5292cf1e36d7e0a572c06683f40541e/DSCResources/MSFT_xVMDvdDrive/MSFT_xVMDvdDrive.psm1#L399

---

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

                }
            }
            else 

remove trailing whitespace behind else

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 132 at r2 (raw file):

            {
                Write-Verbose -Message "The switch $Name EnableEmbeddedTeaming is incorrect ..."
                $removeReaddSwitch = $true

any idea why this is called removeReaddSwitch all over the place? Could you make an effort to rename these to removeReadSwitch?

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

    }

    if(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))

Why do we have 2 ways of detecting the OS version now? Can you update so only one is used (I prefer Get-WmiObject to be replaced)

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 273 at r2 (raw file):

    if ($EnableEmbeddedTeaming -eq $true -and (Get-OSVersion).Major -lt 10)
    {
        throw "Embedded teaming is only supported on Windows Server 2016"

Please make use of New-InvalidArgumentError from HyperVCommon instead of throw. Doing this makes sure we can add tests for the specific error.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 346 at r2 (raw file):

                        else 
                        {
                            Write-Verbose -Message "Switch $Name has a correct list of network adapters"    

so if $null -eq $switch.NetAdapterInterfaceDescriptions all is good? With embedded teaming it's not possible to have no NetAdapterInterfaceDescriptions I think. If this is an impossible situation, the if / else construct can be removed, the content from the else scriptblock can be removed and the content of the if scriptblock can be moved to the parent else.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 371 at r2 (raw file):

                # Only check embedded teaming if specified
                if ($PSBoundParameters.ContainsKey("EnableEmbeddedTeaming") -eq $true)

It is possible this construct is never called due to the return $true right after the if ($PSBoundParameters.ContainsKey("AllowManagementOS")) construct.
So if AllowManagementOS is not provided, the logic ends there.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 376 at r2 (raw file):

                    if($switch.EmbeddedTeamingEnabled -eq $EnableEmbeddedTeaming -or $null -eq $switch.EmbeddedTeamingEnabled)
                    {
                        Write-Verbose -Message "Switch $Name has correct EnableEmbeddedTeaming or it does not apply to this OS"

this is the final call in the if ($Ensure -eq 'Present') where is the true when this complies?

I think this resource needs some general redesign pinging @PlagueHO, @iainbrighton and @rchaganti for input

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):

<#
.SYNOPSIS
Returns the OS version, it is put here so we can mock it for tests only though

but it's used in the code. So not only for mocks right?

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 1 at r2 (raw file):

[CmdletBinding()]

please make use of the test templates
for the guideline on how to use these see TestsGuidelines

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 159 at r2 (raw file):

        }

        Context "A virtual switch with embedded teaming does not exist but should" {

Missing a test for OS version lower then required and an error should be thrown

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 181 at r2 (raw file):

            it "Should run the set method without exceptions" {
                Set-TargetResource @testParams

Missing test like Assert Get-VMSwitch and New-VMSwitch is called exactly 1 time, etc. This will make sure which code path has been used.
Example if Get-VMSwitch was called twice, the code path for an existing switch is followed which is something we don't want. So to spot regression it is good to add these.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 210 at r2 (raw file):

            it "Should return true in the test method" {
                Test-TargetResource @testParams | Should Be $true

I'm pretty sure this is the $true from if ($PSBoundParameters.ContainsKey("AllowManagementOS")) {

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 256 at r2 (raw file):

            it "Should run the set method without exceptions" {
                Set-TargetResource @testParams

Missing the assert called tests (won't repeat for the rest)

---

Comments from Reviewable

[PlagueHO]

Sorry about all the style nitpicks - but I figure as we'll want to bring this up to HQRM then we should be knocking these out now rather than later. I didn't list them all but the ones I mentioned usually occur more than once. Great job on the feature though - it will be awesome!

---

Reviewed 2 of 4 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 60 unresolved discussions.

---

README.md, line 746 at r2 (raw file):

This configuration will create an external VM Switch that uses multiple NICs
and embedded teaming, on Hyper-V host that runs Server 2016

nitpick: ...teaming, on a Hyper-V...

---

README.md, line 753 at r2 (raw file):

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

Move $NodeName = 'localhost' to next line and add [Parameter()]

---

README.md, line 755 at r2 (raw file):

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

        [Parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

---

README.md, line 756 at r2 (raw file):

        [Parameter(Mandatory)]
        [string]$SwitchName,

Move $SwitchName to next line

---

README.md, line 758 at r2 (raw file):

        [string]$SwitchName,

        [Parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

---

README.md, line 759 at r2 (raw file):

        [Parameter(Mandatory)]
        [string[]]$NetAdapterNames

Move $NetAdapterNames to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 39 at r2 (raw file):

        AllowManagementOS     = $switch.AllowManagementOS
        EnableEmbeddedTeaming = $switch.EmbeddedTeamingEnabled
        Ensure                = if($switch){'Present'}else{'Absent'}

If only we had a ternary operator in PS but should be converted to:

$Ensure = if($switch)
{
'Present'
}
else{
'Absent'
}

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 44 at r2 (raw file):

    }

    if($switch.BandwidthReservationMode -ne $null)

Space after if and $null on left side of comparison.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 58 at r2 (raw file):

    param
    (
        [parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

---

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

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

Move $Name to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 61 at r2 (raw file):

        [String]$Name,

        [parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 63 at r2 (raw file):

        [parameter(Mandatory)]
        [ValidateSet("External","Internal","Private")]
        [String]$Type,

Move $Type to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 65 at r2 (raw file):

        [String]$Type,

        [ValidateNotNullOrEmpty()]

Add [Parameter()] above

---

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

        [ValidateNotNullOrEmpty()]
        [String[]]$NetAdapterName,

Move $NetAdapterName to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 68 at r2 (raw file):

        [String[]]$NetAdapterName,

        [Boolean]$AllowManagementOS = $false,

Add [Parameter()] above

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):

        [Boolean]$AllowManagementOS = $false,

        [Boolean]$EnableEmbeddedTeaming = $false,

Add [Parameter()] above

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):

        [Boolean]$AllowManagementOS = $false,

        [Boolean]$EnableEmbeddedTeaming = $false,

Move $EnableEmbeddedTeaming to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 72 at r2 (raw file):

        [Boolean]$EnableEmbeddedTeaming = $false,

        [ValidateSet("Default","Weight","Absolute","None","NA")]

Add [Parameter()] above

---

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

        [ValidateSet("Default","Weight","Absolute","None","NA")]
        [String]$BandwidthReservationMode = "NA",

Move $BandwidthReservationMode to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 75 at r2 (raw file):

        [String]$BandwidthReservationMode = "NA",

        [ValidateSet("Present","Absent")]

Add [Parameter()] above

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r2 (raw file):

        [ValidateSet("Present","Absent")]
        [String]$Ensure = "Present"

Move $Ensure to next line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 79 at r2 (raw file):

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

Add space after if - also appears elsewhere - suggest search/replace.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 81 at r2 (raw file):

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

See @bgelen's comment about throw further down

---

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

        Throw "Please ensure that the Hyper-V role is installed with its PowerShell module"
    }
    # Check to see if the BandwidthReservationMode chosen is supported in the OS

Add blank line after end of if block - also appears in other places. Please see styleguidelines.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 84 at r2 (raw file):

    }
    # Check to see if the BandwidthReservationMode chosen is supported in the OS
    elseif(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))

Add space after elseif - again, suggest search replace.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 86 at r2 (raw file):

    elseif(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))
    {
        Throw "The BandwidthReservationMode cannot be set on a Hyper-V version lower than 2012"

See @bgelen's comment about throw further down

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 122 at r2 (raw file):

            }
            
            if(($BandwidthReservationMode -ne "NA") -and ($switch.BandwidthReservationMode -ne $BandwidthReservationMode))

Please add space after if.

Also, convert to single quotes around NA.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 135 at r2 (raw file):

            }

            if($removeReaddSwitch)

Please add space after if

---

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

                $switch | Remove-VMSwitch -Force
                $parameters = @{}
                $parameters["Name"] = $Name

Please convert to single quotes - this needs to be done all over.

---

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

                    $parameters["MinimumBandwidthMode"] = $BandwidthReservationMode
                }
                if($PSBoundParameters.ContainsKey("AllowManagementOS"))

Add blank line after if block

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 150 at r2 (raw file):

                    $parameters["AllowManagementOS"] = $AllowManagementOS
                }
                if($PSBoundParameters.ContainsKey("EnableEmbeddedTeaming"))

Add blank line after if block

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 154 at r2 (raw file):

                    $parameters["EnableEmbeddedTeaming"] = $EnableEmbeddedTeaming
                }
                $null = New-VMSwitch @parameters

Add blank line after if block

---

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

    param
    (
        [parameter(Mandatory)]

Can this parameter block please be fixed as per previous comments?

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

    }

    if(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))

Suggest using the OS detection method from PSDscResources and add the function to the common helper module.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 268 at r2 (raw file):

    if(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))
    {
        Throw "The BandwidthReservationMode cannot be set on a Hyper-V version lower than 2012"

Throw using commo

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):

                        Write-Verbose -Message "Checking if Switch $Name has correct NetAdapterInterface ..."
                        $adapter = $null
                        try

This try catch block seems to serve no purpose. The SilentlyContinue prevents it from ever hitting the catch block anyway.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 402 at r2 (raw file):

}

<#

This should be moved to the Helper functions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 8 at r2 (raw file):

    [Write, Description("Network adapter name/s for external switch type")] String NetAdapterName[];
    [Write, Description("Specify if the VM host has access to the physical NIC")] Boolean AllowManagementOS;
    [Write, Description("Support embedded teaming on this switch? (Server 2016 only)")] Boolean EnableEmbeddedTeaming;

Text should match readme parameter.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 78 at r2 (raw file):

            )
        }

Please remove extra blank lines here.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 172 at r2 (raw file):

            }

            it "Should return absent in the get method" {

Use capital I for it - suggest using search replace to fix all.

---

Comments from Reviewable

[BrianFarnhill]

@PlagueHO You're killing me dude, I was just following the style of the existing code because I didn't wanna go through and touch all the bits that weren't mine :P I'll see what I can do with it though!

[PlagueHO]

Sorry @BrianFarnhill 😁 there is no need to do the HQRM stuff for the code not changed in the PR - we can do this at a later date (it'll happen sometime). I'm just in the habit of looking at the whole file (thanks @johlju 😜 ). I definitely won't hold this PR up for that. So just do what you have time for.

[BrianFarnhill]

Review status: 3 of 6 files reviewed at latest revision, 60 unresolved discussions, some commit checks failed.

---

README.md, line 67 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Could you change name/s to name(s)

Done.

---

README.md, line 71 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Would you mind updating all of these under xVMSwitch so they state the data type and dsc property type (as with xVMDvdDrive)?
e.g:

• [Boolean] EnableEmbeddedTeaming (Write): Specifies switch embedded NIC teaming should be used (Windows Server 2016 only)

Done.

---

README.md, line 105 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

please rephrase:
MSFT_xVMHyperV: Added support for Switch Embedded Teaming (SET) in Server 2016

Done.

---

README.md, line 771 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

The Hyper-V PowerShell module needs to be installed as well or the xHyper-V resources won't work. Please add another WindowsFeature resource to install this

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 31 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

I'm not too font on this construct as it's really hard to see the diff being a plural against a singular. Could you make this more elegant?

Isn't it so that the Plural property is only populated when it's a SET switch?
You can make it something like:

if ($switch.EmbeddedTeaminEnabled)
{
    # when SET, use NetAdapterInterfaceDescriptions (plural)
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescriptions).Name
    $description = $switch.NetAdapterInterfaceDescriptions
}
else
{
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescription -ErrorAction SilentlyContinue).Name
    $description = $switch.NetAdapterInterfaceDescription
}

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 46 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

I know this is not code from your PR but please update so BandwidthReservationMode is always part of the hashtable. All properties in the schema should be accounted for and no more as defined in the schema may exist.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 91 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Please make use of New-InvalidArgumentError from HyperVCommon instead of throw. Doing this makes sure we can add tests for the specific error.

For an example see: https://github.com/PowerShell/xHyper-V/blob/fabd28b7f5292cf1e36d7e0a572c06683f40541e/DSCResources/MSFT_xVMDvdDrive/MSFT_xVMDvdDrive.psm1#L399

Done.

---

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

Previously, bgelens (Ben Gelens) wrote…

remove trailing whitespace behind else

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 132 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

any idea why this is called removeReaddSwitch all over the place? Could you make an effort to rename these to removeReadSwitch?

My best guess here is that it's been abbreviated from "Remove and re-add" (thus RemoveReadd and not an abbreviated for of "Remove Read"). So I don't think this needs to be changed.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 273 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Please make use of New-InvalidArgumentError from HyperVCommon instead of throw. Doing this makes sure we can add tests for the specific error.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 346 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

so if $null -eq $switch.NetAdapterInterfaceDescriptions all is good? With embedded teaming it's not possible to have no NetAdapterInterfaceDescriptions I think. If this is an impossible situation, the if / else construct can be removed, the content from the else scriptblock can be removed and the content of the if scriptblock can be moved to the parent else.

I had the wrong wording on the verbose message here. Basically what we are testing for here is that if you desire embedded teaming to be enabled (checked at the outer if) that there should be a descriptions value (in the plural property) and if not, return false and say its wrong. So we were returning false but outputting the wrong text which is why I think this got confused, so I fixed that but left the logic as is.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 376 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

this is the final call in the if ($Ensure -eq 'Present') where is the true when this complies?

I think this resource needs some general redesign pinging @PlagueHO, @iainbrighton and @rchaganti for input

I rearranged the return $true statements to basically say "if I get to the end of it and haven't found a reason to return false that I'll just return true" so that should help the flow of things here I think.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

but it's used in the code. So not only for mocks right?

It is used in the code, but the only reason it exists as a cmdlet is so we can mock it. I can mock a cmdlet but I can't (easily) override a system type to mock it's value, so we drop it in this cmdlet for that reason. Happy to update the synopsis to make that clearer if you think it needs it.

---

Comments from Reviewable

[BrianFarnhill]

Review status: 3 of 6 files reviewed at latest revision, 60 unresolved discussions, some commit checks failed.

---

README.md, line 746 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nitpick: ...teaming, on a Hyper-V...

Done.

---

README.md, line 753 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $NodeName = 'localhost' to next line and add [Parameter()]

Done.

---

README.md, line 755 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Change to [Parameter(Mandatory = $true)]

Done.

---

README.md, line 756 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $SwitchName to next line

Done.

---

README.md, line 758 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Change to [Parameter(Mandatory = $true)]

Done.

---

README.md, line 759 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $NetAdapterNames to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 39 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

If only we had a ternary operator in PS but should be converted to:

$Ensure = if($switch)
{
'Present'
}
else{
'Absent'
}

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 44 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Space after if and $null on left side of comparison.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 58 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Change to [Parameter(Mandatory = $true)]

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $Name to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 61 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Change to [Parameter(Mandatory = $true)]

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 63 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $Type to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 65 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add [Parameter()] above

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $NetAdapterName to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 68 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add [Parameter()] above

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add [Parameter()] above

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $EnableEmbeddedTeaming to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 72 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add [Parameter()] above

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $BandwidthReservationMode to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 75 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add [Parameter()] above

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Move $Ensure to next line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 79 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add space after if - also appears elsewhere - suggest search/replace.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 81 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See @bgelen's comment about throw further down

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line after end of if block - also appears in other places. Please see styleguidelines.

This one is an elseif, so I assumed that rule doesn't apply here. But will fix other instances

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 84 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add space after elseif - again, suggest search replace.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 86 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See @bgelen's comment about throw further down

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 122 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please add space after if.

Also, convert to single quotes around NA.

Am I missing something or is there nothing in the current version of the style guideline about the whole single vs double quotes thing. I know it technically makes a difference to processing, but are we seriously at the point where we are enforcing that sort of crap where it will in most cases make little to no difference?

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 135 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please add space after if

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please convert to single quotes - this needs to be done all over.

See above question on this style request

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line after if block

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 150 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line after if block

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 154 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line after if block

Done.

---

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this parameter block please be fixed as per previous comments?

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Suggest using the OS detection method from PSDscResources and add the function to the common helper module.

What method are you meaning? A quick search and I found this - https://github.com/PowerShell/PSDscResources/blob/15b40ddabdf6a46e40fc17d0d07535cf12580c89/DscResources/MSFT_WindowsFeature/MSFT_WindowsFeature.psm1#L549 - but it's doing literally what we are doing, it's just not in a common helper thing in this case. So is the issue here that this should go in the helpers module but the approach is fine? I just wanted to check before processing this change

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Why do we have 2 ways of detecting the OS version now? Can you update so only one is used (I prefer Get-WmiObject to be replaced)

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 268 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Throw using commo

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This try catch block seems to serve no purpose. The SilentlyContinue prevents it from ever hitting the catch block anyway.

Not true, I was seeing scenarios where the SilentlyContinue was still seeing exceptions be thrown - I know it makes no sense and that it shouldn't behave like that, but it absolutely was which is why I had to include this to get it working

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 371 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

It is possible this construct is never called due to the return $true right after the if ($PSBoundParameters.ContainsKey("AllowManagementOS")) construct.
So if AllowManagementOS is not provided, the logic ends there.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 402 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This should be moved to the Helper functions.

Leaving this one for now based on the question above about the approach here

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 8 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Text should match readme parameter.

Done.

---

Comments from Reviewable

[BrianFarnhill]

Review status: 0 of 6 files reviewed at latest revision, 60 unresolved discussions.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 1 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

please make use of the test templates
for the guideline on how to use these see TestsGuidelines

Done.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 78 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Please remove extra blank lines here.

Done.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 159 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Missing a test for OS version lower then required and an error should be thrown

Done.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 172 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Use capital I for it - suggest using search replace to fix all.

Done.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 181 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Missing test like Assert Get-VMSwitch and New-VMSwitch is called exactly 1 time, etc. This will make sure which code path has been used.
Example if Get-VMSwitch was called twice, the code path for an existing switch is followed which is something we don't want. So to spot regression it is good to add these.

Done.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 210 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

I'm pretty sure this is the $true from if ($PSBoundParameters.ContainsKey("AllowManagementOS")) {

Done.

---

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 256 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Missing the assert called tests (won't repeat for the rest)

Done.

---

Comments from Reviewable

[BrianFarnhill]

@PlagueHO @bgelens OK I've put all of the code review feedback in to the latest updates on here, hopefully that should be enough to get it over the line.

[bgelens]

Thanks, I did an initial review of some of it but don't have time to complete just now (also need to do my day job ;) ). I'll do the rest later today or tomorrow.

---

Reviewed 1 of 7 files at r4.
Review status: 1 of 6 files reviewed at latest revision, 56 unresolved discussions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

Not true, I was seeing scenarios where the SilentlyContinue was still seeing exceptions be thrown - I know it makes no sense and that it shouldn't behave like that, but it absolutely was which is why I had to include this to get it working

OK I know about a lot of cmdlets not respecting ErrorAction and have created these kind of solutions for other work as well.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 23 at r4 (raw file):

    -Path (Split-Path -Path $PSScriptRoot -Parent) `
    -ChildPath '\HyperVCommon\HyperVCommon.psm1' )
    

There are 4 spaces here, could you remove them please so it's just a blank line

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):

    -ChildPath '\HyperVCommon\HyperVCommon.psm1' )
    
function Get-TargetResource

Could you add Synopsis and Parameter help for all functions
See functions-have-comment-based-help for guidance

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 71 at r4 (raw file):

    }

    

Could you reduce the amount of blank newlines to just 1 and make sure the newline isn't filled with spaces.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 90 at r4 (raw file):

    else 
    {
        $returnValue['BandwidthReservationMode'] = 'NA'   

Thanks for adding this! Could you remove the 3 trailing spaces after 'NA' please

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):

                }

                New-VMSwitch @parameters | Out-Null

Could you revert these (I see a couple). $null = is preferred over | Out-Null in general.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 341 at r4 (raw file):

            -ErrorMessage $LocalizedData.NetAdapterNameRequiredError
    }
    

could you remove the 1 excessive new line and also the spaces on the other

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 407 at r4 (raw file):

                        catch 
                        {
                            Write-Verbose -Message "Network adapter not found"

These messages should also be localized as with the error messages.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 471 at r4 (raw file):

                }

                return $true                

please remove trailing spaces

---

Comments from Reviewable

[PlagueHO]

Sorry about the false negatives @BrianFarnhill - looking good to go from my perspective. I'm thinking we should get the new VS Code settings added to this repo sooner rather than later as it'll easily clean up many of these issues that had to be done manually.

---

Reviewed 5 of 7 files at r4.
Review status: all files reviewed at latest revision, 18 unresolved discussions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 122 at r2 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

Am I missing something or is there nothing in the current version of the style guideline about the whole single vs double quotes thing. I know it technically makes a difference to processing, but are we seriously at the point where we are enforcing that sort of crap where it will in most cases make little to no difference?

I do agree - but didn't realize it wasn't in the guidelines. It most likely is a case of someone having given this comment to some of my PR's and I've just assumed it was a style guideline. So just ignore. Sorry mate!

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

What method are you meaning? A quick search and I found this - https://github.com/PowerShell/PSDscResources/blob/15b40ddabdf6a46e40fc17d0d07535cf12580c89/DscResources/MSFT_WindowsFeature/MSFT_WindowsFeature.psm1#L549 - but it's doing literally what we are doing, it's just not in a common helper thing in this case. So is the issue here that this should go in the helpers module but the approach is fine? I just wanted to check before processing this change

Nope - please disregard this. It came from a feature branch I never got committed. I apologize for the mistake.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

OK I know about a lot of cmdlets not respecting ErrorAction and have created these kind of solutions for other work as well.

Ok that is useful to know. I hadn't encountered this problem before. Might be worth a comment explaining so I don't forget and decide it makes no sense again 😁

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 90 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Thanks for adding this! Could you remove the 3 trailing spaces after 'NA' please

I'm thinking that once the standard VS Code settings are added to the repo, whitespace automatically gets trimmed - which will be really handy.

---

Comments from Reviewable

[bgelens]

Awesome work @BrianFarnhill! couple of last things and it's good to go. I've added a comment about how to fix the Get-DscResource failure as well.

---

Reviewed 5 of 7 files at r4.
Review status: all files reviewed at latest revision, 20 unresolved discussions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

It is used in the code, but the only reason it exists as a cmdlet is so we can mock it. I can mock a cmdlet but I can't (easily) override a system type to mock it's value, so we drop it in this cmdlet for that reason. Happy to update the synopsis to make that clearer if you think it needs it.

The New-Guid function that ships with PSv5+ does a simular thing. Looking at it's synopsis: Creates a GUID
Looking at it's definition:

        [OutputType([System.Guid])]
    Param()

    Begin
    {
        [Guid]::NewGuid()
    }

Why I point this out.. The function is used in the code as a normal function. IMO it should just state what it does and not that for you the purpose was mocking 😄 Make sense?

Synopsis would be, Returns the OS version.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):

function Get-OSVersion
{
    return [Environment]::OSVersion.Version

there is no need for the return statement here. Please remove it

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r4 (raw file):

        Name                  = $switch.Name
        Type                  = $switch.SwitchType
        NetAdapterName        = $netAdapterName

It was mentioned in issue 95 that Get-DscResource is now broken with the updates from this PR.
The schema of the resource is updated so that NetAdapterName is a array of strings [string[]]
What you need to do here to fix the issue is typecast the netAdapterName variable as an array of strings.

$returnValue = @{
    Name                  = $switch.Name
    Type                  = $switch.SwitchType
    NetAdapterName        = [string[]]$netAdapterName
    AllowManagementOS     = $switch.AllowManagementOS
    EnableEmbeddedTeaming = $switch.EmbeddedTeamingEnabled
    Ensure                = $ensure
    Id                    = $switch.Id
    NetAdapterInterfaceDescription = $description
}

Get-DscResource will now return properly.
An integration test would have pointed out the issue when testing locally. If you feel like it, you could contribute a basic integration test which shouldn't run on appveyor (I'm not making that a requirement for this PR though so if don't want to, please raise an issue for it instead).

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 6 at r4 (raw file):

    [Key, Description("Name of the VM Switch")] String Name;
    [Key, Description("Type of switch"), ValueMap{"External","Internal","Private"}, Values{"External","Internal","Private"}] String Type;
    [Write, Description("Network adapter name/s for external switch type")] String NetAdapterName[];

please update to name(s) instead of name/s

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 27 at r4 (raw file):

        # Function to create a exception object for testing output exceptions
        function Get-InvalidArguementError

Isn't it Argument?

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):

            return $errorRecord
        } # end function Get-InvalidArguementError
        

please remove the spaces from the newline

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 109 at r4 (raw file):

        }

While you are at it, could you remove excessive newlines here please?

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 170 at r4 (raw file):

        }

Please remove excessive new lines

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 175 at r4 (raw file):

        }

Too many newlines here as well

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 291 at r4 (raw file):

            # Test Test-TargetResource when the version of Windows doesn't support BandwidthReservationMode
            It 'Invalid Operating System Exception' {
                

Please remove spaces on newline. Great you added this test!!

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 312 at r4 (raw file):

            }
        }

Please remove these excessive newlines as well

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 371 at r4 (raw file):

                                -ErrorId 'BandwidthReservationModeError' `
                                -ErrorMessage $LocalizedData.BandwidthReservationModeError
                {Set-TargetResource -Name 'WeightBRM' -Type 'External' -NetAdapterName 'SomeNIC' -AllowManagementOS $true -BandwidthReservationMode 'Weight' -Ensure 'Present'} | Should Throw $errorRecord

Great! Could you also add a test where the OS version is correct. Test it to should not throw

---

Comments from Reviewable

[BrianFarnhill]

Review status: all files reviewed at latest revision, 21 unresolved discussions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Ok that is useful to know. I hadn't encountered this problem before. Might be worth a comment explaining so I don't forget and decide it makes no sense again 😁

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

The New-Guid function that ships with PSv5+ does a simular thing. Looking at it's synopsis: Creates a GUID
Looking at it's definition:

        [OutputType([System.Guid])]
    Param()

    Begin
    {
        [Guid]::NewGuid()
    }

Why I point this out.. The function is used in the code as a normal function. IMO it should just state what it does and not that for you the purpose was mocking 😄 Make sense?

Synopsis would be, Returns the OS version.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

there is no need for the return statement here. Please remove it

What's wrong with specifically stating it rather than just assuming it hits the output stream and comes out that way?

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 23 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

There are 4 spaces here, could you remove them please so it's just a blank line

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Could you add Synopsis and Parameter help for all functions
See functions-have-comment-based-help for guidance

Can I challenge you on this one? Since there is no way to call the help for these directly from DSC why are we including them here? I know in other modules we don't (we do include it for cmdlets and functions that aren't actually DSC ones, so things that someone might legitimately call get-help on) so I'm curious why we would do it like this here?

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 71 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Could you reduce the amount of blank newlines to just 1 and make sure the newline isn't filled with spaces.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

It was mentioned in issue 95 that Get-DscResource is now broken with the updates from this PR.
The schema of the resource is updated so that NetAdapterName is a array of strings [string[]]
What you need to do here to fix the issue is typecast the netAdapterName variable as an array of strings.

$returnValue = @{
    Name                  = $switch.Name
    Type                  = $switch.SwitchType
    NetAdapterName        = [string[]]$netAdapterName
    AllowManagementOS     = $switch.AllowManagementOS
    EnableEmbeddedTeaming = $switch.EmbeddedTeamingEnabled
    Ensure                = $ensure
    Id                    = $switch.Id
    NetAdapterInterfaceDescription = $description
}

Get-DscResource will now return properly.
An integration test would have pointed out the issue when testing locally. If you feel like it, you could contribute a basic integration test which shouldn't run on appveyor (I'm not making that a requirement for this PR though so if don't want to, please raise an issue for it instead).

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 90 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I'm thinking that once the standard VS Code settings are added to the repo, whitespace automatically gets trimmed - which will be really handy.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Could you revert these (I see a couple). $null = is preferred over | Out-Null in general.

According to who? This way we don't end up with an unused variable (which will get highlighted in VS Code and I suspect means will turn up in the PS Script Analyser as an unused variable as well).

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 341 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

could you remove the 1 excessive new line and also the spaces on the other

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 407 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

These messages should also be localized as with the error messages.

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 471 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

please remove trailing spaces

Done.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 6 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

please update to name(s) instead of name/s

Done.

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 27 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Isn't it Argument?

I copy/pasted this from the tests in the VHD DVD Drive ... Have fixed

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

please remove the spaces from the newline

OK look - I didn't make changes to this test file, and I'm really starting to feel that I'm being asked to clean up everyone elses code rather than just get the change through that I started in the first place. The only reason this file got updated at all was based on me needing to introduce a change to how the error handling got done in the main module. I get that you all want to push towards meeting the style guidelines and I'm more than happy to do that for code I added, but I'm not going to go through and do that right now for every file that I touch a handful of lines in - it's just not practical. So I won't be making any further changes to this file.

---

Comments from Reviewable

[bgelens]

Thank you @BrianFarnhill!! You've gone out of your way 😄 Sorry to be so pushy..
Please address the last couple of points and I'm sure we can merge this asap

---

Reviewed 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

What's wrong with specifically stating it rather than just assuming it hits the output stream and comes out that way?

return is a keyword with special meaning / function. In this occurrence it isn't necessary to use. Also it is totally legitimate to assume that the resulting object is send to the output stream.

When compared to the return statements in Test-TargetResource, these serve the purpose of breaking out of processing the rest of the code.

return doesn't add value in this case so please remove it.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

Can I challenge you on this one? Since there is no way to call the help for these directly from DSC why are we including them here? I know in other modules we don't (we do include it for cmdlets and functions that aren't actually DSC ones, so things that someone might legitimately call get-help on) so I'm curious why we would do it like this here?

OK I'll do this myself together with the new line / spaces removal left over after this PR is merged (you've done so much to improve this resource already!).

No need to challenge me 😄, it is stated in the guideline and I'm just trying to enforce it. If you don't like the comment based help in here, please raise an issue at the DscResource GitHub repo.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

According to who? This way we don't end up with an unused variable (which will get highlighted in VS Code and I suspect means will turn up in the PS Script Analyser as an unused variable as well).

It is common sense that piping something to Out-Null is overhead (slower, more cycles, etc) compared to assigning $null or casting to void. I don't believe there is an official statement from the PG but you can measure this yourself.

I just checked vs code and it doesn't complain about $null being an unassigned variable. PSScriptAnalyzer is used in vs code so it on it's own won't complain about it either.

Please revert the change

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):

Previously, BrianFarnhill (Brian Farnhill) wrote…

OK look - I didn't make changes to this test file, and I'm really starting to feel that I'm being asked to clean up everyone elses code rather than just get the change through that I started in the first place. The only reason this file got updated at all was based on me needing to introduce a change to how the error handling got done in the main module. I get that you all want to push towards meeting the style guidelines and I'm more than happy to do that for code I added, but I'm not going to go through and do that right now for every file that I touch a handful of lines in - it's just not practical. So I won't be making any further changes to this file.

OK, I understand why this is getting a little frustrating to you and I agree with you it shouldn't be your work to fix every detail in here. My apologies to have pushed you with this (you have fixed a lot already!! thank you). I'll fix the rest myself.

Please put Won't fix in the other review comments so I can acknowledge them

---

Comments from Reviewable

[BrianFarnhill]

Review status: 6 of 7 files reviewed at latest revision, 11 unresolved discussions.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

return is a keyword with special meaning / function. In this occurrence it isn't necessary to use. Also it is totally legitimate to assume that the resulting object is send to the output stream.

When compared to the return statements in Test-TargetResource, these serve the purpose of breaking out of processing the rest of the code.

return doesn't add value in this case so please remove it.

Done. I still disagree with you on this point but will just make the change at this point

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

OK I'll do this myself together with the new line / spaces removal left over after this PR is merged (you've done so much to improve this resource already!).

No need to challenge me 😄, it is stated in the guideline and I'm just trying to enforce it. If you don't like the comment based help in here, please raise an issue at the DscResource GitHub repo.

Sorry the reason I challenged you is because I believe the intent on the guideline there is that you should have comment based help where it will actually be used by get-help, since there is no way to pull that comment based help out for a DSC resource's get/test/set functions there is no need to have it on those functions. That's the reason that I wrote (and @PlagueHO integrated in to the main DSCResources repo) a bunch of documentation helpers for DSC modules, so we could do help in ways that would actually be surfaced to end users. Anyway if you're happy to let this go through without it on there then it's fine - just wanted to put that thought out there for you.

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

It is common sense that piping something to Out-Null is overhead (slower, more cycles, etc) compared to assigning $null or casting to void. I don't believe there is an official statement from the PG but you can measure this yourself.

I just checked vs code and it doesn't complain about $null being an unassigned variable. PSScriptAnalyzer is used in vs code so it on it's own won't complain about it either.

Please revert the change

Sure

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

OK, I understand why this is getting a little frustrating to you and I agree with you it shouldn't be your work to fix every detail in here. My apologies to have pushed you with this (you have fixed a lot already!! thank you). I'll fix the rest myself.

Please put Won't fix in the other review comments so I can acknowledge them

Won't fix

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 109 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

While you are at it, could you remove excessive newlines here please?

Won't fix

---

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 371 at r4 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Great! Could you also add a test where the OS version is correct. Test it to should not throw

Won't fix

---

Comments from Reviewable

[bgelens]

---

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

---

Comments from Reviewable

[bgelens]

Thanks @BrianFarnhill!