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

xClusterNetwork: Fix Role property never in desired state #176

Merged
merged 6 commits into from
Feb 17, 2018

Conversation

claudiospizzi
Copy link
Contributor

@claudiospizzi claudiospizzi commented Feb 16, 2018

Pull Request (PR) description
Convert the network role to the enum value if it returns a stirng, e.g. on WS2016.

This Pull Request (PR) fixes the following issues:
Fixes #175

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Feb 16, 2018

Codecov Report

Merging #176 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #176   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         7      7           
  Lines       434    437    +3     
===================================
+ Hits        434    437    +3

@johlju johlju changed the title xClusterNetwork: xClusterNetwork: Fix Role property never in desired state Feb 16, 2018
@johlju johlju added the needs review The pull request needs a code review. label Feb 16, 2018
@johlju
Copy link
Member

johlju commented Feb 17, 2018

Reviewed 1 of 3 files at r1.
Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


DSCResources/MSFT_xClusterNetwork/MSFT_xClusterNetwork.psm1, line 40 at r1 (raw file):

    {
        # Try to convert the role to it's enum value
        $role = [String] [Int32] $NetworkResource.Role

Please see issue #175 for a suggestion that does need to try-catch a conversion. Would that be a better solution?


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Feb 17, 2018
@claudiospizzi
Copy link
Contributor Author

Added new test for the WS2016 role property


Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xClusterNetwork/MSFT_xClusterNetwork.psm1, line 40 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please see issue #175 for a suggestion that does need to try-catch a conversion. Would that be a better solution?

Done


Comments from Reviewable

@claudiospizzi
Copy link
Contributor Author

Done


Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Feb 17, 2018

Thanks for fixing the tests, appreciate it! I thought about it, but forgot to comment on that! Just one more comment about the test. I hope it will be as easy as the new test you already did. After that I think this is ready to merge. 😄


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


Tests/Unit/MSFT_xClusterNetwork.Tests.ps1, line 156 at r2 (raw file):

                }

                It 'Should return the the correct values for the cluster network' {

Could we add a similar test when the role is in desired state as well for the new code path?


Comments from Reviewable

@claudiospizzi
Copy link
Contributor Author

Done


Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Feb 17, 2018

:lgtm:


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


Comments from Reviewable

@johlju johlju merged commit fabf4dc into dsccommunity:dev Feb 17, 2018
@claudiospizzi claudiospizzi deleted the FixClusterNetworkRoleTest branch February 17, 2018 13:24
@claudiospizzi
Copy link
Contributor Author

claudiospizzi commented Feb 17, 2018

Thanks @johlju

@johlju
Copy link
Member

johlju commented Feb 17, 2018

@claudiospizzi no problem. Thanks for fixing this! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author response The pull request is waiting for the author to respond to comments in the pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xClusterNetwork: Role property never in desired state
3 participants