Convert RDConnectionBrokerHAMode to class-based#155
Open
dan-hughes wants to merge 14 commits into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughConverts RDConnectionBrokerHAMode from a legacy function/MOF DSC resource to a class-based ResourceBase DSC class, removes legacy module/schema, adds class implementation and comprehensive Pester tests, adds localization, and updates 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 🚥 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 526 -10
===================================
- Hits 524 516 -8
+ Misses 12 10 -2
🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai rate limit |
|
Review rate limit: 1/1 review remaining, available now. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/Classes/020.RDConnectionBrokerHAMode.ps1 (1)
100-110: 💤 Low valueExtract the
ConnectionBrokerFQDN fallback into a helper.The same "default to
Get-ComputerName -FullyQualifiedDomainNamewhen empty" logic is duplicated inGetCurrentStateandModify. A small private helper (e.g.,Resolve-ConnectionBroker) or a single local variable computed once at the top of each method would keep the two call sites in sync and avoid future drift.Also applies to: 132-140
🤖 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` around lines 100 - 110, Extract the repeated "default to Get-ComputerName -FullyQualifiedDomainName when $this.ConnectionBroker is empty" logic into a single helper or local resolver and use it in both GetCurrentState and Modify: create a private helper named Resolve-ConnectionBroker (or compute a single local variable $resolvedConnectionBroker at the top of each method) that returns $this.ConnectionBroker if not null/whitespace otherwise calls Get-ComputerName -FullyQualifiedDomainName; then update the call sites that build $getParameters and the call to Get-RDConnectionBrokerHighAvailability to use the resolved value instead of duplicating the IsNullOrWhiteSpace check.
🤖 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 `@RequiredModules.psd1`:
- Line 15: Replace the non-deterministic Configuration value in
RequiredModules.psd1 by pinning the Configuration entry (symbol: Configuration)
to a specific stable version (e.g. '1.6.0') or a bounded range (e.g. '>=1.6.0
<2.0.0') instead of 'latest'; retain the temporary comment but update it to
reference the removal condition or issue ID so future maintainers know when to
revert the pin.
---
Nitpick comments:
In `@source/Classes/020.RDConnectionBrokerHAMode.ps1`:
- Around line 100-110: Extract the repeated "default to Get-ComputerName
-FullyQualifiedDomainName when $this.ConnectionBroker is empty" logic into a
single helper or local resolver and use it in both GetCurrentState and Modify:
create a private helper named Resolve-ConnectionBroker (or compute a single
local variable $resolvedConnectionBroker at the top of each method) that returns
$this.ConnectionBroker if not null/whitespace otherwise calls Get-ComputerName
-FullyQualifiedDomainName; then update the call sites that build $getParameters
and the call to Get-RDConnectionBrokerHighAvailability to use the resolved value
instead of duplicating the IsNullOrWhiteSpace check.
🪄 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: ab4e9e9f-80c2-46b0-a10b-909b19dc816d
📒 Files selected for processing (15)
CHANGELOG.mdRequiredModules.psd1source/Classes/020.RDConnectionBrokerHAMode.ps1tests/Unit/Classes/RDConnectionBrokerHAMode.Tests.ps1tests/Unit/Classes/RDSReason.Tests.ps1tests/Unit/Classes/RDSessionDeployment.Tests.ps1tests/Unit/DSC_RDCertificateConfiguration.Tests.ps1tests/Unit/DSC_RDGatewayConfiguration.Tests.ps1tests/Unit/DSC_RDLicenseConfiguration.Tests.ps1tests/Unit/DSC_RDRemoteApp.Tests.ps1tests/Unit/DSC_RDServer.Tests.ps1tests/Unit/DSC_RDSessionCollection.Tests.ps1tests/Unit/DSC_RDSessionCollectionConfiguration.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1
✅ Files skipped from review due to trivial changes (10)
- tests/Unit/DSC_RDCertificateConfiguration.Tests.ps1
- tests/Unit/DSC_RDServer.Tests.ps1
- tests/Unit/Classes/RDSReason.Tests.ps1
- tests/Unit/DSC_RDSessionCollection.Tests.ps1
- tests/Unit/DSC_RDSessionCollectionConfiguration.Tests.ps1
- tests/Unit/Classes/RDSessionDeployment.Tests.ps1
- tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
- tests/Unit/DSC_RDRemoteApp.Tests.ps1
- tests/Unit/DSC_RDGatewayConfiguration.Tests.ps1
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@source/Classes/020.RDConnectionBrokerHAMode.ps1`:
- Around line 72-76: Remove DatabaseConnectionString from the
ExcludeDscProperties array (or alternatively ensure Modify() always adds it to
$setParameters) so the property flows through $properties into Modify();
specifically update the ExcludeDscProperties declaration to not include
'DatabaseConnectionString' (or change the Modify() logic to explicitly populate
$setParameters with DatabaseConnectionString even when it's excluded) so
Set-RDConnectionBrokerHighAvailability is always given the required DB
connection value.
🪄 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: c04f3dba-2a71-4dd0-bda8-461c5d7fdbfa
📒 Files selected for processing (1)
source/Classes/020.RDConnectionBrokerHAMode.ps1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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