Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve xNetAdapterName Resource to handle already renamed adapters - Fixes #209 #210

Merged
merged 5 commits into from
Jun 4, 2017
Merged

Conversation

PlagueHO
Copy link
Member

@PlagueHO PlagueHO commented May 6, 2017

I've improved the log information as well and extended the integration and unit tests to provide better coverage of these scenarios.

The resource can now be correctly used to rename an adapter by just identifying it by name. But this isn't the primary user story that drove this. If you know the adapter name already, then renaming it isn't really that useful (but it'll work).

The primary user story that this should address is when you don't know the adapter name in the OS - you only know various parameters of the adapter (e.g. MAC Address, Driver) and therefore need to rename it so that that all other resources have a known name they can reference.

For example:

        xNetAdapterName RenameNetAdapterCluster
        {
            NewName        = 'Cluster'
            MacAddress     = '9C-D2-1E-61-B5-DA'
        }

This will work fine - but it's not really the primary purpose:

        xNetAdapterName RenameNetAdapterCluster
        {
            NewName        = 'Cluster'
            Name      = 'Ethernet 1'
        }

This change is Reviewable

@PlagueHO PlagueHO added help wanted The issue is up for grabs for anyone in the community. needs review The pull request needs a code review. and removed help wanted The issue is up for grabs for anyone in the community. labels May 6, 2017
@codecov-io
Copy link

codecov-io commented May 6, 2017

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #210    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        17     17            
  Lines      1273   1277     +4     
====================================
+ Hits       1225   1229     +4     
  Misses       48     48

@johlju
Copy link
Member

johlju commented Jun 1, 2017

While I was in review mode I did this one too 😄


Reviewed 10 of 11 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions.


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 115 at r2 (raw file):

    if (-not $adapter)
    {
        $PSBoundParameters.Remove('NewName')

Maybe write a verbose message her as well using $LocalizedData.GettingNetAdapterNameMessage but for the Name parameter?


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 237 at r2 (raw file):

        ) -join '')

    $null = $PSBoundParameters.Remove('NewName')

Maybe use the same method (style) as removing the parameter as it is done in the Get-method?


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 352 at r2 (raw file):

        ) -join '')

    $null = $PSBoundParameters.Remove('NewName')

Maybe use the same method (style) as removing the parameter as it is done in the Get-method?


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 370 at r2 (raw file):

    else
    {
        # Find an adapter matching the parameters - throw if none can be found

Maybe write a verbose message her as well using $LocalizedData.TestingNetAdapterNameMessage but for the Name parameter?


Modules/xNetworking/Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 223 at r2 (raw file):

        New-InvalidOperationException `
            -Message ($LocalizedData.NetAdapterNotFoundError)
        return $null

Why do you need return here when you are throwing before?


Modules/xNetworking/Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 241 at r2 (raw file):

                        -Message ($LocalizedData.InvalidNetAdapterNumberError `
                            -f $matchingAdapters.Count,$InterfaceNumber)
                    return $null

Same as above (just mark so I know where to verify)


Modules/xNetworking/Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 249 at r2 (raw file):

                    -Message ($LocalizedData.MultipleMatchingNetAdapterFound `
                        -f $matchingAdapters.Count)
                return $null

Same as above (just mark so I know where to verify)


Modules/xNetworking/Modules/NetworkingDsc.Common/en-us/NetworkingDsc.Common.strings.psd1, line 3 at r2 (raw file):

Found all Network Adapters because no filter parameters provided.

'network adapters' with lower-case?


Tests/Integration/MSFT_xNetAdapterName.Integration.Tests.ps1, line 103 at r2 (raw file):

        #region DEFAULT TESTS
        It 'should compile and apply the MOF without throwing' {

Should with upper 'S' (to match the one above) - and all the rest. TestsGuideline says 'Should' with upper 'S'.


Tests/Integration/MSFT_xNetAdapterName.Integration.Tests.ps1, line 119 at r2 (raw file):

                    -OutputPath $TestDrive `
                    -ConfigurationData $configData
                Start-DscConfiguration -Path $TestDrive `

Blank row before this one.


Tests/Unit/MSFT_xNetAdapterName.Tests.ps1, line 99 at r2 (raw file):

Assert-MockCalled -commandName Find-NetworkAdapter -Exactly 1

I think this should be -Exactly -Times 1

Also a bunch of them. :)


Tests/Unit/MSFT_xNetAdapterName.Tests.ps1, line 110 at r2 (raw file):

                    -ParameterFilter { $Name -eq $script:adapterName }

                It 'should not throw' {

Upper 'S' in 'Should' (an all others)


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

                    -Message ($LocalizedData.NetAdapterNotFoundError)

                It 'should throw exception' {

Should throw the correct error
(or exception)


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

                    -Message ($LocalizedData.MultipleMatchingNetAdapterFound -f 2)

                It 'should throw exception' {

Should throw the correct error
(or exception)


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

                    -MockWith { $adapterArray }

                It 'should throw exception' {

Should not throw an error
(or exception)


Comments from Reviewable

@PlagueHO
Copy link
Member Author

PlagueHO commented Jun 2, 2017

Wow! You're a legend @johlju - I'd even forgotten I had this one sitting around! 😁 I can now begin working on some new features and bug fixes! Go 🇸🇪 !


Review status: 3 of 10 files reviewed at latest revision, 15 unresolved discussions.


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 115 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe write a verbose message her as well using $LocalizedData.GettingNetAdapterNameMessage but for the Name parameter?

I've added the logging message, but I created a new message - as this search will most likely be querying using other things (not usually name) - e.g. MAC Address, adapter type.


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 237 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe use the same method (style) as removing the parameter as it is done in the Get-method?

I've actually changed the one in Get-TargetResource - that one was wrong - this is the correct method that should be used.


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 352 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe use the same method (style) as removing the parameter as it is done in the Get-method?

I've actually changed the one in Get-TargetResource - that one was wrong - this is the correct method that should be used.


Modules/xNetworking/DSCResources/MSFT_xNetAdapterName/MSFT_xNetAdapterName.psm1, line 370 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe write a verbose message her as well using $LocalizedData.TestingNetAdapterNameMessage but for the Name parameter?

Done - but as above it is a generic message because we do not know the search parameters that are being used.


Modules/xNetworking/Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 223 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why do you need return here when you are throwing before?

I've added a comment explaining this, but because I sometimes call this function with -EA SilentlyContinue (which prevents the exception from terminating the call) this ensures that a $null is always returned.


Modules/xNetworking/Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 241 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as above (just mark so I know where to verify)

I've added a comment explaining this, but because I sometimes call this function with -EA SilentlyContinue (which prevents the exception from terminating the call) this ensures that a $null is always returned.


Modules/xNetworking/Modules/NetworkingDsc.Common/NetworkingDsc.Common.psm1, line 249 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as above (just mark so I know where to verify)

I've added a comment explaining this, but because I sometimes call this function with -EA SilentlyContinue (which prevents the exception from terminating the call) this ensures that a $null is always returned.


Modules/xNetworking/Modules/NetworkingDsc.Common/en-us/NetworkingDsc.Common.strings.psd1, line 3 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Found all Network Adapters because no filter parameters provided.

'network adapters' with lower-case?

Done.


Tests/Integration/MSFT_xNetAdapterName.Integration.Tests.ps1, line 103 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should with upper 'S' (to match the one above) - and all the rest. TestsGuideline says 'Should' with upper 'S'.

Great catch! I've fixed the ones in this resource, but I'll do a global fix when I work on finishing bringing this up to HQRM (soon).


Tests/Integration/MSFT_xNetAdapterName.Integration.Tests.ps1, line 119 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this one.

Done.


Tests/Unit/MSFT_xNetAdapterName.Tests.ps1, line 99 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Assert-MockCalled -commandName Find-NetworkAdapter -Exactly 1

I think this should be -Exactly -Times 1

Also a bunch of them. :)

Done.


Tests/Unit/MSFT_xNetAdapterName.Tests.ps1, line 110 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Upper 'S' in 'Should' (an all others)

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Should throw the correct error
(or exception)

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Should throw the correct error
(or exception)

Done. And all the others too.


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

Previously, johlju (Johan Ljunggren) wrote…

Should not throw an error
(or exception)

Done.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Jun 3, 2017

Haha 😆 Sound great that we can get more stuff into this resource 😃

Two comments left, then this is good to go I think.


Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


CHANGELOG.md, line 28 at r3 (raw file):

  - Added Find-NetAdapter cmdlet to NetworkingDsc.Common.
- Correct example parameters format to meet style guidelines.
- Find-NetworkAdapter:

These should move to the unreleased section? Line 28-33


Modules/xNetworking/Modules/NetworkingDsc.Common/en-us/NetworkingDsc.Common.strings.psd1, line 3 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

The change was not done. And if you change that, change the rest of the strings appropriately as well.


Comments from Reviewable

@PlagueHO
Copy link
Member Author

PlagueHO commented Jun 3, 2017

All done! Thanks @johlju - I also added a missing entry to README.MD.


Review status: 8 of 11 files reviewed at latest revision, 2 unresolved discussions.


CHANGELOG.md, line 28 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

These should move to the unreleased section? Line 28-33

Doh - you're right - not sure how that happened... Fixed.


Modules/xNetworking/Modules/NetworkingDsc.Common/en-us/NetworkingDsc.Common.strings.psd1, line 3 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The change was not done. And if you change that, change the rest of the strings appropriately as well.

Doh! Sorry - I only changed the MSFT_xNetAdapterName.strings.psd1 file.

Fixed now.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Jun 4, 2017

:lgtm:

Great work @PlagueHO!


Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@PlagueHO
Copy link
Member Author

PlagueHO commented Jun 4, 2017

Thanks @johlju !

@PlagueHO PlagueHO merged commit 20c090a into dsccommunity:dev Jun 4, 2017
@vors vors removed the needs review The pull request needs a code review. label Jun 4, 2017
@PlagueHO PlagueHO deleted the Issue-209 branch June 4, 2017 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants