Add SystemLocale Resource - Fixes #50 #288

PlagueHO · 2019-10-29T20:27:05Z

Pull Request (PR) description

This PR migrates SystemLocale from SystemLocaleDsc.

This Pull Request (PR) fixes the following issues

Fixes Move SystemLocale resource from SystemLocaleDSC to xComputerManagement #50

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.

@johlju - would you mind reviewing this one?

This change is

johlju · 2019-10-29T20:28:14Z

Starting review. I see if I have time to finish, if not I will finish tomorrow.

johlju · 2019-10-29T20:29:08Z

Oh it become far less to review now after a correct rebase 😄

johlju

Reviewed 10 of 10 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @PlagueHO)

DSCResources/MSFT_SystemLocale/en-US/MSFT_SystemLocale.strings.psd1, line 3 at r1 (raw file):

Windows System Locale

Nitpick. Not sure, but should it be written 'Windows system locale'? Lower-case 's' and 'l'? 🤔 Upper-case suggest it is a name... is it?

DSCResources/MSFT_SystemLocale/en-US/MSFT_SystemLocale.strings.psd1, line 5 at r1 (raw file):

 Setting Windows System Locale updated.

Should is say 'Updated Windows System Locale'? Should it also say what it updated it too?

DSCResources/MSFT_SystemLocale/en-US/MSFT_SystemLocale.strings.psd1, line 6 at r1 (raw file):

RestartRequiredMessage

Couldn't this verbose message be part of SystemLocaleUpdatedMessage message? Concatenate those two? 🤔

Tests/Integration/MSFT_SystemLocale.Integration.Tests.ps1, line 48 at r1 (raw file):

Start-DscConfiguration `

Blank line before this one.

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 30 at r1 (raw file):

    $localizedData = InModuleScope $script:DSCResourceName {
        $LocalizedData
    }

Maybe we should use InModuleScope for all tests?

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 33 at r1 (raw file):

Quoted 11 lines of code…

    Describe 'Schema' {
        it 'IsSingleInstance should be mandatory with one value.' {
            $systemLocaleResource = Get-DscResource -Name SystemLocale
            $systemLocaleResource.Properties.Where{
                $_.Name -eq 'IsSingleInstance'
            }.IsMandatory | Should -BeTrue
            $systemLocaleResource.Properties.Where{
                $_.Name -eq 'IsSingleInstance'
            }.Values | Should -Be 'Yes'
        }
    }

Already tested with the test framework?

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 64 at r1 (raw file):

$systemLocale.SystemLocale = $testSystemLocale

Missing Should -Be

codecov-io · 2019-10-29T21:27:06Z

Codecov Report

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

@@         Coverage Diff          @@
##            dev   #288    +/-   ##
====================================
+ Coverage    88%    88%   +<1%     
====================================
  Files        14     15     +1     
  Lines      1480   1520    +40     
====================================
+ Hits       1304   1344    +40     
  Misses      176    176

PlagueHO

Should be good to go now. Thanks @johlju.

Reviewable status: 6 of 11 files reviewed, 7 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_SystemLocale/en-US/MSFT_SystemLocale.strings.psd1, line 3 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Windows System Locale
Nitpick. Not sure, but should it be written 'Windows system locale'? Lower-case 's' and 'l'? 🤔 Upper-case suggest it is a name... is it?

Done.

DSCResources/MSFT_SystemLocale/en-US/MSFT_SystemLocale.strings.psd1, line 5 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

 Setting Windows System Locale updated.
Should is say 'Updated Windows System Locale'? Should it also say what it updated it too?

Done. And also updated it to include the actual system locale set.

DSCResources/MSFT_SystemLocale/en-US/MSFT_SystemLocale.strings.psd1, line 6 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

RestartRequiredMessage
Couldn't this verbose message be part of SystemLocaleUpdatedMessage message? Concatenate those two? 🤔

Done.

Tests/Integration/MSFT_SystemLocale.Integration.Tests.ps1, line 48 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Start-DscConfiguration `
Blank line before this one.

Done.

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 30 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    $localizedData = InModuleScope $script:DSCResourceName {
        $LocalizedData
    }
Maybe we should use InModuleScope for all tests?

Done. There was a brief period where the PowerShell team were trying to get all testing out of the module scope. Ended up just being super painful. This must be a remnant of that.

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 33 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    Describe 'Schema' {
        it 'IsSingleInstance should be mandatory with one value.' {
            $systemLocaleResource = Get-DscResource -Name SystemLocale
            $systemLocaleResource.Properties.Where{
                $_.Name -eq 'IsSingleInstance'
            }.IsMandatory | Should -BeTrue
            $systemLocaleResource.Properties.Where{
                $_.Name -eq 'IsSingleInstance'
            }.Values | Should -Be 'Yes'
        }
    }

Already tested with the test framework?

Yep. Not worth testing like this anyway.

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 64 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$systemLocale.SystemLocale = $testSystemLocale

Missing Should -Be

Done.

PlagueHO · 2019-11-08T07:37:04Z

Something really odd is going on here @johlju, @SSvilen: The Hash table checks are telling me there is hash table style violations, but I can't see any: https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/28707989?fullLog=true

All the tests pass locally. I've not noticed this before. Can either of you see anything wrong with the Hash tables that would cause this check to fail on AppVeyor but pass locally?

I'm also seeing this in NetworkingDsc: https://ci.appveyor.com/project/PlagueHO/networkingdsc?fullLog=true#L1286

Are you seeing this on any of yours @johlju?

SSvilen

The back tick...I've assumed that a hashtable ends with \s*} (as regex).Should we consider the back tick as an edge case?

Reviewable status: 6 of 11 files reviewed, 7 unresolved discussions (waiting on @johlju)

PlagueHO · 2019-11-08T21:51:57Z

@SSvilen, the strange thing is that the hash table tests all pass locally (and in VS Code). @johlju has suggested this might be related to CRLF related, so I'm testing with a .gitattributes. Will get back shortly.

SSvilen · 2019-11-08T22:12:51Z

@PlagueHO Using the preview extensions - nothing detected. Got back to official one - style violation was immediately shown and removing back tick resolves it. Side effect is also that the formatting does not work properly. VS Code insists on formatting the code like that:

    $convertToCimCredential = New-CimInstance `
        -ClassName MSFT_Credential `
        -Property @{
        Username = [System.String] $Credential.UserName
        Password = [System.String] $null
    } `
        -Namespace root/microsoft/windows/desiredstateconfiguration `
        -ClientOnly

Should I do a quick fix about it?
I actually thought, that back tick is considered a bad practice nowadays.

PlagueHO · 2019-11-08T22:37:07Z

@SSvilen - the backtick issue would only explain a few of the violations (not many). Not sure what is causing the other ones. I don't think the back tick is a bad practice, unless it is not required. In this case if we didn't use it, the line would be very long. Admittedly, it would probably be better to create a parameter splat though. @johlju - what do you think? Allow the backtick?

PlagueHO · 2019-11-09T00:10:51Z

@SSvilen - It doesn't look like the .GitAttributes is going to fix the issue either. I suspect there is an issue with the regex and CR vs CRLF that will need to be addressed.

SSvilen

Sorry, I'm talking nonsense... The back tick is not in the extent at all.. The problem is indeed new line - the build agent of appveyor runs Server 2012 R2 and I believe that is may be causing this:

I manually cloned the repo on the appveyor box - still getting only LF.
I'll try to adjust the check today..

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

SSvilen

To make the things funnier, the DSCResource test repo on appveyor box is formatted with CRLF:

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

johlju

Reviewed 4 of 4 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md, line 26 at r6 (raw file):

- RemoveDesktopAdmin:
  - Correct Context messages in integration tests by adding 'When'.

Should this change be here, and not in the unreleased section? Also, maybe the resource name is misspelled? :)

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 32 at r6 (raw file):

-ModuleName 'MSFT_SystemLocale'

Since we are in module scope this parameter is not necessary? 🤔 Throughout if so.

johlju · 2019-11-09T12:50:02Z

@SSvilen would be awesome if you could help resolve that line ending problem, seeing that in other modules as well. Thank you @SSvilen 🙇

SSvilen

The PR is already there. :)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO)

PlagueHO · 2019-11-16T07:27:36Z

@johlju - should be good to go now!

johlju

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md, line 26 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

- RemoveDesktopAdmin:
  - Correct Context messages in integration tests by adding 'When'.
Should this change be here, and not in the unreleased section? Also, maybe the resource name is misspelled? :)

This still looks off... 🤔

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 32 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-ModuleName 'MSFT_SystemLocale'
Since we are in module scope this parameter is not necessary? 🤔 Throughout if so.

This should still not be necessary? 🤔

PlagueHO

Thanks again @johlju - should be good to go!

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @johlju)

CHANGELOG.md, line 26 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This still looks off... 🤔

Good catch - sorry, missed this one. Fixed.

Tests/Unit/MSFT_SystemLocale.Tests.ps1, line 32 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should still not be necessary? 🤔

Missed this. Fixed.

PlagueHO · 2019-11-23T21:49:49Z

Hi @johlju - I've squash rebased this to clean up the commits and resolve conflicts. So hopefully good to go.

johlju

Reviewed 2 of 2 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved

PlagueHO added the needs review The pull request needs a code review. label Oct 29, 2019

johlju reviewed Oct 29, 2019

View reviewed changes

PlagueHO commented Nov 8, 2019

View reviewed changes

SSvilen reviewed Nov 8, 2019

View reviewed changes

SSvilen reviewed Nov 9, 2019

View reviewed changes

johlju reviewed Nov 9, 2019

View reviewed changes

SSvilen reviewed Nov 9, 2019

View reviewed changes

PlagueHO closed this Nov 14, 2019

PlagueHO reopened this Nov 14, 2019

johlju reviewed Nov 16, 2019

View reviewed changes

PlagueHO commented Nov 16, 2019

View reviewed changes

Add SystemLocale Resource

4825448

PlagueHO force-pushed the Issue-50-B branch from 52b50ae to 4825448 Compare November 23, 2019 21:47

johlju reviewed Nov 24, 2019

View reviewed changes

PlagueHO merged commit f709d86 into dsccommunity:dev Nov 25, 2019

PlagueHO deleted the Issue-50-B branch November 25, 2019 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SystemLocale Resource - Fixes #50 #288

Add SystemLocale Resource - Fixes #50 #288

PlagueHO commented Oct 29, 2019 •

edited by joeyaiello

Loading

johlju commented Oct 29, 2019

johlju commented Oct 29, 2019

johlju left a comment

codecov-io commented Oct 29, 2019 •

edited

Loading

PlagueHO left a comment

PlagueHO commented Nov 8, 2019

SSvilen left a comment

PlagueHO commented Nov 8, 2019

SSvilen commented Nov 8, 2019 •

edited

Loading

PlagueHO commented Nov 8, 2019

PlagueHO commented Nov 9, 2019

SSvilen left a comment

SSvilen left a comment

johlju left a comment

johlju commented Nov 9, 2019

SSvilen left a comment

PlagueHO commented Nov 16, 2019

johlju left a comment

PlagueHO left a comment

PlagueHO commented Nov 23, 2019

johlju left a comment

Add SystemLocale Resource - Fixes #50 #288

Add SystemLocale Resource - Fixes #50 #288

Conversation

PlagueHO commented Oct 29, 2019 • edited by joeyaiello Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

johlju commented Oct 29, 2019

johlju commented Oct 29, 2019

johlju left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 29, 2019 • edited Loading

Codecov Report

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO commented Nov 8, 2019

SSvilen left a comment

Choose a reason for hiding this comment

PlagueHO commented Nov 8, 2019

SSvilen commented Nov 8, 2019 • edited Loading

PlagueHO commented Nov 8, 2019

PlagueHO commented Nov 9, 2019

SSvilen left a comment

Choose a reason for hiding this comment

SSvilen left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Nov 9, 2019

SSvilen left a comment

Choose a reason for hiding this comment

PlagueHO commented Nov 16, 2019

johlju left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO commented Nov 23, 2019

johlju left a comment

Choose a reason for hiding this comment

PlagueHO commented Oct 29, 2019 •

edited by joeyaiello

Loading

codecov-io commented Oct 29, 2019 •

edited

Loading

SSvilen commented Nov 8, 2019 •

edited

Loading