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

xSQLServerSetup: Fixes failover cluster bugs with shared disks #404

Merged
merged 10 commits into from
Feb 27, 2017

Conversation

johlju
Copy link
Member

@johlju johlju commented Feb 25, 2017

Changes to xSQLServerSetup

This Pull Request (PR) fixes the following issues:
Fixes #394
Fixes #400
Fixes #401

  • 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

Changed tests to be more dynamically and to find the bug in issue dsccommunity#401.
Now it can correctly determine the right cluster when only parameter InstallSQLDataDir is assigned a path (issue dsccommunity#401).
Now the only mandatory path parameter is InstallSQLDataDir when installing Database Engine (issue dsccommunity#400).
It now can handle mandatory parameters, and are not using wildcards to find the variables containing paths (issue dsccommunity#394).
@johlju johlju added the needs review The pull request needs a code review. label Feb 25, 2017
@codecov-io
Copy link

codecov-io commented Feb 25, 2017

Codecov Report

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

@@         Coverage Diff          @@
##            dev   #404    +/-   ##
====================================
+ Coverage    68%    68%   +<1%     
====================================
  Files        31     31            
  Lines      3171   3183    +12     
====================================
+ Hits       2180   2192    +12     
  Misses      991    991

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0af7566...dbcaabe. Read the comment docs.

Changed the logic of how the parameters are evaluated.
@johlju
Copy link
Member Author

johlju commented Feb 26, 2017

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


README.md, line 857 at r2 (raw file):

* Target machine must be running Windows Server 2008 R2 or later.
* For configurations that utilize the 'InstallFailoverCluster' action, the following parameters are required (beyond those required for the standalone installation)

Add full stop (.) to each sentence.


README.md, line 860 at r2 (raw file):

  * InstanceName (can be MSSQLSERVER if you want to install a default clustered instance)
  * FailoverClusterNetworkName
  * When installation SQL Server database engine:

Grammar wrong. 'Parameters mandatory when...'
Remove ':'


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 26, 2017
@johlju
Copy link
Member Author

johlju commented Feb 26, 2017

Looks good to me. Waiting until tomorrow to merge if someone else wants to comment on anything.


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


Comments from Reviewable

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

johlju commented Feb 27, 2017

:LGTM:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@johlju johlju merged commit 04237a5 into dsccommunity:dev Feb 27, 2017
@vors vors removed the needs review The pull request needs a code review. label Feb 27, 2017
@johlju johlju deleted the fix-cluster-bugs branch April 7, 2017 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment