Merged xTimeZone and converted to HQRM - Fixes #157

[PlagueHO]

Pull Request (PR) description
This PR moves the xTimeZone resource into this repo and renames it to TimeZone.

This Pull Request (PR) fixes the following issues:

• Fixes Move xTimeZone resource into ComputerManagementDsc #157

Task list:

• 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?

@johlju - would you mind reviewing this if you get a moment? It is mostly the original xTimeZone, but updated to HQRM.

---

This change is 

[codecov-io]

Codecov Report

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

@@         Coverage Diff         @@
##           dev   #158    +/-   ##
===================================
+ Coverage   89%    89%   +<1%     
===================================
  Files        5      6     +1     
  Lines      647    668    +21     
===================================
+ Hits       579    600    +21     
  Misses      68     68

[johlju]

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

---

README.md, line 20 at r1 (raw file):

It replaces the older standalone `xTimeZone` resource

Maybe remove this so we don't reference 'x' resources. It says that in the changelog, should be enough with that plus a deprecated message in xTimeZone?

---

Modules/ComputerManagementDsc/DSCResources/MSFT_TimeZone/MSFT_TimeZone.psm1, line 20 at r1 (raw file):

<#
    .SYNOPSIS
    Returns the current time zone of the node.

Should be indented one more step. Throughout.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_TimeZone/MSFT_TimeZone.psm1, line 95 at r1 (raw file):

    {
        Write-Verbose -Message ($LocalizedData.TimeZoneAlreadySetMessage `
                -f $TimeZone)

Non-blocking: Does auto-formatting do this indentation? If so, good as-is. It just looks wrong indented with two tabs, but just my personal opinion, so again, non-blocking. 😃

---

Modules/ComputerManagementDsc/DSCResources/MSFT_TimeZone/en-us/MSFT_TimeZone.strings.psd1, line 4 at r1 (raw file):

ConvertFrom-StringData -StringData @'
    GettingTimeZoneMessage       = Getting the time zone.
    ReplaceSystemTimeZoneMessage = Replace the System time zone.

Don't think this used? 🤔

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 345 at r1 (raw file):

        Get the of the current time zone Id.
#>
function Get-TimeZoneId

Shouldn't we keep this as a helper function in the resource .psm1 file instead if that resource is the only one using this?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 382 at r1 (raw file):

        The Id of the time zone to compare with the current time zone.
#>
function Test-TimeZoneId

See previous comment.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 405 at r1 (raw file):

        The Id of the time zone to set.
#>
function Set-TimeZoneId

See previous comment

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 455 at r1 (raw file):

        The Id of the time zone to set using .NET.
#>
function Set-TimeZoneUsingDotNet

See previous comment

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r1 (raw file):

using System;

Maybe we could add a comment what this file and namespace does?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 212 at r1 (raw file):

            var regTimeZones = Registry.LocalMachine.OpenSubKey("SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\Time Zones");
            // Print out all the possible time-zones.
            //foreach(var subKey in regTimeZones.GetSubKeyNames())

Maybe we could add a debug parameter that is default false? Or, remove this commented code?

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/en-us/ComputerManagementDsc.Common.strings.psd1, line 15 at r1 (raw file):

    MatchElementValueMessage           = MATCH: Value [{0}] (type '{1}') for property '{2}' does match. Current state is '{3}' and desired state is '{4}'.
    TestDscParameterResultMessage      = Test-DscParameter result is '{0}'.
    CurrentTimeZoneMessage             = Current time zone is set to '{0}'

If we move the functions (see previous comments) these should move too.

---

Comments from Reviewable

[PlagueHO]

Review status: 14 of 18 files reviewed at latest revision, 10 unresolved discussions.

---

README.md, line 20 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It replaces the older standalone `xTimeZone` resource

Maybe remove this so we don't reference 'x' resources. It says that in the changelog, should be enough with that plus a deprecated message in xTimeZone?

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_TimeZone/MSFT_TimeZone.psm1, line 20 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should be indented one more step. Throughout.

Done.

---

Modules/ComputerManagementDsc/DSCResources/MSFT_TimeZone/MSFT_TimeZone.psm1, line 95 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Non-blocking: Does auto-formatting do this indentation? If so, good as-is. It just looks wrong indented with two tabs, but just my personal opinion, so again, non-blocking. 😃

It doesn't correct it - but I've put it on the same line now anyway. So fixed hopefully 😁

---

Modules/ComputerManagementDsc/DSCResources/MSFT_TimeZone/en-us/MSFT_TimeZone.strings.psd1, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Don't think this used? 🤔

Wow - great catch! Fixed.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 345 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Shouldn't we keep this as a helper function in the resource .psm1 file instead if that resource is the only one using this?

It is actually used by some of the integration tests for ScheduledTasks. I could move these and keep two copies, but it felt like duplication.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 382 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

See previous comment.

It is actually used by some of the integration tests for ScheduledTasks. I could move these and keep two copies, but it felt like duplication.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 405 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

See previous comment

It is actually used by some of the integration tests for ScheduledTasks. I could move these and keep two copies, but it felt like duplication.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 455 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

See previous comment

It is actually used by some of the integration tests for ScheduledTasks. I could move these and keep two copies, but it felt like duplication.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe we could add a comment what this file and namespace does?

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 212 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe we could add a debug parameter that is default false? Or, remove this commented code?

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/en-us/ComputerManagementDsc.Common.strings.psd1, line 15 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If we move the functions (see previous comments) these should move too.

See previous - happy to move these.

---

Comments from Reviewable

[johlju]

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

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 345 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

It is actually used by some of the integration tests for ScheduledTasks. I could move these and keep two copies, but it felt like duplication.

No, we shouldn't duplicate code. Could we add a .NOTES section explaining that these are also used by the integration tests? Same for the three other functions.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r2 (raw file):

// This file enables setting of the machine time zone

Could we make this code run in the integration test also by mocking "away" the cmdlets? It is tested in the unit tests but would be great if we could run this in the integration tests too?

---

Comments from Reviewable

[johlju]

@PlagueHO my last comment, I meant to say that the cmdlets is mocked "away" in the unit test, but the C# code is never added I think, since that helper function is mock too. So if we could actually run the C# code in the integration test that would be great I think.

[PlagueHO]

Review status: 17 of 18 files reviewed at latest revision, 3 unresolved discussions.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/ComputerManagementDsc.Common.psm1, line 345 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

No, we shouldn't duplicate code. Could we add a .NOTES section explaining that these are also used by the integration tests? Same for the three other functions.

Done.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make this code run in the integration test also by mocking "away" the cmdlets? It is tested in the unit tests but would be great if we could run this in the integration tests too?

I'm not sure this is possible (to Mock within the integration tests) because the LCM runs under the context of a WMI process - not the process that invokes it. So the ComputerManagementDsc.Common module (that contains Test-Command) isn't loaded to even mock (it is loaded into the WMI process context). But to make double sure I did actually give it a try 😢 Would have been great if it could have worked.

---

Comments from Reviewable

[johlju]

Review status: 17 of 18 files reviewed at latest revision, 3 unresolved discussions.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I'm not sure this is possible (to Mock within the integration tests) because the LCM runs under the context of a WMI process - not the process that invokes it. So the ComputerManagementDsc.Common module (that contains Test-Command) isn't loaded to even mock (it is loaded into the WMI process context). But to make double sure I did actually give it a try 😢 Would have been great if it could have worked.

Yes that won't work. My thought was not run it in LCM but run the helper commands directly in a (separate) integration tests after the LCM tests has run. Would that work?

---

Comments from Reviewable

[PlagueHO]

Review status: 17 of 18 files reviewed at latest revision, 1 unresolved discussion.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Yes that won't work. My thought was not run it in LCM but run the helper commands directly in a (separate) integration tests after the LCM tests has run. Would that work?

Oh I see what you mean - yes, I think that would work. I'll see what I can do.

---

Comments from Reviewable

[PlagueHO]

Review status: 15 of 19 files reviewed at latest revision, 1 unresolved discussion.

---

Modules/ComputerManagementDsc/Modules/ComputerManagementDsc.Common/SetTimeZone.cs, line 1 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Oh I see what you mean - yes, I think that would work. I'll see what I can do.

Done.

---

Comments from Reviewable

[PlagueHO]

Should be ready for review now- thanks @johlju

---

Review status: 15 of 19 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[johlju]

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

---

Tests/Integration/ComputerManagementDsc.Common.Tests.ps1, line 61 at r4 (raw file):

                }

                Mock -CommandName Add-Type

We are mocking Add-Type here so it will never run the .cs code? And the next row we are mocking Set-TimeZoneUsingDotNet that actually loads the C# code? :)
Maybe just mock Test-Command to return $true for 'Add-Type'? 🤔

---

Comments from Reviewable

[PlagueHO]

Review status: 17 of 19 files reviewed at latest revision, 1 unresolved discussion.

---

Tests/Integration/ComputerManagementDsc.Common.Tests.ps1, line 61 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We are mocking Add-Type here so it will never run the .cs code? And the next row we are mocking Set-TimeZoneUsingDotNet that actually loads the C# code? :)
Maybe just mock Test-Command to return $true for 'Add-Type'? 🤔

Wow - that was a great catch and identified a problem with the C# debug code - which I've now removed entirely. It just resulted in warnings and other stuff anyway due to "Unreachable code".

---

Comments from Reviewable

[johlju]

Awesome work here, as usually! 😄

---

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[PlagueHO]

Thank you so much @johlju! I'll get to work on xPendingReboot next weekend (the rest of the week I'm presenting 😢)

---

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

---

Comments from Reviewable