dsccommunity / ComputerManagementDsc

DSC resources for for configuration of a Windows computer. These DSC resources allow you to perform computer management tasks, such as renaming the computer, joining a domain and scheduling tasks as well as configuring items such as virtual memory, event logs, time zones and power settings.
https://dsccommunity.org
MIT License
303 stars 83 forks source link

PSResourceRepository: Add property `Reasons` #402

Closed johlju closed 1 year ago

johlju commented 1 year ago

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

None.

Task list


This change is Reviewable

johlju commented 1 year ago

@nickgw if you have time, it would be great if you could help to review this one. 🙂

codecov[bot] commented 1 year ago

Codecov Report

Merging #402 (7474163) into main (41ec47e) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #402   +/-   ##
===================================
  Coverage    90%    90%           
===================================
  Files        18     18           
  Lines      1802   1802           
===================================
  Hits       1629   1629           
  Misses      173    173           
Impacted Files Coverage Δ
source/ComputerManagementDsc.psm1 85% <0%> (ø)
johlju commented 1 year ago

So there is an issue using the property Reasons using type [Reason]. Reproduced by @nyanhp, configuration script fail to parse when two modules (psm1 files) contain the class Reason. Same happens for MOF-based resources.

image

@nyanhp created a repository that shows the issue with both class-based resources and MOF-based resources: https://github.com/nyanhp/DscRepro

johlju commented 1 year ago

Closing this PR until there is a solution for this, but most likely not gonna be a fix for WMF5.x.

johlju commented 1 year ago

Seems there is a fix for this, will update this PR.

johlju commented 1 year ago

This PR should be changed according to https://github.com/dsccommunity/DscResource.Base/issues/4. It should also wait for PR #405 is merged so we don't need to do the same modification in this module as being done in DscResource.Base.

johlju commented 1 year ago

@PlagueHO this one is now ready for review. The module DscResource.Base got a fix that was released as v1.1 to support this change.

I will fix the resources over at SqlServerDsc the same way.

johlju commented 1 year ago

@PlagueHO when you approve this, do not merge it until I say OK. I want to make sure all integration tests in SqlServerDsc works so there is nothing else that need to be done. Currently the integration tests using Invoke-DscResource are failing. I will look into it during the week. I report back when this is OK to merge.

johlju commented 1 year ago

@PlagueHO this good to continue reviw and merge if everything looks good. I couldn't find an easy way to return a hashtable for the property Reason to avoid the need for a unique class in each module, but couldn't find a way, so I reverted to using a uniqe class instead (CMReason, and in SqlServerDsc it will be called SqlReason). The important part is that the object that is returned have the correct properties. I also added an integration test to verify the property Reason by using Invoke-DscResource that calls the method Get().