dsccommunity / FailoverClusterDsc

This module contains DSC resources for deployment and configuration of Windows Server Failover Cluster.

MIT License

60 stars 54 forks source link

xClusterPreferredOwner: Enabled localization #117

Closed johlju closed 7 years ago

johlju commented 7 years ago

Pull Request (PR) description

Changes to xClusterPreferredOwner
- Enabled localization for all strings (issue #86).
- Fixed typo in the returned hash table from Get-TargetResource.

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

Task list:

[x] 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?
[x] New/changed code adheres to Style Guidelines?
[ ] Unit and (optional) Integration tests created/updated where possible?

This change is

msftclas commented 7 years ago

@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 commented 7 years ago

Codecov Report

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

@@         Coverage Diff         @@
##           dev   #117    +/-   ##
===================================
+ Coverage   99%    99%   +<1%     
===================================
  Files        7      7            
  Lines      381    383     +2     
===================================
+ Hits       379    381     +2     
  Misses       2      2

johlju commented 7 years ago

@PlagueHO do you have time to review this one for me? No hurry.

PlagueHO commented 7 years ago

No problem @johlju - will review tomorrow night after work (bed time now!)

PlagueHO commented 7 years ago

@Johlju - I figure you're going to be correcting the other issues to meet standards later on, so I didn't comment on anything outside the changes you made (it was difficult to resist the urge though :grin: )

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

Comments from Reviewable

johlju commented 7 years ago

@PlagueHO Actually I already run this thru "basic" style change cleanup. So please. if you see stuff I missed, then please comment on them, because then there is something I overlooked. I keep this PR open if you want to comment more. 😄

All resources should go through more detailed cleanup later. Planing creating an issue for each style guideline "rule" and fix all resources per issue later on, so this module can get to HQRM. But can we squash some things now, then that is even better.

PlagueHO commented 7 years ago

@johlju - actually, it was mainly comments querying why there were brackets around things that don't seem to need them. But everything else is OK!

$null = (Get-ClusterGroup -Cluster $ClusterName) |  .....

PlagueHO commented 7 years ago

Anyway...

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

Comments from Reviewable

johlju commented 7 years ago

@PlagueHO Aha, I missed those, or looked passed them because of other style changes. I will fix those in another PR, and go thru all resource at the same time :)

Big thanks for reviewing! 😄