PSResource: Resource to manage PowerShell Resources #400

nickgw · 2022-11-19T02:03:33Z

Pull Request (PR) description

New resource to manage PowerShell Resources

PSResource

This Pull Request (PR) fixes the following issues

Fixes PSResource: New resource proposal #398

Task list

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).
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

codecov · 2022-11-19T02:52:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

@@         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%)`	⬇️

... and 2 files with indirect coverage changes

johlju · 2022-12-21T14:06:27Z

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 · 2022-12-21T14:08:23Z

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

johlju · 2022-12-21T14:59:02Z

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

Reviewable status: 29 of 34 files reviewed, 3 unresolved discussions (waiting on @nickgw)

source/Classes/020.PSResource.ps1 line 283 at r14 (raw file):

        Assert-Module -ModuleName PackageManagement

        $powerShellGet = Get-Module -Name PowerShellGet

Instead of this maybe we can use Import-Module instead? Currently this only works if PowerShellGet is loaded into the session.
We could instead when AllowPrerelease is set do:

PS > Import-Module -Name 'PowerShellGet' -MinimumVersion '99.0.0' -Force
Import-Module: The specified module 'PowerShellGet' with version '99.0.0' was not loaded because no valid module file was found in any module directory.

This throws an exception if the correct module is not available that we can catch in a try-catch-block. Alternative is to use -ListAvailable an enumerate that the correct version is available, but then we must still make sure to import into the session. 🤔

Code quote:

        $powerShellGet = Get-Module -Name PowerShellGet

        if ($powerShellGet.Version -lt [version]'1.6.0' -and $properties.ContainsKey('AllowPrerelease'))
        {
            $errorMessage = $this.localizedData.PowerShellGetVersionTooLowForAllowPrerelease
            New-InvalidArgumentException -ArgumentName 'AllowPrerelease' -message ($errorMessage -f $powerShellGet.Version)
        }

source/Classes/020.PSResource.ps1 line 400 at r14 (raw file):

            Assert() calls this before Get/Set/Test, so this ensures they're always set if necessary.
        #>
        if ($properties.ContainsKey('Latest'))

Seems to me this can be moved to TestLatestVersion() so that we don't need the hidden property at all? If we need to cache the value, then that method can do it?

Code quote:

        if ($properties.ContainsKey('Latest'))
        {
            $this.LatestVersion = $this.GetLatestVersion()
        }

source/Classes/020.PSResource.ps1 line 406 at r14 (raw file):

        if ($properties.ContainsKey('MinimumVersion') -or
            $Properties.ContainsKey('MaximumVersion') -or

Can't we move this to TestVersionRequirement() so that we don't need the hidden property at all? If we need to cache the value, then that method can do it?

Code quote:

        if ($properties.ContainsKey('MinimumVersion') -or
            $Properties.ContainsKey('MaximumVersion') -or
            $Properties.ContainsKey('RequiredVersion') -or
            $Properties.ContainsKey('Latest')
        )
        {
            $this.VersionRequirement = $this.GetVersionRequirement()
        }

johlju

There will be a few iterations of this review, it is a more complex resource. 🙂 Awesome work!

Reviewed all commit messages.
Reviewable status: 29 of 34 files reviewed, 13 unresolved discussions (waiting on @nickgw)

source/Classes/020.PSResource.ps1 line 24 at r14 (raw file):

        Specifies the minimum version of the resource you want to install or uninstall.

    .PARAMETER Latest

Should we call this AutomaticallyUpdateToLatest instead so it is more clear what the user opt-in for?

Code quote:

Latest

source/Classes/020.PSResource.ps1 line 29 at r14 (raw file):

    .PARAMETER Force
        Forces the installation of resource. If a resource of the same name and version already exists on the computer,
        this parameter overwrites the existing resource with one of the same name that was found by the command.

This parameter will not work as Test() will not handle this. If this should be kept, which I don't recommend, then we should always fail Test() when assigned tp $trueso that Set() always run and installs the resource on every run (e.g. every 15 minutes on some systeme). I suggest removing this parameter. If the module is already installed, then the configuration is in desired state.

Code quote:

    .PARAMETER Force
        Forces the installation of resource. If a resource of the same name and version already exists on the computer,
        this parameter overwrites the existing resource with one of the same name that was found by the command.

source/Classes/020.PSResource.ps1 line 38 at r14 (raw file):

        Allows the installation of resource that have not been catalog signed.

    .PARAMETER SingleInstance

I think we shouild rename this to something else, for example "OnlySingleVersion`. The current name can be misintepreted as as single instance DSC resource, see https://learn.microsoft.com/en-us/powershell/dsc/resources/singleinstance?view=dsc-1.1

Code quote:

 SingleInstance

source/Classes/020.PSResource.ps1 line 53 at r14 (raw file):

        Specifies the Credential to connect to the repository proxy.

    .PARAMETER RemoveNonCompliantVersions

This feels to me like the property SingleInstance should already handle., I think we can remove RemoveNonCompliantVersions. If the user specifies SingleInstance then the user already opt-ins to to remove all other versions of that module. Example, if the user specifies that Pester v5.1.0 shoud be the only one, then it should remove all other versions (and throw an error if cannot).

Code quote:

RemoveNonCompliantVersions

source/Classes/020.PSResource.ps1 line 187 at r14 (raw file):

    hidden [void] Modify([System.Collections.Hashtable] $properties)
    {
        $installedResource = $this.GetInstalledResource()

This only needs to run in one or more block that should remove existing modules, so that we don't run it unneccessary.

Code quote:

$installedResource = $this.GetInstalledResource()

source/Classes/020.PSResource.ps1 line 191 at r14 (raw file):

        if ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq 'Absent' -and $this.Ensure -eq 'Absent')
        {
            if ($properties.ContainsKey('RequiredVersion') -and $this.RequiredVersion)

Looking at this, and since the Ensure property is connected to the Key properties, I think we don't need the below code. If a user says thet the key property Name should be absent, then we should remove every instance of the modules with that name. So I think we just need the foreach-loop here. Also, there is not method called UninstallModule().

Code quote:

            if ($properties.ContainsKey('RequiredVersion') -and $this.RequiredVersion)
            {
                $resourceToUninstall = $installedResource | Where-Object {$_.Version -eq [Version]$this.RequiredVersion}
                $this.UninstallModule($resourceToUninstall)
            }

source/Classes/020.PSResource.ps1 line 203 at r14 (raw file):

        elseif ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq 'Present' -and $this.Ensure -eq 'Present')
        {
            #* Module does not exist at all

Can we remove the asterisk here in the comment, or does it mean something?

Code quote:

#* Module does not exist at all

source/Classes/020.PSResource.ps1 line 210 at r14 (raw file):

            $this.ResolveSingleInstance($installedResource)

            return

Should we keep it consistant and skip the return-statement here an below since they are non oin the other blocks?

Code quote:

return

source/Classes/020.PSResource.ps1 line 223 at r14 (raw file):

        }
        else
        {

When is this needed, wouldn't it have installed already on line 201-205?

Code quote:

        else
        {
            $this.InstallResource()
        }

source/Classes/020.PSResource.ps1 line 565 at r14 (raw file):

    hidden [void] UninstallResource ([System.Collections.Hashtable]$resource)
    {
        $params = $this | Get-DscProperty -ExcludeName @('Latest','SingleInstance','Ensure', 'SkipPublisherCheck', 'RemoveNonCompliantVersions','MinimumVersion', 'MaximumVersion', 'RequiredVersion') -Type Optional -HasValue

This method should uninstall a specific version of a module, so not sure we need to this? 🤔 I think we just need to run:

Write-Verbose -Message ($this.localizedData.UninstallResource -f $resource.Name,$resource.Version)

Uninstall-Module -Name $resource.Name -RequiredVersion $resource.Version -Force -AllowPrerelease

Using AllowPrerelease if the module happen to be a prerelease, if it is a full release then the parameter AllowPrerelease? is "ignored".

Suggestion:

        Write-Verbose -Message ($this.localizedData.UninstallResource -f $resource.Name,$resource.Version)

        Uninstall-Module -Name $resource.Name -RequiredVersion $resource.Version -Force -AllowPrerelease

johlju · 2022-12-23T15:04:27Z

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

nickgw

Reviewable status: 29 of 34 files reviewed, 13 unresolved discussions (waiting on @johlju)

source/Classes/020.PSResource.ps1 line 24 at r14 (raw file):