Opt-in to validate localization, and Removed 'SupportsShouldProcess' - Fixes #217 #216

johlju · 2019-04-18T14:23:27Z

Pull Request (PR) description

Opt-in to common test to validate localization. Fixed localization strings
in resources - Fixes Issue #217.
PowerShellExecutionPolicy:
- Removed SupportsShouldProcess as it cannot be used with DSC - Fixes
  Issue #219.
Combined all ComputerManagementDsc.ResourceHelper module functions into
ComputerManagementDsc.Common module - Fixes Issue #218.
- Minor code cleanup against style guideline.
- Remove code from New-InvalidOperationException because it was a
  code path that could never could be used due to that the parameter
  validation prevented the helper function being called that way.
- Updated all Get-LocalizationData to latest version from
  DSCResource.Template.
- Fixed an issue with the helper function Test-IsNanoServer that
  prevented it to work. Though the helper function is not used, so this
  issue was not caught until now when unit tests was added.
- Improved code coverage.

This Pull Request (PR) fixes the following issues

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md in the resource folder.
Resource parameter descriptions added/updated in schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

codecov-io · 2019-04-18T14:31:12Z

Codecov Report

Merging #216 into dev will increase coverage by <1%.
The diff coverage is 93%.

@@         Coverage Diff          @@
##            dev   #216    +/-   ##
====================================
+ Coverage    85%    86%   +<1%     
====================================
  Files        11     10     -1     
  Lines      1242   1229    -13     
====================================
- Hits       1064   1059     -5     
+ Misses      178    170     -8

johlju · 2019-04-30T15:19:33Z

CLosing and reopening to kick off the updated localization common test.

PlagueHO · 2019-05-03T22:25:57Z

This should be able to come out of draft now that the PR to DSCResources.Test was merged.

johlju

Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @PlagueHO)

Modules/ComputerManagementDsc.ResourceHelper/ComputerManagementDsc.ResourceHelper.psm1, line 51 at r1 (raw file):

Quoted 8 lines of code…

<#
    Import localization strings. This must be called after the function declaration
    of Get-LocalizedData to be able to use localized strings in the helper
    functions of this module.
#>
$script:localizedData = Get-LocalizedData `
    -ResourceName 'ComputerManagementDsc.ResourceHelper' `
    -ResourcePath $PSScriptRoot

The change to this file is not necessary. But having the en-US folder a string resource file with no keys is necessary for the new common test to pass.
If you like I can remove this change since there are no localized messages in this file.

Depending on the issue PowerShell/DscResource.Template#21 there might be another solution.

Let me know what you like me to do here. Just keep an string resource file with no keys?

johlju · 2019-05-04T06:37:45Z

@PlagueHO I'm fixing stuff for this PR to increase coverage

johlju · 2019-05-04T07:54:03Z

@PlagueHO Not sure where the last 4 misses come from.

johlju · 2019-05-04T07:59:06Z

It ready for review again! Let me knoe if I need to squash more coverage :p

PlagueHO

Sorry for taking so long on this @johlju - I completely missed it. I maintain an e-mail folder that contains all the GitHub e-mails from PRs I'm trying to get onto, but this wasn't in there). Sorry about that - the good thing is it was on the back log of changes I'd be intending to make as soon as I had time - and you completed it already! 😁

Reviewed 5 of 12 files at r1, 10 of 13 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @johlju)

CHANGELOG.md, line 32 at r3 (raw file):

  - Minor code cleanup against style guideline.
  - Remove code from `New-InvalidOperationException` because it was a
    code path that could never could be used due to that the parameter

due to that the -> due to the

CHANGELOG.md, line 33 at r3 (raw file):

  - Remove code from `New-InvalidOperationException` because it was a
    code path that could never could be used due to that the parameter
    validation prevented the helper function being called that way.

prevented -> preventing

Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 4 at r3 (raw file):

    .SYNOPSIS
        Retrieves the localized string data based on the machine's culture.
        Falls back to en-US strings if the machine's culture is not supported.

Should be a blank line after each section here.

Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 64 at r3 (raw file):

<#
    .SYNOPSIS
    Tests if the current machine is a Nano server.

Can you indent this line?

Modules/ComputerManagementDsc.ResourceHelper/ComputerManagementDsc.ResourceHelper.psm1, line 51 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

<#
    Import localization strings. This must be called after the function declaration
    of Get-LocalizedData to be able to use localized strings in the helper
    functions of this module.
#>
$script:localizedData = Get-LocalizedData `
    -ResourceName 'ComputerManagementDsc.ResourceHelper' `
    -ResourcePath $PSScriptRoot
The change to this file is not necessary. But having the en-US folder a string resource file with no keys is necessary for the new common test to pass.
If you like I can remove this change since there are no localized messages in this file.

Depending on the issue PowerShell/DscResource.Template#21 there might be another solution.

Let me know what you like me to do here. Just keep an string resource file with no keys?

Lets keep the string resource file with no keys.

Tests/Unit/ComputerManagementDsc.Common.Tests.ps1, line 1107 at r3 (raw file):

        Describe 'DscResource.LocalizationHelper\New-InvalidOperationException' {
        Context 'When calling with Message parameter only' {

Indent seems to be wrong here.

Modules/ComputerManagementDsc.Common/en-US/ComputerManagementDsc.Common.strings.psd1, line 16 at r3 (raw file):

    TestDscParameterResultMessage      = Test-DscParameter result is '{0}'.
    CurrentTimeZoneMessage             = Current time zone is set to '{0}'
    GettingTimeZoneMessage             = Getting current time zone using {0}.

Can you put '' around {0} for consistency and readability?

…rings in resources.

- Removed `SupportsShouldProcess` as it cannot be used with DSC.

- Combined all ComputerManagementDsc.ResourceHelper module functions into ComputerManagementDsc.Common module

johlju

Ready for review again.

Reviewable status: 12 of 17 files reviewed, 6 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md, line 32 at r3 (raw file):