Blankf commented 7 years ago

Pull Request (PR) description

Changes to xClusterQuorum
- When using NodeAndFileShareMajority on Windows Server 2016 any subsequent run failed when Test-TargetResource validated the configuration.

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

Task list:

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

Previous PR-> https://github.com/PowerShell/xFailOverCluster/pull/141

This change is

codecov-io commented 7 years ago

Codecov Report

Merging #144 into dev will not change coverage. The diff coverage is n/a.

@@         Coverage Diff         @@
##            dev   #144   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         7      7           
  Lines       471    471           
===================================
  Hits        471    471

johlju commented 7 years ago

Thanks for sending is this PR! Really appreciate this! :smile:

Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 3 at r1 (raw file):

# Change log for xFailOverCluster

## Unreleased

Please fix lint errors in this README.md (tests are failing)

https://ci.appveyor.com/project/PowerShell/xfailovercluster/build/1.6.273.0#L374

CHANGELOG.md, line 5 at r1 (raw file):

server 2016 test failed on nodeandfilesharemajority if you used filesharemajority

Please make this more descriptive. What do you think of "When using NodeAndFileShareMajority on Windows Server 2016 any subsequent run failed when Test-TargetResource validated the configuration."?

README.md, line 183 at r1 (raw file):

  NodeMajority.

>Note: if you want to use FileShareWitness, use NodeAndFileShareMajority.

Please add the other two as well

NodeMajority = NoWitness NodeAndDiskMajority = DiskWitness

README.md, line 183 at r1 (raw file):

use NodeAndFileShareMajority

Maybe "..., use the option NodeAndFileShareMajority."?

DSCResources/MSFT_xClusterQuorum/MSFT_xClusterQuorum.psm1, line 42 at r1 (raw file):

                $clusterQuorumType = 'NodeAndDiskMajority'
            }
            elseif ($getClusterQuorumResult.QuorumResource.ResourceType.DisplayName -eq 'File Share Witness' -or $getClusterQuorumResult.QuorumResource.ResourceType.DisplayName -eq 'File Share Quorum Witness')

Could you please write (add) a unit test for this change?

Comments from Reviewable

Blankf commented 7 years ago

Hi Johlju,

Thank you for the review, you have a good point. changed the readme and changelog, need to take a look later at the tests.

johlju commented 7 years ago

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

README.md, line 185 at r3 (raw file):

>Note: if you want to use FileShareWitness, use the option NodeAndFileShareMajority.
>Note: if you want to use DiskWitness, use the option NodeAndDiskMajority.
>Note: if you want to use NoWitness, use the option NodeMajority.

Could you please revise these? If you look at the result here you see that they are concatenated, and 'Note:' is part of the paragraph. Maybe revise these so only one 'Note:' is needed.

Comments from Reviewable

Blankf commented 7 years ago

Hi johlju,

can you help me out with the checks, i have looked at them a couple of times but cant seem to get them fixed.

johlju commented 6 years ago

@Blankf Sorry that I haven't answered you for so long! 😞 Shall we try get this one over the finish line now? I can help you with the tests. Maybe you can resolve the review comments? If you don't have time then please check the 'Allow edit from maintainers' to the right here and I will get this one over the finish line,

Blankf commented 6 years ago

Hi Johlju,

i would really appriciate if you can finish the PR. at this moment, i dont have any time to do the fixes.

i've ticked the box "Allow edits from maintainers."

Thank you.

johlju commented 6 years ago

@Blankf Sure I will get this over the finish line. Thanks for ticking the box. And thanks for contirbuting, hope I will see you back here again!

johlju commented 6 years ago

Reviewed 4 of 4 files at r5. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

johlju commented 6 years ago

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

johlju commented 6 years ago

@Blankf So this one is over the finish line! Thanks for contribution this change, I hope I see you here again! 😄

dsccommunity / FailoverClusterDsc

xClusterQuorum: Fixed the filesharewitness checks #144

Codecov Report