xRDSessionCollection: Fix Issues #105 and #106

[uw-dc]

Pull Request (PR) description

This PR supercedes #107 and #108, to hopefully save our future selves time and pain in handling merge conflicts.

If this PR is merged, please close the two earlier PRs referenced above

xRDSessionCollection's Get-TargetResource has been amended so that it filters the Session Collections returned from Get-RDSessionCollection on CollectionName matching the desired $CollectionName, in all circumstances.

xRDSessionCollection will now ignore errors that are returned from the underlying New-RDSessionCollection cmdlet because errors are always returned when certain RD Host GPO settings are applied. xRDSessionCollection as part of Set-TargetResource now instead checks whether the desired Session Collection has been created, and if not throws an exception.

This Pull Request (PR) fixes the following issues

• Fixes #105
• Fixes #106
• Closes #107
• Closes #108

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).
• [ ] 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.
• [x] Unit tests added/updated. See DSC Community Testing Guidelines.
• [ ] Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
• [x] New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[codecov[bot]]

Codecov Report

Merging #109 (30e0310) into master (b97c82b) will increase coverage by 0%. The diff coverage is 98%.

@@          Coverage Diff          @@
##           master   #109   +/-   ##
=====================================
  Coverage      88%    89%           
=====================================
  Files           9     10    +1     
  Lines         461    509   +48     
=====================================
+ Hits          410    457   +47     
- Misses         51     52    +1     

Impacted Files	Coverage Δ
...onBrokerHAMode/MSFT_xRDConnectionBrokerHAMode.psm1	96% <96%> (ø)
...RDSessionCollection/MSFT_xRDSessionCollection.psm1	97% <100%> (+1%)	:arrow_up:

[uw-dc]

@danielboth sorry to trouble, you; I'm just wondering if it's possible to get a review on this PR please? Been waiting for some months now.

[uw-dc]

Thank you for the review. Very much appreciated.

    if ($Collection.Count -eq 0)
    {
        return $null

We should always return all the properties, so we should change this to:

Suggestion:

return @{
    "CollectionName" = $CollectionName
    "SessionHost" = $SessionHost
    "ConnectionBroker" = $ConnectionBroker
    "CollectionDescription" = $null
}

Returning the properties, but all except $SessionHost as $null when no existing matching RDCollection is returned, on account that there is no $ensure property for this resource. (That sounds like a breaking change which should be tackled separately?

_source/DSCResources/MSFT_xRDSessionCollection/MSFT_xRDSessionCollection.psm1 line 45 at r1 (raw file):_

    if ($Collection.Count -gt 1)
    {
        $CollectionType = $Collection.GetType()

Suggest we check the types directly like below. But do we need this check, isn't it enough that the count was 2 or more above?

Suggestion:

        if ($Collection -is [System.Array] -or $Collection.GetType().BaseType -is [System.Array])
        {
            throw 'Non-singular RDSessionCollection in result set'
        }

This was a good shout. I checked the type on account that it fixed the tests. On further investigation, the output the original mocks (which I'd copied, pasted and edited) was not a close enough match to the real implementation. It looks like the original logic I wrote was flawed anyway.

[uw-dc]

Reviewed 1 of 3 files at r3, 3 of 3 files at r4, all commit messages. Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @uw-dc)

_source/DSCResources/MSFT_xRDSessionCollection/MSFT_xRDSessionCollection.psm1 line 82 at r4 (raw file):_

    )

    $PSBoundParameters.Add('ErrorAction','SilentlyContinue')

After thinking again around this change I would like to suggest another approach. With the suggested change a user would never know what error occured if the error is any other than those that is "false negatives".

I would suggest calling the command New-RDSessionCollection with -ErrorAction 'Stop'. Then wrapping that command in a try-catch-block where the catch block singles out the false positives and re-throws on any (actual) other error. Then we would not need to call Test-TargetResource either.

How about calling New-RDSessionCollection with -ErrorAction Stop but instead of working through the exception types, we stash the exception and retain the call to Test-TargetResource. If the new RDSessionCollection has not been created, then the logic can re-throw?

I think this will keep the logic a bit simpler and given that the RemoteDesktop cmdlets are not that robust, will effectively handle any false-negative situation.

[johlju]

Let's try your suggestion.

[johlju]

Thank you for this @uw-dc! 🙂

[uw-dc]

Thank you for this @uw-dc! 🙂

Thank you!