xRDSessionDeployment can now remove servers, improved test function #124
+25
−35
Conversation
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
WalkthroughUpdates CHANGELOG.md and MSFT_xRDSessionDeployment.psm1: returned array types annotated, Set-TargetResource adds removal of servers for SessionHost and WebAccessServer and tweaks a log message, and Test-TargetResource now delegates validation to Test-DscParameterState using desired/current state maps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor LCM as DSC LCM
participant Res as MSFT_xRDSessionDeployment
rect rgb(250,250,255)
note right of LCM: Query current deployment
LCM->>Res: Get-TargetResource()
Res-->>LCM: { SessionHost: [string[]], WebAccessServer: [string[]], ... }
end
rect rgb(245,255,245)
note right of LCM: Consolidated state check
LCM->>Res: Test-TargetResource(params)
Res->>Res: Build $desiredState, $currentState
Res->>Res: Test-DscParameterState(Current, Desired, -SortArrayValues)
Res-->>LCM: Boolean (inDesiredState)
end
alt Not in desired state
rect rgb(255,250,240)
note right of LCM: Reconcile deployment
LCM->>Res: Set-TargetResource(params)
par Add missing
Res->>Infra: Add RDS-RD-SERVER for new SessionHost
Res->>Infra: Add RDS-WEB-ACCESS for new WebAccessServer
and Remove extra
Res->>Infra: Remove RDS-RD-SERVER for removed SessionHost
Res->>Infra: Remove RDS-WEB-ACCESS for removed WebAccessServer
end
Res-->>LCM: Completed
end
else Already desired
LCM-->>LCM: No changes required
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow Requirements
- Run PowerShell script files from repository root
- Setup build and test environment (once per
pwshsession):./build.ps1 -Tasks noop- Build project before running tests:
./build.ps1 -Tasks build- Always run tests in new
pwshsession:Invoke-Pester -Path @({test paths}) -Output DetailedFile Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow instructions over existing code patterns
- Follow PowerShell style and test guideline instructions strictly
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- DSC resources should always be created as class-based resources
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
CHANGELOG.mdsource/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- Escape backslashes in file paths only (not in code blocks)
- Code blocks must specify language identifiers
Text Formatting
- Parameters: bold
- Values/literals:
inline code- Resource/module/product names: italic
- Commands/files/paths:
inline code
Files:
CHANGELOG.md
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: # Changelog Guidelines
- Always update the Unreleased section in CHANGELOG.md
- Use Keep a Changelog format
- Describe notable changes briefly, ≤2 items per change type
- Reference issues using format issue #<issue_number>
- No empty lines between list items in same section
- Skip adding entry if same change already exists in Unreleased section
- No duplicate sections or items in Unreleased section
Files:
CHANGELOG.md
{**/*.ps1,**/*.psm1,**/*.psd1}
⚙️ CodeRabbit configuration file
{**/*.ps1,**/*.psm1,**/*.psd1}: # PowerShell GuidelinesNaming
- Use descriptive names (3+ characters, no abbreviations)
- Functions: PascalCase with Verb-Noun format using approved verbs
- Parameters: PascalCase
- Variables: camelCase
- Keywords: lower-case
- Classes: PascalCase
- Include scope for script/global/environment variables:
$script:,$global:,$env:File naming
- Class files:
###.ClassName.ps1format (e.g.001.SqlReason.ps1,004.StartupParameters.ps1)Formatting
Indentation & Spacing
- Use 4 spaces (no tabs)
- One space around operators:
$a = 1 + 2- One space between type and variable:
[String] $name- One space between keyword and parenthesis:
if ($condition)- No spaces on empty lines
- Try to limit lines to 120 characters
Braces
- Newline before opening brace (except variable assignments)
- One newline after opening brace
- Two newlines after closing brace (one if followed by another brace or continuation)
Quotes
- Use single quotes unless variable expansion is needed:
'text'vs"text $variable"Arrays
- Single line:
@('one', 'two', 'three')- Multi-line: each element on separate line with proper indentation
- Do not use the unary comma operator (
,) in return statements to force
an arrayHashtables
- Empty:
@{}- Each property on separate line with proper indentation
- Properties: Use PascalCase
Comments
- Single line:
# Comment(capitalized, on own line)- Multi-line:
<# Comment #>format (opening and closing brackets on own line), and indent text- No commented-out code
Comment-based help
- Always add comment-based help to all functions and scripts
- Comment-based help: SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, EXAMPLE sections before function/class
- Comment-based help indentation: keywords 4 spaces, text 8 spaces
- Include examples for all parameter sets and combinations
- INPUTS: List each pipeline‑accepted type (one per line) with a 1‑line description...
Files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
source/DSCResources/**/*.psm1
⚙️ CodeRabbit configuration file
source/DSCResources/**/*.psm1: # MOF-based Desired State Configuration (DSC) Resources GuidelinesRequired Functions
- Every DSC resource must define:
Get-TargetResource,Set-TargetResource,Test-TargetResource- Export using
*-TargetResourcepatternFunction Return Types
Get-TargetResource: Must return hashtable with all resource propertiesTest-TargetResource: Must return boolean ($true/$false)Set-TargetResource: Must not return anything (void)Parameter Guidelines
Get-TargetResource: Only include parameters needed to retrieve actual current state valuesGet-TargetResource: Remove non-mandatory parameters that are never usedSet-TargetResourceandTest-TargetResource: Must have identical parametersSet-TargetResourceandTest-TargetResource: Unused mandatory parameters: Add "Not used in <function_name>" to help commentRequired Elements
- Each function must include
Write-Verboseat least once
Get-TargetResource: Use verbose message starting with "Getting the current state of..."Set-TargetResource: Use verbose message starting with "Setting the desired state of..."Test-TargetResource: Use verbose message starting with "Determining the current state of..."- Use localized strings for all messages (Write-Verbose, Write-Error, etc.)
- Import localized strings using
Get-LocalizedDataat module topError Handling
- Do not use
throwfor terminating errors- Use
try/catchblocks to handle exceptions- Throw localized exceptions using the appropriate
New-*Exceptioncmdlet:
New‑InvalidDataExceptionNew-ArgumentExceptionNew-InvalidOperationException- [
New-ObjectNotFoundException](https:/...
Files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
🧠 Learnings (5)
📚 Learning: 2025-09-12T13:20:57.155Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-mof-resources.instructions.md:0-0
Timestamp: 2025-09-12T13:20:57.155Z
Learning: Applies to source/DSCResources/**/*.psm1 : Test-TargetResource must return a boolean ($true/$false)
Applied to files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
📚 Learning: 2025-09-12T13:20:57.155Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-mof-resources.instructions.md:0-0
Timestamp: 2025-09-12T13:20:57.155Z
Learning: Applies to source/DSCResources/**/*.psm1 : Test-TargetResource: Write-Verbose message should start with "Determining the current state of..."
Applied to files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
📚 Learning: 2025-09-12T13:20:57.155Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-mof-resources.instructions.md:0-0
Timestamp: 2025-09-12T13:20:57.155Z
Learning: Applies to source/DSCResources/**/*.psm1 : Set-TargetResource: Write-Verbose message should start with "Setting the desired state of..."
Applied to files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
📚 Learning: 2025-09-12T13:20:57.155Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-mof-resources.instructions.md:0-0
Timestamp: 2025-09-12T13:20:57.155Z
Learning: Applies to source/DSCResources/**/*.psm1 : Set-TargetResource and Test-TargetResource must have identical parameters
Applied to files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
📚 Learning: 2025-09-12T13:20:57.155Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-mof-resources.instructions.md:0-0
Timestamp: 2025-09-12T13:20:57.155Z
Learning: Applies to source/DSCResources/**/*.psm1 : Get-TargetResource: Write-Verbose message should start with "Getting the current state of..."
Applied to files:
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1
🪛 LanguageTool
CHANGELOG.md
[grammar] ~30-~30: There might be a mistake here.
Context: ...ses Test-DscParameterState internally. - xRDServer - Improve error handling in ...
(QB_NEW_EN)
🔇 Additional comments (5)
source/DSCResources/MSFT_xRDSessionDeployment/MSFT_xRDSessionDeployment.psm1 (5)
45-47: LGTM: Explicit type annotations improve clarity.The explicit
[string[]]casts make the return types clearer and more maintainable.
90-94: LGTM: SessionHost removal correctly implemented.The removal logic correctly identifies servers to remove by comparing current state against desired state. Using
-Forceis appropriate for DSC resources to avoid interactive prompts.
98-98: LGTM: Improved log message specificity.The updated message "Adding Web Server '$server' to deployment." is more specific and clearer than the previous generic message.
102-106: LGTM: WebAccessServer removal correctly implemented.The removal logic is consistent with the SessionHost removal pattern and correctly implements the feature for removing web access servers.
128-137: Replace backticks with splatting.The backtick continuations violate the “never use backtick” guideline—use a splatted parameter hashtable for clarity:
- $desiredState = $PSBoundParameters - $currentState = Get-TargetResource @PSBoundParameters - - $result = Test-DscParameterState ` - -CurrentValues $currentState ` - -DesiredValues $desiredState ` - -SortArrayValues ` - -Verbose:$VerbosePreference - - return $result + $desiredState = $PSBoundParameters + $currentState = Get-TargetResource @PSBoundParameters + + $testParameters = @{ + CurrentValues = $currentState + DesiredValues = $desiredState + SortArrayValues = $true + Verbose = $VerbosePreference + } + + return (Test-DscParameterState @testParameters)Also confirm that
DscResource.Commonis listed in the module’s RequiredModules (or imported) so thatTest-DscParameterStateis available at runtime.
Comment on lines
+29
to
+30
| - Servers can be also removed from the deployment. | ||
| - Test function now uses `Test-DscParameterState` internally. |
There was a problem hiding this comment.
Grammar and punctuation consistency for Changed > xRDSessionDeployment
- Use “can also be removed” (natural word order).
- Drop trailing periods to match neighboring bullets.
- Prefer explicit “Test-TargetResource” over “Test function” for clarity.
- - Servers can be also removed from the deployment.
- - Test function now uses `Test-DscParameterState` internally.
+ - Servers can also be removed from the deployment
+ - Test-TargetResource now uses `Test-DscParameterState` internally📝 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.
Suggested change
| - Servers can be also removed from the deployment. | |
| - Test function now uses `Test-DscParameterState` internally. | |
| - Servers can also be removed from the deployment | |
| - Test-TargetResource now uses `Test-DscParameterState` internally |
🧰 Tools
🪛 LanguageTool
[grammar] ~30-~30: There might be a mistake here.
Context: ...ses Test-DscParameterState internally. - xRDServer - Improve error handling in ...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 29 to 30, the two bullets under "Changed >
xRDSessionDeployment" have awkward word order, inconsistent punctuation, and an
ambiguous reference; change "Servers can be also removed from the deployment."
to "Servers can also be removed from the deployment" (move "also" and remove
trailing period), and change "Test function now uses `Test-DscParameterState`
internally." to "Test-TargetResource now uses `Test-DscParameterState`
internally" (use explicit name and drop trailing period) so the bullets match
neighboring items in style and clarity.
|
Would you like me to incorporate this PR into #122? |
|
Yes, please and thanks, @dan-hughes. |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
xRDSessionDeployment can now remove servers, improved test function
This Pull Request (PR) fixes the following issues
NA
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