Update to RemoteDesktopServicesDsc#247
Open
dan-hughes wants to merge 5 commits intodsccommunity:mainfrom
Open
Conversation
WalkthroughReplaced DSC dependency xRemoteDesktopSessionHost with RemoteDesktopServicesDsc (v4.1.0) across configs and docs, regenerated multiple module GUIDs, made WebAccess and SessionHosts parameters mandatory in RemoteDesktopDeployment and removed the gateways loop, and re-enabled previously skipped DSC resource test suites. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/DSCResources/RemoteDesktopCollections/RemoteDesktopCollections.schema.psm1 (1)
46-70:⚠️ Potential issue | 🟠 MajorReset
$collectionSettingsbefore each collection.If one collection has
Settingsand the next one does not, theRDSessionCollectionConfigurationcall at Line 70 reuses the previous hashtable because$collectionSettingsis never cleared. That applies the wrong settings to later collections.🛠️ Minimal fix
foreach ($collection in $Collections) { + $collectionSettings = $null $executionName = "rdsc_$($collection.CollectionName -replace '[().:\s]', '')" if ($collection.Settings) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/DSCResources/RemoteDesktopCollections/RemoteDesktopCollections.schema.psm1` around lines 46 - 70, The bug is that $collectionSettings persists between iterations so RDSessionCollectionConfiguration can reuse the previous hashtable; inside the foreach ($collection in $Collections) loop, ensure $collectionSettings is reset before using it (e.g. set $collectionSettings = $null or = @{} at the start of each iteration) so the logic around $collection.Settings, adding ConnectionBroker/CollectionName, and the subsequent (Get-DscSplattedResource -ResourceName RDSessionCollectionConfiguration ...).Invoke($collectionSettings) call only uses settings for the current $collection and does not carry over prior values.source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.schema.psm1 (1)
17-19:⚠️ Potential issue | 🔴 Critical
Gatewaysis accepted but ignored.The parameter is still public and documented, but the only code path that used it is commented out. Any supplied gateway entries are silently dropped, so gateway deployment no longer works.
As per coding guidelines, "No commented-out code".
Also applies to: 31-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.schema.psm1` around lines 17 - 19, The public parameter $Gateways is declared but ignored because the code path that processed it was commented out; either remove the $Gateways declaration and related documentation or restore the gateway-processing logic by uncommenting and wiring the original handling (the commented block around lines 31-36) so each entry in $Gateways is iterated and passed to the gateway deployment routine used elsewhere in RemoteDesktopDeployment.schema.psm1; ensure no commented-out code remains and update unit tests/docs accordingly.
🧹 Nitpick comments (2)
tests/Unit/DSCResources/DscResources.Tests.ps1 (1)
18-18: Use one skip list for both code paths.
$skippedDscResourcesand$skippednow represent the same filter in two different shapes (''vs@()). Keeping them separate makes discovery and WinPS batch compilation easy to desynchronize again.♻️ Suggested simplification
- $skippedDscResources = '' + $skippedDscResources = @() @@ - $skipped = @() $resourceNames = Get-ChildItem -Path "$($moduleUnderTest.ModuleBase)\DSCResources\*" -Directory | - Where-Object { $_.BaseName -notin $skipped } | + Where-Object { $_.BaseName -notin $skippedDscResources } | ForEach-Object { $_.BaseName }Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/DSCResources/DscResources.Tests.ps1` at line 18, Unify the two skip-list variables into a single consistently-typed variable (e.g., use an empty array @() rather than ''), replacing both $skippedDscResources and $skipped usages so there is only one source of truth for the filter; update any code that currently initializes or reads $skippedDscResources or $skipped (including the test sections around the batch compilation/discovery logic) to use the single variable name and array shape so both code paths share the same skip list representation.CHANGELOG.md (1)
16-34: Condense the newUnreleased > Changedentries.These additions create three new top-level bullets under
Changed, which pushes the section past the repo's limit for a single change type. Please fold them into one or two bullets.As per coding guidelines, "Describe notable changes briefly, ≤2 items per change type".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 16 - 34, Condense the three separate bullets under "Unreleased > Changed" into one or two concise bullets: combine the xRemoteDesktopSessionHost rename/version/doc updates (referencing xRemoteDesktopSessionHost → RemoteDesktopServicesDsc and version 4.1.0) into a single bullet, and merge the Tests and Module GUIDs lists into a single second bullet that briefly states "Re-enabled tests for PowerShellRepositories, RemoteDesktopCollections, RemoteDesktopDeployment and regenerated GUIDs for related modules (PowerShellRepositories, RemoteDesktopCertificates, RemoteDesktopCollections, RemoteDesktopDeployment, RemoteDesktopHAMode, RemoteDesktopLicensing, RenameNetworkAdapters, RestartSystem)"; ensure each change type has ≤2 items and keep wording brief.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.schema.psm1`:
- Around line 9-15: The change made WebAccess and SessionHosts parameters of the
RemoteDesktopDeployment DSC resource mandatory (the [Parameter(Mandatory =
$true)] attributes), which silently breaks existing configurations; revert these
to optional by removing or setting Mandatory to $false for the $WebAccess and
$SessionHosts parameters in RemoteDesktopDeployment.schema (or restore their
previous parameter attributes) and update doc/RemoteDesktopDeployment.adoc and
release notes to reflect any intentional contract change if you actually intend
to make them required.
---
Outside diff comments:
In
`@source/DSCResources/RemoteDesktopCollections/RemoteDesktopCollections.schema.psm1`:
- Around line 46-70: The bug is that $collectionSettings persists between
iterations so RDSessionCollectionConfiguration can reuse the previous hashtable;
inside the foreach ($collection in $Collections) loop, ensure
$collectionSettings is reset before using it (e.g. set $collectionSettings =
$null or = @{} at the start of each iteration) so the logic around
$collection.Settings, adding ConnectionBroker/CollectionName, and the subsequent
(Get-DscSplattedResource -ResourceName RDSessionCollectionConfiguration
...).Invoke($collectionSettings) call only uses settings for the current
$collection and does not carry over prior values.
In
`@source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.schema.psm1`:
- Around line 17-19: The public parameter $Gateways is declared but ignored
because the code path that processed it was commented out; either remove the
$Gateways declaration and related documentation or restore the
gateway-processing logic by uncommenting and wiring the original handling (the
commented block around lines 31-36) so each entry in $Gateways is iterated and
passed to the gateway deployment routine used elsewhere in
RemoteDesktopDeployment.schema.psm1; ensure no commented-out code remains and
update unit tests/docs accordingly.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 16-34: Condense the three separate bullets under "Unreleased >
Changed" into one or two concise bullets: combine the xRemoteDesktopSessionHost
rename/version/doc updates (referencing xRemoteDesktopSessionHost →
RemoteDesktopServicesDsc and version 4.1.0) into a single bullet, and merge the
Tests and Module GUIDs lists into a single second bullet that briefly states
"Re-enabled tests for PowerShellRepositories, RemoteDesktopCollections,
RemoteDesktopDeployment and regenerated GUIDs for related modules
(PowerShellRepositories, RemoteDesktopCertificates, RemoteDesktopCollections,
RemoteDesktopDeployment, RemoteDesktopHAMode, RemoteDesktopLicensing,
RenameNetworkAdapters, RestartSystem)"; ensure each change type has ≤2 items and
keep wording brief.
In `@tests/Unit/DSCResources/DscResources.Tests.ps1`:
- Line 18: Unify the two skip-list variables into a single consistently-typed
variable (e.g., use an empty array @() rather than ''), replacing both
$skippedDscResources and $skipped usages so there is only one source of truth
for the filter; update any code that currently initializes or reads
$skippedDscResources or $skipped (including the test sections around the batch
compilation/discovery logic) to use the single variable name and array shape so
both code paths share the same skip list representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b57ce36c-04fd-4bf9-b692-3ee68d77b12b
📒 Files selected for processing (24)
CHANGELOG.mdREADME.mdRequiredModules.psd1doc/RemoteDesktopCertificates.adocdoc/RemoteDesktopCollections.adocdoc/RemoteDesktopDeployment.adocdoc/RemoteDesktopHAMode.adocdoc/RemoteDesktopLicensing.adocdoc/RemoteDesktopServers.adocsource/DSCResources/PowerShellRepositories/PowerShellRepositories.psd1source/DSCResources/RemoteDesktopCertificates/RemoteDesktopCertificates.psd1source/DSCResources/RemoteDesktopCertificates/RemoteDesktopCertificates.schema.psm1source/DSCResources/RemoteDesktopCollections/RemoteDesktopCollections.psd1source/DSCResources/RemoteDesktopCollections/RemoteDesktopCollections.schema.psm1source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.psd1source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.schema.psm1source/DSCResources/RemoteDesktopHAMode/RemoteDesktopHAMode.psd1source/DSCResources/RemoteDesktopHAMode/RemoteDesktopHAMode.schema.psm1source/DSCResources/RemoteDesktopLicensing/RemoteDesktopLicensing.psd1source/DSCResources/RemoteDesktopLicensing/RemoteDesktopLicensing.schema.psm1source/DSCResources/RemoteDesktopServers/RemoteDesktopServers.schema.psm1source/DSCResources/RenameNetworkAdapters/RenameNetworkAdapters.psd1source/DSCResources/RestartSystem/RestartSystem.psd1tests/Unit/DSCResources/DscResources.Tests.ps1
Comment on lines
+9
to
15
| [Parameter(Mandatory = $true)] | ||
| [string] | ||
| $WebAccess, | ||
|
|
||
| [Parameter()] | ||
| [Parameter(Mandatory = $true)] | ||
| [string[]] | ||
| $SessionHosts, |
There was a problem hiding this comment.
This silently changes the public contract.
Line 9 and Line 13 make WebAccess and SessionHosts mandatory, so existing RemoteDesktopDeployment entries that omitted them will now fail to compile. doc/RemoteDesktopDeployment.adoc still marks both as optional, and the Unreleased notes do not call out the break.
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[error] 9-9: Unexpected attribute 'Parameter'.
(UnexpectedAttribute)
[error] 13-13: Unexpected attribute 'Parameter'.
(UnexpectedAttribute)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@source/DSCResources/RemoteDesktopDeployment/RemoteDesktopDeployment.schema.psm1`
around lines 9 - 15, The change made WebAccess and SessionHosts parameters of
the RemoteDesktopDeployment DSC resource mandatory (the [Parameter(Mandatory =
$true)] attributes), which silently breaks existing configurations; revert these
to optional by removing or setting Mandatory to $false for the $WebAccess and
$SessionHosts parameters in RemoteDesktopDeployment.schema (or restore their
previous parameter attributes) and update doc/RemoteDesktopDeployment.adoc and
release notes to reflect any intentional contract change if you actually intend
to make them required.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/DSCResources/DscPullServerSql/DscPullServerSql.schema.psm1`:
- Around line 16-22: The parameters SqlServer and DatabaseName in
DscPullServerSql.schema.psm1 were made optional with defaults ('localhost' and
'DSC'), changing runtime behavior; either revert them to required parameters by
removing the default values and adding [ValidateNotNullOrEmpty()] to the
SqlServer and DatabaseName parameter declarations, or add unit/config tests that
exercise the new default path by invoking the resource/module without providing
SqlServer and/or DatabaseName and asserting the effective defaults ('localhost'
and 'DSC') and any downstream behavior (e.g., connection attempts) are as
expected; refer to the SqlServer and DatabaseName parameter declarations in
DscPullServerSql.schema.psm1 and the module/resource invocation tests to
implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 583e609d-5df4-49ad-9b18-ca49e746b38d
📒 Files selected for processing (1)
source/DSCResources/DscPullServerSql/DscPullServerSql.schema.psm1
Comment on lines
+16
to
22
| [Parameter()] | ||
| [string] | ||
| $SqlServer = 'localhost', | ||
|
|
||
| [Parameter(Mandatory = $true)] | ||
| [Parameter()] | ||
| [string] | ||
| $DatabaseName = 'DSC', |
There was a problem hiding this comment.
Behavior contract changed to implicit SQL defaults without coverage
Line 16 and Line 20 make SqlServer/DatabaseName optional, which now allows silent fallback to 'localhost'/'DSC'. That is a meaningful behavior change and can mis-target deployments if callers omit either value. Current config asset coverage still supplies both values, so this path is untested.
Please either (a) keep these parameters required, or (b) add unit/config tests that intentionally omit them and assert the default behavior is expected.
As per coding guidelines: “Add unit tests for all commands/functions/resources”.
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[error] 16-16: Unexpected attribute 'Parameter'.
(UnexpectedAttribute)
[error] 20-20: Unexpected attribute 'Parameter'.
(UnexpectedAttribute)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/DSCResources/DscPullServerSql/DscPullServerSql.schema.psm1` around
lines 16 - 22, The parameters SqlServer and DatabaseName in
DscPullServerSql.schema.psm1 were made optional with defaults ('localhost' and
'DSC'), changing runtime behavior; either revert them to required parameters by
removing the default values and adding [ValidateNotNullOrEmpty()] to the
SqlServer and DatabaseName parameter declarations, or add unit/config tests that
exercise the new default path by invoking the resource/module without providing
SqlServer and/or DatabaseName and asserting the effective defaults ('localhost'
and 'DSC') and any downstream behavior (e.g., connection attempts) are as
expected; refer to the SqlServer and DatabaseName parameter declarations in
DscPullServerSql.schema.psm1 and the module/resource invocation tests to
implement the change.
Contributor
Author
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
Update RemoteDesktop* resources to RemoteDesktopServicesDsc.
Enable tests
Fix PSGallery badge in README.
Make GUIDs unique.
Pull Request (PR) description
Task list
build.ps1 -ResolveDependency).and comment-based help.
This change is