johlju commented 7 years ago

Pull Request (PR) description

Changes to xFailOverCluster
- Added a common resource helper module with helper functions for localization.
- Added helper functions; Get-LocalizedData, New-InvalidResultException, New-ObjectNotFoundException, InvalidOperationException and InvalidArgumentException.
- Fixed lint error MD034 and fixed typos in README.md.
Changes to xClusterDisk
- Enabled localization for all strings (issue #84).
Changes to xClusterPreferredOwner
- Replace an URL which describes more generic the Role setting for xClusterNetwork.

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

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?
[x] Examples appropriately updated?
[x] New/changed code adheres to Style Guidelines?
[x] 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 #111 into dev will increase coverage by 1%. The diff coverage is 100%.

@@        Coverage Diff         @@
##           dev   #111   +/-   ##
==================================
+ Coverage   88%    90%   +1%     
==================================
  Files        6      7    +1     
  Lines      325    379   +54     
==================================
+ Hits       289    343   +54     
  Misses      36     36

johlju commented 7 years ago

@bgelens When you have some time over to review here in xFailOverCluster, could you then please review this one first? I need this merged to be able to do localization for the other resources. 😄

johlju commented 7 years ago

@PlagueHO since @bgelens is very busy with the normal day job. Do you have time to review this one? It basically just adds the localization helper functions, and localizes one of the resource (and some minor tweaks). When I get this in I can localize the rest of the resources. 😄

PlagueHO commented 7 years ago

Hi @johlju - I definitely will but it will be in about 48 hours. I've taken a short holiday in the mountains in NZ for a bit of snow. Will be back in review zone in 2 days :grin:

johlju commented 7 years ago

No worries :) I have also taken 48 hours leave from DSC, but for some summer and nature 😜

PlagueHO commented 7 years ago

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

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

  - Added a common resource helper module with helper functions for localization.
    - Added helper functions; Get-LocalizedData, New-InvalidResultException,
      New-ObjectNotFoundException, InvalidOperationException and InvalidArgumentException.

Shouldn't these be: New-InvalidOperationException and New-InvalidArgumentException

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

  - Fixed lint error MD034 and fixed typos in README.md.
- Changes to xClusterDisk
  - Enabled localization for all strings (issue #84).

Maybe remove # and include link to actual GitHub issue.

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

  - Enabled localization for all strings (issue #84).
- Changes to xClusterNetwork
  - Replaced an URL which describes more generic the Role setting for xClusterNetwork.

This doesn't read quite right. Perhaps something like: "Replaced the URL with a more generic one which describes the Role setting for xClusterNetwork."

Although, to be honest I'm not sure that is correct either as I didn't quite understand the exact change...

DSCResources/CommonResourceHelper.psm1, line 60 at r2 (raw file):

        $Message,

        [ValidateNotNull()]

Need: [Parameter()]

DSCResources/CommonResourceHelper.psm1, line 111 at r2 (raw file):

        $Message,

        [ValidateNotNull()]

Need: [Parameter()]

DSCResources/CommonResourceHelper.psm1, line 162 at r2 (raw file):

        $Message,

        [ValidateNotNull()]

Need: [Parameter()]

Tests/Unit/CommonResourceHelper.Tests.ps1, line 123 at r2 (raw file):

                    $mockErrorRecord = New-Object System.Management.Automation.ErrorRecord $mockException, $null, 'InvalidResult', $null

                    { New-InvalidResultException -Message $mockErrorMessage -ErrorRecord $mockErrorRecord } | Should Throw 'System.Exception: Mocked error ---> System.Exception: Mocked exception error message'

I'd suggest that if you're defining the $mockErrorMessage and $mockExceptionErrorMessage variables and creating the exception with it that you use these in the "Should Throw" - e.g. Should Throw ('System.Exception: {0} ---> System.Exception: {1}' -f $mockErrorMessage, $mockExceptionErrorMessage)

Same thing in the next few tests.

Comments from Reviewable

PlagueHO commented 7 years ago

@johlju - sometimes taking a break is a great idea :grin: Need to recharge every now and then.

johlju commented 7 years ago

@PlagueHO thanks for reviewing! Thanks for finding all these! Fixed some in xSQLServer too.

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

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