Get-SqlDscPreferredModule: Optionally specify which version of the SQL module is imported

[YaroBear]

Pull Request (PR) description

Optionally set the SMODefaultModuleVersion environment variable for your preferred module.

For example:

• SqlServer: 22.0.49-preview
• SQLPS: 130
• OtherModule: <maj.min.patch>

This Pull Request (PR) fixes the following issues

• Fixes Get-SqlDscPreferredModule: Should be possible to specify which version of the SqlServer module is imported #1965

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 updated in the resource's README.md.
• Resource parameter descriptions updated in schema.mof.
• Comment-based help updated, including parameter descriptions.
• Localization strings updated.
• Examples updated.
• Unit tests updated. See DSC Community Testing Guidelines.
• Integration tests updated (where possible). See DSC Community Testing Guidelines.
• Code changes adheres to DSC Community Style Guidelines.

---

This change is 

[codecov]

Codecov Report

Merging #1966 (9c6420c) into main (472ef04) will decrease coverage by 1%.
The diff coverage is 97%.

@@         Coverage Diff          @@
##           main   #1966   +/-   ##
====================================
- Coverage    92%     92%   -1%     
====================================
  Files        92      93    +1     
  Lines      7829    7832    +3     
====================================
+ Hits       7204    7206    +2     
- Misses      625     626    +1     

Flag	Coverage Δ
unit	92% <97%> (-1%)	⬇️

Files Changed	Coverage Δ
source/Public/Import-SqlDscPreferredModule.ps1	96% <95%> (-4%)	⬇️
source/Private/Get-SMOModuleCalculatedVersion.ps1	100% <100%> (ø)
source/Public/Get-SqlDscPreferredModule.ps1	100% <100%> (ø)

[johlju]

Awesome work on this, and impressive work on the unit tests! Just minor comments.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @YaroBear)

---

source/Public/Get-SqlDscPreferredModule.ps1 line 96 at r1 (raw file):

    $availableModules = Get-Module -Name $Name -ListAvailable |
        ForEach-Object {

We should use named parameters.

Suggestion:

ForEach-Object -Process {

---

source/Public/Get-SqlDscPreferredModule.ps1 line 126 at r1 (raw file):

    {
        $preferredModules = $availableModules |
            Where-Object { $_.PSModuleInfo.Name -eq $preferredModuleName}

We should use named parameters.

Suggestion:

Where-Object -FilterScript { $_.PSModuleInfo.Name -eq $preferredModuleName}

---

source/Public/Get-SqlDscPreferredModule.ps1 line 126 at r1 (raw file):

    {
        $preferredModules = $availableModules |
            Where-Object { $_.PSModuleInfo.Name -eq $preferredModuleName}

Couldn't we filter the preferred module from the result from Get-Module above directly, before calculating version? It would remove this foreach-loop. 🤔 What do you think`

---

source/Public/Get-SqlDscPreferredModule.ps1 line 134 at r1 (raw file):

                # Get the version specified in $env:SMODefaultModuleVersion if available
                $availableModule = $preferredModules |
                Where-Object { $_.CalculatedVersion -eq $env:SMODefaultModuleVersion} |

We should use named parameters.

Suggestion:

Where-Object -FIlterScript { $_.CalculatedVersion -eq $env:SMODefaultModuleVersion} |

---

source/Public/Get-SqlDscPreferredModule.ps1 line 135 at r1 (raw file):

                $availableModule = $preferredModules |
                Where-Object { $_.CalculatedVersion -eq $env:SMODefaultModuleVersion} |
                Select-Object -First 1

We should indent these two lines of the pipeline one indent when they are on separate rows (which is good to not make the line too long).

Code quote:

                Where-Object { $_.CalculatedVersion -eq $env:SMODefaultModuleVersion} |
                Select-Object -First 1

---

source/Public/Get-SqlDscPreferredModule.ps1 line 139 at r1 (raw file):

                Write-Verbose -Message ($script:localizedData.PreferredModule_ModuleVersionFound -f $availableModule.PSModuleInfo.Name, $availableModule.CalculatedVersion)
            }
            else {

We should have open brace on a separate line.

Code quote:

else {

---

source/Public/Get-SqlDscPreferredModule.ps1 line 143 at r1 (raw file):

                $availableModule = $preferredModules |
                Sort-Object -Property 'CalculatedVersion' -Descending |
                Select-Object -First 1

We should indent these two lines of the pipeline one indent when they are on separate rows (which is good to not make the line too long).

Code quote:

                Sort-Object -Property 'CalculatedVersion' -Descending |
                Select-Object -First 1

---

source/Public/Get-SqlDscPreferredModule.ps1 line 145 at r1 (raw file):

                Select-Object -First 1

                Write-Verbose -Message ($script:localizedData.PreferredModule_ModuleFound -f $availableModule.PSModuleInfo.Name)

Maybe we could use the string PreferredModule_ModuleVersionFound here too, to output the version even if the user did not specify a specific version. It would simplify the code with just one Write-Verbose before the break and the output is more detailed. 🤔 What do you think?

Code quote:

PreferredModule_ModuleFound

---

source/Public/Import-SqlDscPreferredModule.ps1 line 83 at r1 (raw file):

        if ($PSBoundParameters.ContainsKey('Name'))
        {
            $removeModule += Get-Module $Name

We should use named parameters.

Suggestion:

Get-Module -Name $Name

---

source/Public/Import-SqlDscPreferredModule.ps1 line 97 at r1 (raw file):

        }

        Remove-Module $removeModule -Force -ErrorAction 'SilentlyContinue'

We should use named parameters.

Suggestion:

Remove-Module -Name $removeModule -Force -ErrorAction 'SilentlyContinue'

---

source/Public/Import-SqlDscPreferredModule.ps1 line 114 at r1 (raw file):

                return
            }

What if the wrong version is already imported, what shall happen then? I think we need to throw an exception because then the wrong SMO assemblies has been loaded into the session and that can mess things up if we would just try to import and newer or older version.

Code quote:

            <#
                Check if the preferred module is already loaded into the session.
            #>
            $loadedModuleName = (Get-Module -Name $availableModule.Name | Select-Object -First 1).Name

            if ($loadedModuleName)
            {
                Write-Verbose -Message ($script:localizedData.PreferredModule_AlreadyImported -f $loadedModuleName)

                return
            }

[johlju]

I merged another PR so you will need to rebase this one.

[johlju]

Also, check the HQRM tests failing (style errors): https://dev.azure.com/dsccommunity/SqlServerDsc/_build/results?buildId=8129&view=logs&j=9150365c-bae3-55fa-031b-59bba4aba4d9&t=dc12ef69-6f79-52f3-2e04-c23de96e6308&l=565

[YaroBear]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @johlju)

---

source/Public/Get-SqlDscPreferredModule.ps1 line 126 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Couldn't we filter the preferred module from the result from Get-Module above directly, before calculating version? It would remove this foreach-loop. 🤔 What do you think`

We are already filtering by $name on line 95: $availableModules = Get-Module -Name $Name

The foreach-loop serves to find the preferred module in the order defined by the $Name variable.
For example: if $Name = @('sqlserver', 'sqlps'), in the foreach-loop, if a sqlserver module is found, it will set $availableModule = SqlServer and break from the loop.
If sqlserver module is not found, then it will look for sqlps.

---

source/Public/Import-SqlDscPreferredModule.ps1 line 114 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

What if the wrong version is already imported, what shall happen then? I think we need to throw an exception because then the wrong SMO assemblies has been loaded into the session and that can mess things up if we would just try to import and newer or older version.

Hm, Get-SqlDscPreferredModule probably needs to return not just the PSModuleInfo object, but also the CalculatedVersion property in order to check this accurately for SQLPS (without duplicating logic)

Do you recommend a statement terminating error, i.e $PSCmdlet.ThrowTerminatingError?

[YaroBear]

Reviewable.io seems to be down currently...

Hm, Get-SqlDscPreferredModule probably needs to return not just the PSModuleInfo object, but also the CalculatedVersion property in order to check this accurately for SQLPS (without duplicating logic)

Actually, I think I'll need to reuse the logic either way. Maybe a private function would be better? Get-SMOModuleCalculatedVersion can be reused in both Get-SqlDscPreferredModule and Import-SqlDscPreferredModule

[YaroBear]

Reviewable status: 1 of 8 files reviewed, 11 unresolved discussions (waiting on @johlju)

---

source/Public/Get-SqlDscPreferredModule.ps1 line 96 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use named parameters.

Done.

---

source/Public/Get-SqlDscPreferredModule.ps1 line 126 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use named parameters.

Done.

---

source/Public/Get-SqlDscPreferredModule.ps1 line 134 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use named parameters.

Done.

---

source/Public/Get-SqlDscPreferredModule.ps1 line 135 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should indent these two lines of the pipeline one indent when they are on separate rows (which is good to not make the line too long).

Done.

---

source/Public/Get-SqlDscPreferredModule.ps1 line 139 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have open brace on a separate line.

Done.

---

source/Public/Get-SqlDscPreferredModule.ps1 line 143 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should indent these two lines of the pipeline one indent when they are on separate rows (which is good to not make the line too long).

Done.

---

source/Public/Get-SqlDscPreferredModule.ps1 line 145 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe we could use the string PreferredModule_ModuleVersionFound here too, to output the version even if the user did not specify a specific version. It would simplify the code with just one Write-Verbose before the break and the output is more detailed. 🤔 What do you think?

Done.

---

source/Public/Import-SqlDscPreferredModule.ps1 line 83 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use named parameters.

Done.

---

source/Public/Import-SqlDscPreferredModule.ps1 line 97 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use named parameters.

Done.

[YaroBear]

@johlju

I'm running this and don't seem to be getting any ScriptAnalyzer issues locally. VS Code doesn't seem to be picking up on anything either. Any advice?
Invoke-ScriptAnalyzer -Path .\source\DSCResources\**\*.psm1 -CustomRulePath .\tests\QA\AnalyzerRules\SqlServerDsc.AnalyzerRules.psm1 -IncludeRule @('Measure-*')

Run tests\QA\ScriptAnalyzer.Tests.ps1 to run all ScriptAnalyzer tests

Also, getting some test failures in the pipeline for the new private function I added. What am I missing for the function to be picked up?
Get-SMOModuleCalculatedVersion' is not recognized as a name of a cmdlet

Need to wrap tests for unexported/private functions with InModuleScope -ScriptBlock

[johlju]

I will continue review tomorrow I hope. Looks like you answered all questions yourself? 🙂

[YaroBear]

I will continue review tomorrow I hope. Looks like you answered all questions yourself? 🙂

Yep, I think so! Thanks!

[johlju]

Great thought moving the calculation of module version to a private function! Just a few more comments.

Reviewed 4 of 7 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @YaroBear)

---

source/Public/Import-SqlDscPreferredModule.ps1 line 114 at r1 (raw file):

Previously, YaroBear (Yaroslav Berejnoi) wrote…

Hm, Get-SqlDscPreferredModule probably needs to return not just the PSModuleInfo object, but also the CalculatedVersion property in order to check this accurately for SQLPS (without duplicating logic)

Do you recommend a statement terminating error, i.e $PSCmdlet.ThrowTerminatingError?

My bad, it seems there was a bug prior to this change that should have thrown, see previous comment.

---

source/Public/Import-SqlDscPreferredModule.ps1 line 73 at r3 (raw file):

    }

    $availableModule = Get-SqlDscPreferredModule @getSqlDscPreferredModuleParameters

This should be called with -ErrorAction 'Stop' so that it throws if the wrong version is found, or if no module is found at all. This seems like this was a bug prior to this change.

---

source/Public/Import-SqlDscPreferredModule.ps1 line 123 at r3 (raw file):

                        )
                    )
                }

No sure we need this error since Get-SqlDscPreferredModule should throw an exception prior to this (see previous comment). 🤔 Do you agree?

Code quote:

                $loadedModuleCalculatedVersion = $loadedModule | Get-SMOModuleCalculatedVersion
                $availableModuleCalculatedVersion = $availableModule | Get-SMOModuleCalculatedVersion
                if ($env:SMODefaultModuleVersion -and $loadedModuleCalculatedVersion -ne $availableModuleCalculatedVersion)
                {
                    $PSCmdlet.ThrowTerminatingError(
                        [System.Management.Automation.ErrorRecord]::new(
                            ($script:localizedData.PreferredModule_WrongModuleVersionLoaded -f $loadedModule.Name, $loadedModule.Version, $availableModule.Version),
                            'ISDPM0001', # cspell: disable-line
                            [System.Management.Automation.ErrorCategory]::ObjectNotFound,
                            'PreferredModule'
                        )
                    )
                }

[YaroBear]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

---

source/Public/Import-SqlDscPreferredModule.ps1 line 114 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

My bad, it seems there was a bug prior to this change that should have thrown, see previous comment.

Got it! Thanks. Will make the update

---

source/Public/Import-SqlDscPreferredModule.ps1 line 73 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should be called with -ErrorAction 'Stop' so that it throws if the wrong version is found, or if no module is found at all. This seems like this was a bug prior to this change.

ah right, that will make Write-Error in Get-SqlDscPreferredModule a terminating error. Got it!

---

source/Public/Import-SqlDscPreferredModule.ps1 line 123 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

No sure we need this error since Get-SqlDscPreferredModule should throw an exception prior to this (see previous comment). 🤔 Do you agree?

Agreed!

[YaroBear]

Thanks!

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

---

source/Public/Import-SqlDscPreferredModule.ps1 line 73 at r3 (raw file):

Previously, YaroBear (Yaroslav Berejnoi) wrote…

ah right, that will make Write-Error in Get-SqlDscPreferredModule a terminating error. Got it!

Just checking, instead of the if/else we should now wrap Get-SqlDscPreferredModule in a try/catch with the $PSCmdlet.ThrowTerminating error in the catch body?

[johlju]

source/Public/Import-SqlDscPreferredModule.ps1 line 73 at r3 (raw file):

Previously, YaroBear (Yaroslav Berejnoi) wrote…

Just checking, instead of the if/else we should now wrap Get-SqlDscPreferredModule in a try/catch with the $PSCmdlet.ThrowTerminating error in the catch body?

There are already and error thrown for us in the Get-command (the command Write-Error) so by adding ErrorAction to our call here we make sure that when the Write-Error is executed we get a terminating error.

So no need for a try/catch here and no need for adding any kind of extra throw in the import-command . The logic for checking for the wrong version is in the get function and it is that function that throws an error.

A user can igonore the error in the Get-command by adding ErrorAction ‘SilentlyContinue’ but we want a terminating error so we call it with ‘Stop’.

Hope that is more clear what I meant. 😊

[YaroBear]

@johlju Were you able to check my last commit?
There was an if($availableModule) else $PSCmdlet.ThrowTerminatingError at the bottom of the script prior to my changes. Here is the main branch currently

Now that the Get command has an erroraction 'stop', I replaced the if/else with try/catch in the Import command. Otherwise we will get a script terminating error instead of a statement terminating error.

Are we getting rid of $PSCmdlet.ThrowTerminatingError in the Import command entirely now so that it's a script terminating error? Sorry for confusion, just double checking 😅

[johlju]

No worries, glad you discussion and double checking so we get it correct. After testing running the code I do like your change so let's keep it! 🙂

But not sure what you mean by script terminating error? Can you explain what you meant by "script terminating error" so I understand what you meant for the future? 🙂

If we call Get-SqlDscPreferredModule we get this exception:

PS> Get-SqlDscPreferredModule -ErrorAction stop
Get-SqlDscPreferredModule : No preferred PowerShell module was found.
At line:1 char:1
+ Get-SqlDscPreferredModule -ErrorAction stop
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (SqlServer, SQLPS:String) [Write-Error], WriteErrorException
+ FullyQualifiedErrorId : GSDPM0001,Get-SqlDscPreferredModule

Running Import-SqlDscPreferredModule we get this exception (with your change):

PS> Import-SqlDscPreferredModule
Import-SqlDscPreferredModule : Failed to find a dependent module. Unable to run SQL Server commands or use SQL Server types. Please install one of the preferred SMO modules or the SQLPS module, then try to import SqlServerDsc again.
At line:1 char:1
+ Import-SqlDscPreferredModule
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (PreferredModule:String) [Import-SqlDscPreferredModule], Exception
+ FullyQualifiedErrorId : ISDPM0001,Import-SqlDscPreferredModule

The only difference (except for the error message and command name) is the category information were it says Write-Error in the first one.

[johlju]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @YaroBear)

[johlju]

Awesome work on this @YaroBear. I kicked off the failed tests again (they fail because the free Microsoft-hosted runners are to slow at times). Will merge once they passes.

[YaroBear]

Awesome, thanks for working with me and having the great discussions! I wasn't aware of the difference between throw (pipeline terminating error) and $pscmdlet.ThrowTerminatingError (statement terminating error) before this PR actually.

I got the information from this stackoverflow. So it might not be accurate as it's not in the official PowerShell docs:

a script-terminating error terminates the entire runspace by default. i.e., the running script and all its callers, with no further statements getting executed

whereas a statement-terminating error terminates only the current statement (the function calling $PScmdlet.ThrowTerminatingError() and the statement it is a part of, which is often a pipeline), with execution continuing with the next statement by default.

[johlju]

I learned something new today too. But that article does not mention Write-Error which does non-terminating errors, so it too should allow a pipeline to continue running unless ErrorAction is set to Stop. 🤔 The statment throw should not be used, but mostly because it outputs the exception badly visually. It is mentioned here https://github.com/dsccommunity/SqlServerDsc/blob/main/CONTRIBUTING.md#commands but that text might take in account how to throw when a command supports a pipelines. I always assumed one should use Write-Error in those cases. 🤔

[YaroBear]

This portion of CONTRIBUTING.md specifies:

If a command shall throw an terminating error then the statement throw shall not be used, neither shall the command Write-Error with the parameter -ErrorAction Stop. Always use the method $PSCmdlet.ThrowTerminatingError() to throw a terminating error.

Although we are not setting Write-Error "" -ErrorAction 'Stop' directly in Get-SqlDscPreferredModule, we are setting it indirectly by writing Get-SqlDscPreferredModule -ErrorAction 'Stop'. See the example code at the bottom of this comment.

So I think we have to have the try/catch block on Get-SqlDscPreferredModule -ErrorAction 'Stop' otherwise the entire DSC script will terminate.

Example code:

• Calling Invoke-WriteErrowWithErrorActionStop, we see that Write-Host within the function is not executed after we encounter the Write-Error
• Calling Invoke-ThrowTerminatingErrorAndLog, we see that Write-Host within the function is executed after we encounter $PScmdlet.ThrowTerminatingError

function Invoke-WriteError {
    [CmdletBinding()]
    param (
    )

    Write-Error 'An error occured'
}

function Invoke-ThrowTerminatingError() {
    [CmdletBinding()]
    param (
    )
    $PScmdlet.ThrowTerminatingError(
        [System.Management.Automation.ErrorRecord]::new(
        ('Command terminating error'),
        'ISDPM0001',
        [System.Management.Automation.ErrorCategory]::InvalidOperation,
        'CommandTerminatingError'
    ))
}

function Invoke-WriteErrorWithErrorActionStop() {
    Invoke-WriteError -ErrorAction 'Stop'

    Write-Host "This line should not be reached"
}


function Invoke-ThrowTerminatingErrorAndLog() {
    Invoke-ThrowTerminatingError

    Write-Host "This line should be reached"
}

[johlju]

I did not know that $PScmdlet.ThrowTerminatingError did not actually terminated the script. Agree that the change that was made in the PR was the correct one.

But if the above function 'Invoke-ThrowTerminatingErrorAndLog` is made like this:

function Invoke-ThrowTerminatingErrorAndLog {
    [CmdletBinding()]
    param (
    )
    
    Invoke-ThrowTerminatingError

    Write-Host "This line should be reached"
}

Then called with -ErrorAction 'Stop' then the `Write-Host´ are not reached.

PS > Invoke-ThrowTerminatingErrorAndLog -ErrorAction 'Stop'
Invoke-ThrowTerminatingError: 
Line |
   6 |      Invoke-ThrowTerminatingError
     |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Command terminating error

But that requires that the user actually knows to pass in ErrorAction 'Stop' to actually stop the script.

[johlju]

Testing this with a pipeline this is how I would expected it to work

function Invoke-PipelineThrow 
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true, ValueFromPipeline = $true)]
        [System.String]
        $MyStringValue 
    )

    process
    {
        Write-Host ("Start PROCESS for value '{0}'" -f $MyStringValue)

        if ($MyStringValue -eq 'ThrowWriteError')
        {
            Write-Error -Message ("Non-terminating error for value '{0}'" -f $MyStringValue)
        }

        if ($MyStringValue -eq 'ThrowTerminatingError')
        {
            $PScmdlet.ThrowTerminatingError(
                [System.Management.Automation.ErrorRecord]::new(
                ("Terminating error for value '{0}'" -f $MyStringValue),
                'UniqueID',
                [System.Management.Automation.ErrorCategory]::InvalidOperation,
                $MyStringValue
            ))
        }

        Write-Host ("Ran all of PROCESS for value '{0}'" -f $MyStringValue)
    }

    end
    {
        Write-Host "Reached END"
    }
}

Running this we see that $PScmdlet.ThrowTerminatingError will terminate the pipeline (as I would ecpect), while Write-Error will not stop the pipeline as it is a non-terminating error.

PS > 'FirstValue', 'SecondValue', 'ThrowWriteError', 'ThrowTerminatingError', 'LastValue' | Invoke-PipelineThrow
Start PROCESS for value 'FirstValue'
Ran all of PROCESS for value 'FirstValue'
Start PROCESS for value 'SecondValue'
Ran all of PROCESS for value 'SecondValue'
Start PROCESS for value 'ThrowWriteError'
Invoke-PipelineThrow: Non-terminating error for value 'ThrowWriteError'
Ran all of PROCESS for value 'ThrowWriteError'
Start PROCESS for value 'ThrowTerminatingError'
Invoke-PipelineThrow: Terminating error for value 'ThrowTerminatingError'

Even if we pass in 'SilentlyContinue' the $PScmdlet.ThrowTerminatingError will still terminate the pipeline.

PS > 'FirstValue', 'SecondValue', 'ThrowWriteError', 'ThrowTerminatingError', 'LastValue' | Invoke-PipelineThrow -ErrorAction 'SilentlyContinue'
Start PROCESS for value 'FirstValue'
Ran all of PROCESS for value 'FirstValue'
Start PROCESS for value 'SecondValue'
Ran all of PROCESS for value 'SecondValue'
Start PROCESS for value 'ThrowWriteError'
Ran all of PROCESS for value 'ThrowWriteError'
Start PROCESS for value 'ThrowTerminatingError'
Invoke-PipelineThrow: Terminating error for value 'ThrowTerminatingError'

If we pass in ErrorAction 'Stop' we will se that Write-Error will also become a terminating error.

PS > 'FirstValue', 'SecondValue', 'ThrowWriteError', 'ThrowTerminatingError', 'LastValue' | Invoke-PipelineThrow -ErrorAction 'Stop'
Start PROCESS for value 'FirstValue'
Ran all of PROCESS for value 'FirstValue'
Start PROCESS for value 'SecondValue'
Ran all of PROCESS for value 'SecondValue'
Start PROCESS for value 'ThrowWriteError'
Invoke-PipelineThrow: Non-terminating error for value 'ThrowWriteError'

Non of these will reach end so any clean up in end will not run if throwing a terminating error.

So I guess one must use $PScmdlet.ThrowTerminatingError and Write-Error depending on how the commands should be used/behave. I normally use Write-Error in Get-*-commands where errors should be able to be ignored and use $PScmdlet.ThrowTerminatingError in commands that change state so they terminate the action. But what I did not know was that $PScmdlet.ThrowTerminatingError did not terminate the parent function/script that use it (unless passing 'Stop' to ErrorAction. Learned something new, thank you.