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()
        }

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

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

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

Ideally, RequiredVersion could be passed a version or Latest and we'd remove the Latest parameter in it's entirety.

I can make that change, but I am less fond of the overly verbose parameter names.

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

It's a subtle difference from 'SingleInstance'. For example, MinimumVersion could be 9.0.0. In this instance, 8.0.0, 9.0.0, 9.1.0 etc etc could be installed and the resource would be in compliance. With RemoveNonCompliantVersions, 8.0.0 would be removed but all 9+ kept.

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

Not sure what you mean here.

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

I can remove them, they're from https://marketplace.visualstudio.com/items?itemName=aaron-bond.better-comments

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

this catches when minimumversion, requiredversion, maximumversion etc are out of compliance.

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

Done.

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

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

I was looking for a name that more clearly tells the user that this parameter means to opt-in to always install the latest version that exist in the repository every time the configuration is run, not just the latest that existed when the configuration was first run. I felt that the word "Latest" could be mistaken for "the latest version today, and then it never changes". I felt that AutomaticallyUpdateToLatest would clearly says what the user opted-in for, but if that feels too long would the shorter version "AlwaysNewest" or "AlwaysLatest` be better?

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

Sounds good. Now I'm following you, let's keep RemoveNonCompliantVersions.

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

Ah, thanks.

source/Classes/020.PSResource.ps1 line 260 at r15 (raw file):

            $returnValue.Ensure = [Ensure]::Present

            if (-not [System.String]::IsNullOrEmpty($this.VersionRequirement) -and -not $currentState.ContainsKey('Latest'))

Since Latest is muatally exclusive to the other version parameters, can't we just do this to skip this hidden property:

Suggestion:

if (($this | Get-DscProperty -Name @('RequiredVersion','MaximumVersion', 'MinimumVersion') -HasValue))

Reviewed 2 of 3 files at r11, 1 of 3 files at r13.
Reviewable status: 32 of 34 files reviewed, 16 unresolved discussions (waiting on @nickgw)

source/Classes/020.PSResource.ps1 line 547 at r15 (raw file):

        return @{
            Name       = $foundModule.Name
            Version    = $foundModule.Version

This needs to return the prerelease string as well.

Code quote:

        return @{
            Name       = $foundModule.Name
            Version    = $foundModule.Version
            Repository = $foundModule.Repository
        }

source/Classes/020.PSResource.ps1 line 743 at r15 (raw file):

        $return = $null

        switch ($requirement)

We need to be able to handle that both MinimumVersion and MaximumVersion are set at the same time.

Code quote:

 switch ($requirement)

source/Classes/020.PSResource.ps1 line 774 at r15 (raw file):

        $nonCompliantResources = $null

        switch ($this.VersionRequirement)

We need to be able to handle that both MinimumVersion and MaximumVersion are set at the same time.

Code quote:

switch ($this.VersionRequirement)

source/Classes/020.PSResource.ps1 line 824 at r15 (raw file):

        $resourceToKeep = $this.FindResource()

        if ($resourceToKeep.Version -in $resources.Version)

We need to evaluate the prerelease strings here as well if the version is equal. For example if we have '4.5.0-preview1' installed but the desired is '4.5.0-preview2' then the preview1 version need to be uninstalled as well.

Code quote:

if ($resourceToKeep.Version -in $resources.Version)