ComputerManagementDsc icon indicating copy to clipboard operation
ComputerManagementDsc copied to clipboard

PSResource: Resource to manage PowerShell Resources

Open nickgw opened this issue 3 years ago • 10 comments

Pull Request (PR) description

New resource to manage PowerShell Resources

  • PSResource

This Pull Request (PR) fixes the following issues

  • Fixes #398

Task list

  • [x] Added an entry to the change log under the Unreleased section of the file CHANGELOG.md. Entry should say what was changed and how that affects users (if applicable), and reference the issue being resolved (if applicable).
  • [x] Resource documentation added/updated in README.md.
  • [ ] Resource parameter descriptions added/updated in README.md, 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 Community Testing Guidelines.
  • [ ] Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • [ ] New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

nickgw avatar Nov 19 '22 02:11 nickgw

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (196d491) 90% compared to head (79d051c) 86%. Report is 1 commits behind head on main.

:exclamation: Current head 79d051c differs from pull request most recent head a87d39d. Consider uploading reports for the commit a87d39d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #400    +/-   ##
====================================
- Coverage    90%    86%    -5%     
====================================
  Files        18     18            
  Lines      1801   1979   +178     
====================================
+ Hits       1631   1708    +77     
- Misses      170    271   +101     
Files Coverage Δ
source/ComputerManagementDsc.psm1 58% <ø> (-27%) :arrow_down:

... and 2 files with indirect coverage changes

codecov[bot] avatar Nov 19 '22 02:11 codecov[bot]

The other PR is merged, you can rebase this against main branch now.

johlju avatar Dec 11 '22 18:12 johlju

@nickgw let me know when this one is ready for review.

johlju avatar Dec 12 '22 11:12 johlju

@johlju , I think this is ready for a review when you get a chance. I've written unit tests for everything except GetCurrentStatus(), Modify() , Set() and Get() at this point.

I think my two outstanding questions are:

  • Is shimming setting LatestVersion and VersionRequirement readonly properties into AssertProperties() the right move? AssertProperties() is the only method in PSResource that always gets called which is why I put it there.

  • How should I handle AllowPrerelease? I kind of want to gate it to only allow RequiredVersion, Latest or no versionrequirement at all. Trying to massage pre-releases in with MinimumVersion & MaximumVersion kind of makes my head hurt but I haven't thought about it too much.

nickgw avatar Dec 20 '22 15:12 nickgw

As an fun exercise I created a class that might remove some of the logic from the DSC resource and could be re-used in other ways (in the future). I just did a PoC of my thoughts. Let me if you think it could help anything. Please re-use any part you like, if it helps.

See the code in the gist here: https://gist.github.com/johlju/5f1ed2fe2f510c9e4272dd942a389723

classDiagram

class PSResourceObject {
  +string Name
  +version Version
  +string PreRelease
  +PSResourceObject()
  +PSResourceObject(string Name)
  +PSResourceObject(string Name, version Version)
  +PSResourceObject(string Name, version Version, string PreRelease)
  +CompareTo(object) int32
  +Equals(object) boolean
  +GetInstalledResource(string)$ PSResourceObject[]
  +GetMinimumInstalledVersion(string)$ PSResourceObject
  +GetMinimumInstalledVersion(PSResourceObject[])$ PSResourceObject
  +GetMaximumInstalledVersion(string)$ PSResourceObject
  +GetMaximumInstalledVersion(PSResourceObject[])$ PSResourceObject
  +IsPreRelease() Boolean
}

class IComparable {
  <<Interface>>
  CompareTo(object) int32
}

class IEquatable {
  <<Interface>>
  Equals(object) boolean
}

IComparable <|-- PSResourceObject : implements
IEquatable <|-- PSResourceObject : implements

johlju avatar Dec 21 '22 14:12 johlju

I gonna be away tomorrow, but get back to a review of this on friday. 🙂

johlju avatar Dec 21 '22 14:12 johlju

How should I handle AllowPrerelease? I kind of want to gate it to only allow RequiredVersion, Latest or no versionrequirement at all. Trying to massage pre-releases in with MinimumVersion & MaximumVersion kind of makes my head hurt but I haven't thought about it too much.

Would the class I put together help with this? It could compare two objects. We could something like below (assuming the class is renamed to PSResourceObject in lack of a better name).

Assuming the current state contain version 1.0.0 and the minimum required version is 1.0.1-preview1.

$currentStateResource =  [PSResourceObject] @{
    Name = 'MyModule'
    Version = '1.0.0'
}

$minimumRequiredResource =  [PSResourceObject] @{
    Name = 'MyModule'
    Version = '1.0.1'
    PreRelease = 'preview1'
}

$minimumRequiredResource.CompareTo($currentStateResource)

That would result in 1 meaning the minumim version required is not installed.

  • 1 = minimum version need to be installed
  • 0 = minimum version is compliant, the exact minimum version is installed
  • -1 = minimum version is compliant, there is already a higher version installed than the minimum version

Added example output to the gist's README.md (link above).

johlju avatar Dec 21 '22 14:12 johlju

I will continue the review once these are done (or when I have time). I only got half-way through Modify() (and connected methods). 🙂

johlju avatar Dec 23 '22 15:12 johlju