Convert RDConnectionBrokerHAMode to class-based#155
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughConverts RDConnectionBrokerHAMode from a legacy function/MOF DSC resource to a class-based ResourceBase DSC class, removes the old module and MOF schema, adds class implementation and comprehensive Pester tests, updates localization, and adjusts the module manifest and dependency config. ChangesRDConnectionBrokerHAMode Class-Based Conversion
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ResourceBase
participant RDConnectionBrokerHAMode
participant Get-RDConnectionBrokerHighAvailability
participant Set-RDConnectionBrokerHighAvailability
participant Get-ComputerName
Caller->>ResourceBase: Invoke Get/Set/Test
ResourceBase->>RDConnectionBrokerHAMode: Delegate to Get/Set/Test
RDConnectionBrokerHAMode->>Get-RDConnectionBrokerHighAvailability: GetCurrentState query
RDConnectionBrokerHAMode->>Get-ComputerName: Default ConnectionBroker to FQDN (if empty)
alt Applying changes
RDConnectionBrokerHAMode->>Set-RDConnectionBrokerHighAvailability: Modify applies desired state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
===================================
Coverage 97% 98%
===================================
Files 10 10
Lines 536 528 -8
===================================
- Hits 524 518 -6
+ Misses 12 10 -2
🚀 New features to boost your workflow:
|
|
@coderabbitai rate limit |
|
Review rate limit: 1/1 review remaining, available now. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1 (1)
27-50: ⚡ Quick winConsider using
$script:moduleNameto match the required template.The code uses
$script:dscModuleNamethroughout the setup and teardown blocks, but the required template specifies$script:moduleName. While functionally equivalent, matching the template improves consistency across the codebase.♻️ Proposed fix to align with template
BeforeAll { - $script:dscModuleName = 'RemoteDesktopServicesDsc' + $script:moduleName = 'RemoteDesktopServicesDsc' - Import-Module -Name $script:dscModuleName -ErrorAction Stop + Import-Module -Name $script:moduleName -ErrorAction Stop # Load stub cmdlets and classes. Import-Module (Join-Path -Path $PSScriptRoot -ChildPath '..\Stubs\RemoteDesktop.stubs.psm1') - $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:dscModuleName - $PSDefaultParameterValues['Mock:ModuleName'] = $script:dscModuleName - $PSDefaultParameterValues['Should:ModuleName'] = $script:dscModuleName + $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:moduleName + $PSDefaultParameterValues['Mock:ModuleName'] = $script:moduleName + $PSDefaultParameterValues['Should:ModuleName'] = $script:moduleName } AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName') # Unload stub module Remove-Module -Name RemoteDesktop.stubs -Force # Unload the module being tested so that it doesn't impact any other tests. - Get-Module -Name $script:dscModuleName -All | Remove-Module -Force + Get-Module -Name $script:moduleName -All | Remove-Module -Force }As per coding guidelines, test files should use the exact setup block template specified under "Test Setup Requirements".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1` around lines 27 - 50, Rename the test setup variable to match the required template by changing the declaration and all references of $script:dscModuleName to $script:moduleName in the BeforeAll and AfterAll blocks (the declaration line, the Import-Module call, the PSDefaultParameterValues assignments 'InModuleScope:ModuleName' and 'Mock:ModuleName' usages, the Remove-Module -Name RemoteDesktop.stubs -Force call remains unchanged, and the Get-Module | Remove-Module call should use $script:moduleName); ensure every occurrence inside BeforeAll and AfterAll (including Get-Module -Name $script:dscModuleName -All | Remove-Module -Force) is updated so the file matches the standard template.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 15: Update the changelog entry "Converted to class-based resource [Issue
`#144`](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144)" to
use the required lowercase issue-link label by replacing "[Issue `#144`]" with
"[issue `#144`]" so it matches the repository changelog rule and the required
format referenced in the guidelines.
In `@source/Classes/020.RDConnectionBrokerHAMode.ps1`:
- Line 157: In the AssertProperties function change the stream redirection on
the Import-RemoteDesktopModule call so only the Verbose stream is suppressed;
replace the incorrect "4>&1 > $null" redirection with "4> $null" (i.e., update
the Import-RemoteDesktopModule invocation inside AssertProperties to use 4>
$null instead of 4>&1 > $null).
- Line 38: The DSC resource attribute on the class is missing an explicit
RunAsCredential setting; update the class attribute `[DscResource()]` to include
`RunAsCredential = 'Optional'` so it becomes `[DscResource(RunAsCredential =
'Optional')]`, ensuring the class-based resource explicitly declares
RunAsCredential as required by guidelines.
In `@tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1`:
- Around line 1-3: The SuppressMessageAttribute usage is missing the required
Justification parameter: update the attribute on the test setup block
(System.Diagnostics.CodeAnalysis.SuppressMessageAttribute) to include the
Justification parameter set to a short explanatory string (e.g.
Justification='ScriptAnalyzer doesn't understand Pester syntax'), matching the
exact template required by the Test Setup Requirements so the attribute becomes
something like SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments',
'', Justification='...') or using the named parameter Justification='<reason>'.
- Around line 5-25: In the BeforeDiscovery setup block update the two inline
comments that incorrectly say "dependencies has" to the grammatically correct
"dependencies have": change the comment above the noop invocation ("# Assumes
dependencies have been resolved, so if this module is not available, run 'noop'
task.") and the comment before Import-Module ("# If the dependencies have not
been resolved, this will throw an error.") so they match the required test setup
template; locate these comments near the Get-Module checks and the calls to "&
\"$PSScriptRoot/../../../build.ps1\" -Tasks 'noop'" and "Import-Module -Name
'DscResource.Test' -Force -ErrorAction 'Stop'".
---
Nitpick comments:
In `@tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1`:
- Around line 27-50: Rename the test setup variable to match the required
template by changing the declaration and all references of $script:dscModuleName
to $script:moduleName in the BeforeAll and AfterAll blocks (the declaration
line, the Import-Module call, the PSDefaultParameterValues assignments
'InModuleScope:ModuleName' and 'Mock:ModuleName' usages, the Remove-Module -Name
RemoteDesktop.stubs -Force call remains unchanged, and the Get-Module |
Remove-Module call should use $script:moduleName); ensure every occurrence
inside BeforeAll and AfterAll (including Get-Module -Name $script:dscModuleName
-All | Remove-Module -Force) is updated so the file matches the standard
template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af7bbaf0-fbd7-4fbe-a376-b7e40b78ddd2
📒 Files selected for processing (10)
CHANGELOG.mdsource/Classes/020.RDConnectionBrokerHAMode.ps1source/DSCResources/DSC_RDConnectionBrokerHAMode/DSC_RDConnectionBrokerHAMode.psm1source/DSCResources/DSC_RDConnectionBrokerHAMode/DSC_RDConnectionBrokerHAMode.schema.mofsource/DSCResources/DSC_RDConnectionBrokerHAMode/README.mdsource/DSCResources/DSC_RDConnectionBrokerHAMode/en-US/DSC_RDConnectionBrokerHAMode.strings.psd1source/RemoteDesktopServicesDsc.psd1source/en-US/RDConnectionBrokerHAMode.strings.psd1tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1tests/Unit/DSC_RDConnectionBrokerHAMode.Tests.ps1
💤 Files with no reviewable changes (5)
- source/DSCResources/DSC_RDConnectionBrokerHAMode/DSC_RDConnectionBrokerHAMode.schema.mof
- source/DSCResources/DSC_RDConnectionBrokerHAMode/README.md
- source/DSCResources/DSC_RDConnectionBrokerHAMode/en-US/DSC_RDConnectionBrokerHAMode.strings.psd1
- tests/Unit/DSC_RDConnectionBrokerHAMode.Tests.ps1
- source/DSCResources/DSC_RDConnectionBrokerHAMode/DSC_RDConnectionBrokerHAMode.psm1
| - WebAccessServer is now NOT a required parameter, mirroring the use | ||
| of the underlying cmdlet. | ||
| - `RDConnectionBrokerHAMode` | ||
| - Converted to class-based resource [Issue #144](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144). |
There was a problem hiding this comment.
Use the required issue-link label format in changelog entries.
Please use lowercase issue in the link label to match the repository changelog rule.
Proposed fix
- - Converted to class-based resource [Issue `#144`](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144).
+ - Converted to class-based resource [issue `#144`](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144).As per coding guidelines, "Reference issues using format issue #<issue_number>".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Converted to class-based resource [Issue #144](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144). | |
| - Converted to class-based resource [issue `#144`](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 15, Update the changelog entry "Converted to
class-based resource [Issue
`#144`](https://github.com/dsccommunity/RemoteDesktopServicesDsc/issues/144)" to
use the required lowercase issue-link label by replacing "[Issue `#144`]" with
"[issue `#144`]" so it matches the repository changelog rule and the required
format referenced in the guidelines.
| Returns the reason a property is not in desired state. | ||
| #> | ||
|
|
||
| [DscResource()] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PowerShell DSC class resources, what is the default behavior when [DscResource()]omitsRunAsCredential, and how does RunAsCredential = 'Optional' affect PsDscRunAsCredential support?
💡 Result:
In PowerShell class-based DSC resources, the [DscResource()] attribute's RunAsCredential property defines how the resource interacts with the automatic PsDscRunAsCredential property [1]. Default Behavior When RunAsCredential is omitted from the [DscResource()] attribute, the default behavior is Optional [1]. This means that a user may optionally use the PsDscRunAsCredential property with the DSC resource [1]. This default behavior is functionally equivalent to explicitly setting RunAsCredential = 'Optional' or RunAsCredential = 'Default' [1]. Effect of 'Optional' Setting RunAsCredential = 'Optional' explicitly enables support for the PsDscRunAsCredential property [1]. When this is set (or defaulted), the DSC engine allows the user to provide a PSCredential object via the PsDscRunAsCredential parameter in the configuration [2][1]. If provided, the DSC engine attempts to execute the resource's methods under the context of the specified user account [2]. Important Context While these settings are defined in the [DscResource()] attribute, they are primarily relevant for DSC v1.1 and earlier [1]. In DSC v2.0 and later, the RunAsCredential property in the [DscResource()] attribute has no functional effect on the execution of the resource [1]. Additionally, the PsDscRunAsCredential property itself is only supported in DSC v1.1 and earlier [1].
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/dsc/concepts/class-based-resources?view=dsc-2.0
- 2: https://learn.microsoft.com/en-us/powershell/dsc/configurations/configdatacredentials?view=dsc-1.1
🏁 Script executed:
# Check the file and examine line 38
if [ -f "source/Classes/020.RDConnectionBrokerHAMode.ps1" ]; then
echo "=== File exists ==="
sed -n '35,42p' source/Classes/020.RDConnectionBrokerHAMode.ps1
else
echo "File not found"
fiRepository: dsccommunity/RemoteDesktopServicesDsc
Length of output: 284
🏁 Script executed:
# Search for all DSC resources and their attribute declarations
echo "=== All DSC resources in Classes directory ==="
find source/Classes -name "*.ps1" -type f -exec grep -l "class.*ResourceBase" {} \;
echo ""
echo "=== Their DscResource attributes ==="
find source/Classes -name "*.ps1" -type f -exec grep -B2 "class.*ResourceBase" {} + | grep -E "\[DscResource|class"Repository: dsccommunity/RemoteDesktopServicesDsc
Length of output: 558
Add RunAsCredential to the DSC resource attribute.
The coding guidelines require explicit declaration as [DscResource(RunAsCredential = 'Optional')] for class-based resources.
Proposed fix
-[DscResource()]
+[DscResource(RunAsCredential = 'Optional')]
class RDConnectionBrokerHAMode : ResourceBase📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [DscResource()] | |
| [DscResource(RunAsCredential = 'Optional')] | |
| class RDConnectionBrokerHAMode : ResourceBase |
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[info] 38-159: No examples found for resource 'RDConnectionBrokerHAMode'
(PSDSCDscExamplesPresent)
[info] 38-159: No tests found for resource 'RDConnectionBrokerHAMode'
(PSDSCDscTestsPresent)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/Classes/020.RDConnectionBrokerHAMode.ps1` at line 38, The DSC resource
attribute on the class is missing an explicit RunAsCredential setting; update
the class attribute `[DscResource()]` to include `RunAsCredential = 'Optional'`
so it becomes `[DscResource(RunAsCredential = 'Optional')]`, ensuring the
class-based resource explicitly declares RunAsCredential as required by
guidelines.
| } | ||
|
|
||
| # Module Import in verbose mode outputs lots of data, so redirecting to null. | ||
| Import-RemoteDesktopModule 4>&1 > $null |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PowerShell, how is 4>&1 > $null evaluated, and what is the correct redirection syntax to suppress only stream 4 (verbose)?
💡 Result:
In PowerShell, the syntax 4>&1 > $null is evaluated as a two-step redirection process [1][2]. First, 4>&1 redirects the Verbose stream (stream 4) into the Success stream (stream 1) [1][2]. Second, the > $null operator redirects the resulting Success stream (which now contains the original Success output plus the redirected Verbose output) to the $null automatic variable, effectively discarding both [1][2]. To suppress only the Verbose stream (stream 4) without affecting the Success stream, you should redirect stream 4 directly to $null [1][2]. The correct syntax is: 4> $null This directs only the Verbose output to the $null device, leaving the Success stream (stream 1) to be displayed in the console or handled by other redirections as intended [1][2].
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_redirection?view=powershell-7.4
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_redirection?view=powershell-7.6
🏁 Script executed:
cat -n source/Classes/020.RDConnectionBrokerHAMode.ps1 | sed -n '150,165p'Repository: dsccommunity/RemoteDesktopServicesDsc
Length of output: 499
Fix stream redirection order in AssertProperties.
Line 157 uses 4>&1 > $null, which redirects both the Verbose stream (4) and Success stream (1) to $null. To suppress only the Verbose output as intended, use 4> $null.
Proposed fix
- Import-RemoteDesktopModule 4>&1 > $null
+ Import-RemoteDesktopModule 4> $null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Import-RemoteDesktopModule 4>&1 > $null | |
| Import-RemoteDesktopModule 4> $null |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/Classes/020.RDConnectionBrokerHAMode.ps1` at line 157, In the
AssertProperties function change the stream redirection on the
Import-RemoteDesktopModule call so only the Verbose stream is suppressed;
replace the incorrect "4>&1 > $null" redirection with "4> $null" (i.e., update
the Import-RemoteDesktopModule invocation inside AssertProperties to use 4>
$null instead of 4>&1 > $null).
| # Suppressing this rule because Script Analyzer does not understand Pester's syntax. | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '')] | ||
| param () |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add missing Justification parameter to match required template.
The SuppressMessageAttribute is missing the Justification parameter. As per coding guidelines, the test setup block must exactly match the required template.
📋 Proposed fix to match template
# Suppressing this rule because Script Analyzer does not understand Pester's syntax.
-[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '')]
+[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')]
param ()As per coding guidelines, test files must use the exact setup block template specified under "Test Setup Requirements".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1` around lines 1 - 3,
The SuppressMessageAttribute usage is missing the required Justification
parameter: update the attribute on the test setup block
(System.Diagnostics.CodeAnalysis.SuppressMessageAttribute) to include the
Justification parameter set to a short explanatory string (e.g.
Justification='ScriptAnalyzer doesn't understand Pester syntax'), matching the
exact template required by the Test Setup Requirements so the attribute becomes
something like SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments',
'', Justification='...') or using the named parameter Justification='<reason>'.
| BeforeDiscovery { | ||
| try | ||
| { | ||
| if (-not (Get-Module -Name 'DscResource.Test')) | ||
| { | ||
| # Assumes dependencies has been resolved, so if this module is not available, run 'noop' task. | ||
| if (-not (Get-Module -Name 'DscResource.Test' -ListAvailable)) | ||
| { | ||
| # Redirect all streams to $null, except the error stream (stream 2) | ||
| & "$PSScriptRoot/../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null | ||
| } | ||
|
|
||
| # If the dependencies has not been resolved, this will throw an error. | ||
| Import-Module -Name 'DscResource.Test' -Force -ErrorAction 'Stop' | ||
| } | ||
| } | ||
| catch [System.IO.FileNotFoundException] | ||
| { | ||
| throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks build" first.' | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Fix grammatical errors in comments to match required template.
The comments contain grammatical errors that deviate from the required template. "dependencies has" should be "dependencies have" in both occurrences (lines 10 and 17).
📋 Proposed fix to match template
- # Assumes dependencies has been resolved, so if this module is not available, run 'noop' task.
+ # Assumes dependencies have been resolved, so if this module is not available, run 'noop' task.
if (-not (Get-Module -Name 'DscResource.Test' -ListAvailable))
{
# Redirect all streams to $null, except the error stream (stream 2)
& "$PSScriptRoot/../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null
}
- # If the dependencies has not been resolved, this will throw an error.
+ # If the dependencies have not been resolved, this will throw an error.
Import-Module -Name 'DscResource.Test' -Force -ErrorAction 'Stop'As per coding guidelines, test files must use the exact setup block template specified under "Test Setup Requirements".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1` around lines 5 - 25,
In the BeforeDiscovery setup block update the two inline comments that
incorrectly say "dependencies has" to the grammatically correct "dependencies
have": change the comment above the noop invocation ("# Assumes dependencies
have been resolved, so if this module is not available, run 'noop' task.") and
the comment before Import-Module ("# If the dependencies have not been resolved,
this will throw an error.") so they match the required test setup template;
locate these comments near the Get-Module checks and the calls to "&
\"$PSScriptRoot/../../../build.ps1\" -Tasks 'noop'" and "Import-Module -Name
'DscResource.Test' -Force -ErrorAction 'Stop'".
1 participant
Pull Request (PR) description
Convert
RDConnectionBrokerHAModeto a class-based resource.This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
help.
This change is