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

xClusterPreferredOwner: Refactor unit test #98

Merged

Conversation

johlju
Copy link
Member

@johlju johlju commented Jul 6, 2017

Pull Request (PR) description

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

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

Refactored the unit test for this resource to use stubs and increase coverage (issue dsccommunity#76).
Changed the code to be more aligned with the style guideline.
Added examples
  - 1-AddPreferredOwner.ps1
  - 2-RemovePreferredOwner.ps1
Added links to examples from README.md.
@msftclas
Copy link

msftclas commented Jul 6, 2017

@johlju,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@johlju johlju added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Jul 6, 2017
@johlju
Copy link
Member Author

johlju commented Jul 6, 2017

Closing this and reopen to kick off the AppVeyor test again.

@johlju johlju closed this Jul 6, 2017
@johlju johlju reopened this Jul 6, 2017
@joeyaiello joeyaiello removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Jul 6, 2017
@msftclas
Copy link

msftclas commented Jul 6, 2017

@johlju,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #98 into dev will increase coverage by 12%.
The diff coverage is 100%.

Impacted file tree graph

@@        Coverage Diff         @@
##           dev   #98    +/-   ##
==================================
+ Coverage   70%   83%   +12%     
==================================
  Files        6     6            
  Lines      292   315    +23     
==================================
+ Hits       207   263    +56     
+ Misses      85    52    -33

@johlju johlju added the needs review The pull request needs a code review. label Jul 6, 2017
@johlju
Copy link
Member Author

johlju commented Jul 6, 2017

@bgelens This one is ready for review whenever you got time.

@johlju johlju requested a review from bgelens July 6, 2017 18:30
@bgelens
Copy link
Contributor

bgelens commented Jul 6, 2017

Small things. Looking good!


Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


CHANGELOG.md, line 81 at r2 (raw file):

    should remove a disk (issue #90).
  - Changed the code to be more aligned with the style guideline.
  - Added examples (issue ¤46)

Unicode char?


DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 64 at r2 (raw file):

                Write-Verbose -Message "Retrieving Owner information for Cluster Resource $resource"
                (((Get-ClusterResource -Cluster $ClusterName) | Where-Object -FilterScript {
                    $_.Name -like $resource

why like? are there wildcards allowed for $ClusterResources?


DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 163 at r2 (raw file):

        } | Get-ClusterOwnerNode).OwnerNodes).Name | Sort-Object -Unique

        $newOwners = @(

Personally I would think a resource like this handles only one state. So ensure doesn't make sense to me. Instead I would think the resource would govern the owners specified in the configuration and would purge anything else instead of being additive. I'll mark this as satisfied as it is not blocking but just wanted to give my opinion :)


Examples/Resources/xClusterPreferredOwner/2-RemovePreferredOwner.ps1, line 31 at r2 (raw file):

        xClusterPreferredOwner 'RemoveOwnersForClusterGroup2'
        {
            Ensure = 'Absent'

This is what I was talking about... The resource is handling desired state a an imperative operation. So to fix a situation, first I have to remove owners, then push another config that would be consistent with my desired state... see what I mean?


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jul 7, 2017

Review status: 7 of 8 files reviewed at latest revision, 3 unresolved discussions.


CHANGELOG.md, line 81 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Unicode char?

Done. Hit the wrong key ;)


DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 64 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

why like? are there wildcards allowed for $ClusterResources?

Done. Submitted issue #101 (BREAKING CHANGE: xClusterPreferredOwner: Resource allows wildcards for ClusterGroup and ClusterResources)


DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 163 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Personally I would think a resource like this handles only one state. So ensure doesn't make sense to me. Instead I would think the resource would govern the owners specified in the configuration and would purge anything else instead of being additive. I'll mark this as satisfied as it is not blocking but just wanted to give my opinion :)

Done. Submitted issue #102 (xClusterPreferredOwner: Is the behavior of parameter Ensure correct?) to discuss this.


Examples/Resources/xClusterPreferredOwner/2-RemovePreferredOwner.ps1, line 31 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

This is what I was talking about... The resource is handling desired state a an imperative operation. So to fix a situation, first I have to remove owners, then push another config that would be consistent with my desired state... see what I mean?

Done. Submitted issue #102 (xClusterPreferredOwner: Is the behavior of parameter Ensure correct?) to discuss this.


Comments from Reviewable

@bgelens
Copy link
Contributor

bgelens commented Jul 7, 2017

:lgtm:


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


Comments from Reviewable

@johlju johlju removed the needs review The pull request needs a code review. label Jul 7, 2017
@johlju johlju merged commit c625b02 into dsccommunity:dev Jul 7, 2017
@johlju johlju deleted the refactor-unit-test-for-xClusterPreferredOwner branch July 7, 2017 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants