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

Add DHCP support to xDNSServerAddress - Fixes #113 #216

Merged
merged 5 commits into from
Jun 16, 2017
Merged

Add DHCP support to xDNSServerAddress - Fixes #113 #216

merged 5 commits into from
Jun 16, 2017

Conversation

PlagueHO
Copy link
Member

@PlagueHO PlagueHO commented Jun 3, 2017

This PR adds support to use the xDNSServerAddress cmdlet to set DNS Server Addresses to be obtained from a DHCP server.

This PR also improves integration tests and should meet HQRM style guidelines. It also makes changes to support #176.

The changes in detail are:

  • Added Get-DnsClientServerStaticAddress to NetworkingDsc.Common to return statically
    assigned DNS server addresses to support fix for issue 113.
  • MSFT_xDNSserverAddress:
    • Added support for setting DNS Client to DHCP for issue 113.
    • Added new examples to show how to enable DHCP on DNS Client.
    • Improved integration test coverage to enable testing of multiple addresses and DHCP.
    • Converted exception creation to use common exception functions.
  • MSFT_xDhcpClient:
    • Updated example to also cover setting DNS Client to DHCP.

This will resolve #113 and #211


This change is Reviewable

  - Added support for setting DNS Client to DHCP for [issue 113](#113).
  - Added new examples to show how to enable DHCP on DNS Client.
  - Improved integration test coverage to enable testing of multiple addresses and DHCP.
  - Converted exception creation to use common exception functions.
- MSFT_xDhcpClient:
  - Updated example to also cover setting DNS Client to DHCP.
@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Jun 3, 2017
@PlagueHO PlagueHO requested a review from tysonjhayes June 3, 2017 23:51
@codecov-io
Copy link

codecov-io commented Jun 3, 2017

Codecov Report

Merging #216 into dev will decrease coverage by <1%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #216   +/-   ##
===================================
- Coverage    96%    96%   -1%     
===================================
  Files        17     17           
  Lines      1277   1276    -1     
===================================
- Hits       1229   1228    -1     
  Misses       48     48

@tysonjhayes
Copy link
Collaborator

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


Modules/xNetworking/DSCResources/MSFT_xDNSServerAddress/MSFT_xDNSServerAddress.psm1, line 118 at r2 (raw file):

        ) -join '')

    # If address not passed, set to an empty array

Feeling kind of forgetful at the moment. We have a validatenotnullorempty, basically we expect this code to be hit if the parameter is not used at all correct?


Modules/xNetworking/Examples/Resources/xDhcpClient/1-xDHCPClient_EnableDHCP.ps1, line 30 at r2 (raw file):

            AddressFamily  = 'IPv4'
        }
        

Looks like we have some blank spaces here.


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

                -f $InterfaceAlias)

        # Return null to support ErrorAction Silently Continue

I haven't seen this before, so if we don't return something SilentlyContinue doesn't work?


Comments from Reviewable

@PlagueHO
Copy link
Member Author

PlagueHO commented Jun 7, 2017

Thank you so much for reviewing @tysonjhayes! 😁


Review status: 13 of 14 files reviewed at latest revision, 3 unresolved discussions.


Modules/xNetworking/DSCResources/MSFT_xDNSServerAddress/MSFT_xDNSServerAddress.psm1, line 118 at r2 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

Feeling kind of forgetful at the moment. We have a validatenotnullorempty, basically we expect this code to be hit if the parameter is not used at all correct?

That is correct. I originally had the parameter defaulted to an empty array, but this results in a $null rather than empty array object and also throws because of the ValidateNotnulOrEmpty() I also wanted to ensure that a blank string or array wasn't passed through - instead the parameter should be left out entirely if DHCP should be enabled on DNS.


Modules/xNetworking/Examples/Resources/xDhcpClient/1-xDHCPClient_EnableDHCP.ps1, line 30 at r2 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

Looks like we have some blank spaces here.

Done.


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

Previously, tysonjhayes (Tyson J. Hayes) wrote…

I haven't seen this before, so if we don't return something SilentlyContinue doesn't work?

This is because if you call Get-DnsClientServerStaticAddress with -EA SilentlyContinue with an adapter that does not exist then what actually happens is New-InvalidOperationException is called which throws an exception, but the exception is suppressed and control returns to the line after New-InvalidOperationException. The return $null prevents the cmdlet from continuing past this point. The New-InvalidOperationException cmdlet comes from over in PSDscResources.


Comments from Reviewable

@PlagueHO
Copy link
Member Author

Hi @tysonjhayes - I've completed the changes as per your comments 😁 Any chance you could look them over if you get a chance?

@tysonjhayes
Copy link
Collaborator

:lgtm:


Review status: 13 of 14 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@tysonjhayes
Copy link
Collaborator

Reviewed 1 of 15 files at r1, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@tysonjhayes tysonjhayes merged commit f5a552b into dsccommunity:dev Jun 16, 2017
@joeyaiello joeyaiello removed the needs review The pull request needs a code review. label Jun 16, 2017
@PlagueHO
Copy link
Member Author

@tysonjhayes - thank you heaps sir! Appreciate it 👍

@PlagueHO PlagueHO deleted the Issue-113 branch June 16, 2017 22:15
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.

xDhcpClient doesn't set DNS servers back to DHCP
6 participants