Skip to content

RDSessionDeployment: Convert to class-based resource#152

Open
dan-hughes wants to merge 8 commits intodsccommunity:mainfrom
dan-hughes:f/class-RDSessionDeployment
Open

RDSessionDeployment: Convert to class-based resource#152
dan-hughes wants to merge 8 commits intodsccommunity:mainfrom
dan-hughes:f/class-RDSessionDeployment

Conversation

@dan-hughes
Copy link
Copy Markdown
Contributor

@dan-hughes dan-hughes commented Mar 27, 2026

Pull Request (PR) description

Convert RDSessionDeployment to class based resource.

  • Interface/Schema has changed but is forward compatible. WebAccessServer is now not a mandatory parameter to match the underlying cmdlet.
    Add required configuration and modules for class based resources.

This Pull Request (PR) fixes the following issues

Task list

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

This change is Reviewable

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Walkthrough

Converts RDSessionDeployment from the legacy function-based DSC resource to a class-based ResourceBase implementation, adds an RDSReason class and en-US localized strings, removes legacy module/schema/tests, adds a prefix import script and DscResource.Base dependency, updates examples and manifest export settings, and adds class-focused unit tests.

Changes

Cohort / File(s) Summary
Build & Manifest
RequiredModules.psd1, build.yaml, source/RemoteDesktopServicesDsc.psd1
Add DscResource.Base to required modules and build copy-only nested modules; set global prefix script in build; change CmdletsToExport from @() to '*'.
Prefix / Bootstrapping
source/prefix.ps1
Add prefix script to import DscResource.Base, import common helper modules, and initialize localized data.
Class sources
source/Classes/001.RDSReason.ps1, source/Classes/020.RDSessionDeployment.ps1
Add new RDSReason class and class-based RDSessionDeployment : ResourceBase with properties, Get/Set/Test, GetCurrentState, Modify, AddRole/RemoveRole, and AssertProperties.
Removed function-based resource
source/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.psm1, .../DSC_RDSessionDeployment.schema.mof, .../README.md, .../en-US/DSC_RDSessionDeployment.strings.psd1
Remove the legacy function-based DSC_RDSessionDeployment implementation, its MOF schema, README and old localization files.
Localization
source/en-US/RDSessionDeployment.strings.psd1
Add en-US localized strings for RDSessionDeployment messages and errors.
Examples
source/Examples/Resources/RDSessionDeployment/1-CreateSessionDeployment.ps1, .../1-RDSessionDeployment_Full.ps1
Adjust DSC import syntax/formatting in examples to match class-based usage.
Module root
source/RemoteDesktopServicesDsc.psm1
Remove placeholder comment content from root module file (left empty).
Tests — added
tests/Unit/Classes/RDSReason.Tests.ps1, tests/Unit/Classes/RDSessionDeployment.Tests.ps1
Add Pester unit tests for RDSReason and RDSessionDeployment covering construction, lifecycle methods, helpers, and assertions.
Tests — removed
tests/Unit/DSC_RDSessionDeployment.Tests.ps1
Remove legacy Pester suite that tested the function-based DSC resource.
Changelog
CHANGELOG.md
Document conversion of RDSessionDeployment to class-based resource and note WebAccessServer is no longer required.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: converting RDSessionDeployment to a class-based resource, which matches the primary objective of the PR.
Description check ✅ Passed The description is directly related to the changeset, explaining the conversion to class-based resource, forward compatibility, WebAccessServer parameter change, and required configuration/module additions.
Linked Issues check ✅ Passed The PR successfully implements the core objectives of issue #151: converts RDSessionDeployment to a class-based resource with forward compatibility and makes WebAccessServer optional to match the underlying cmdlet behavior.
Out of Scope Changes check ✅ Passed All changes are scope-aligned with issue #151: migration to class-based implementation, required module dependencies (DscResource.Base), build configuration updates, and test suite conversion are all necessary for the class-based resource migration.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97%. Comparing base (7587a76) to head (767458a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #152   +/-   ##
===================================
  Coverage    97%    97%           
===================================
  Files         9     10    +1     
  Lines       536    537    +1     
===================================
+ Hits        523    525    +2     
+ Misses       13     12    -1     
Files with missing lines Coverage Δ
source/Classes/020.RDSessionDeployment.ps1 100% <100%> (ø)
source/prefix.ps1 100% <100%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🧹 Nitpick comments (3)
source/prefix.ps1 (1)

4-9: Inconsistent path separators.

Line 4 uses backslash (Modules\DscResource.Common) while line 8 uses forward slash (Modules/RemoteDesktopServicesDsc.Common). While PowerShell handles both on Windows, consider using a consistent separator throughout for readability.

Suggested fix for consistency
 # Import nested, 'DscResource.Common' module
 $script:dscResourceCommonModulePath = Join-Path -Path $PSScriptRoot -ChildPath 'Modules\DscResource.Common'
 Import-Module -Name $script:dscResourceCommonModulePath

 # TODO: The goal would be to remove this, when no classes and public or private functions need it.
-$script:remoteDesktopServicesDscCommonModulePath = Join-Path -Path $PSScriptRoot -ChildPath 'Modules/RemoteDesktopServicesDsc.Common'
+$script:remoteDesktopServicesDscCommonModulePath = Join-Path -Path $PSScriptRoot -ChildPath 'Modules\RemoteDesktopServicesDsc.Common'
 Import-Module -Name $script:remoteDesktopServicesDscCommonModulePath
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@source/prefix.ps1` around lines 4 - 9, The two module path variables
($script:dscResourceCommonModulePath and
$script:remoteDesktopServicesDscCommonModulePath) use mixed path separators;
normalize them by using Join-Path consistently for both child paths (e.g.,
replace 'Modules/RemoteDesktopServicesDsc.Common' with
'Modules\RemoteDesktopServicesDsc.Common' or vice versa) so both Import-Module
calls are constructed with the same separator style and readability is
consistent; update the Join-Path childPath argument for the
remoteDesktopServicesDscCommonModulePath to match the pattern used for
dscResourceCommonModulePath.
tests/Unit/Classes/RDSReason.Tests.ps1 (2)

60-68: Missing Set-StrictMode -Version 1.0 in InModuleScope block.

Proposed fix
         It 'Should be able to set value in instance' {
             $script:mockRDSReasonInstance = InModuleScope -ScriptBlock {
+                Set-StrictMode -Version 1.0
+
                 $RDSReasonInstance = [RDSReason]::new()

                 $RDSReasonInstance.Code = 'RDSReason:RDSReason:Ensure'
                 $RDSReasonInstance.Phrase = 'The property Ensure should be "Present", but was "Absent"'

                 return $RDSReasonInstance
             }
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Classes/RDSReason.Tests.ps1` around lines 60 - 68, Add
Set-StrictMode -Version 1.0 at the start of the InModuleScope -ScriptBlock so
the test runs with strict mode enabled; specifically, inside the block that
constructs $RDSReasonInstance (the block using [RDSReason]::new() and assigning
.Code and .Phrase), insert the strict mode call before creating
$RDSReasonInstance to ensure undefined variables/typos are caught during the
RDSReasonInstance setup.

46-56: Missing Set-StrictMode -Version 1.0 in InModuleScope blocks.

As per coding guidelines, Set-StrictMode -Version 1.0 should be added immediately before invoking the tested function/class inside InModuleScope blocks.

Proposed fix
     Context 'When instantiating the class' {
         It 'Should not throw an error' {
             $script:mockRDSReasonInstance = InModuleScope -ScriptBlock {
+                Set-StrictMode -Version 1.0
+
                 [RDSReason]::new()
             }
         }

         It 'Should be of the correct type' {
+            InModuleScope -ScriptBlock {
+                Set-StrictMode -Version 1.0
+            }
+
             $mockRDSReasonInstance | Should -Not -BeNullOrEmpty
             $mockRDSReasonInstance.GetType().Name | Should -Be 'RDSReason'
         }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Classes/RDSReason.Tests.ps1` around lines 46 - 56, The
InModuleScope scriptblocks that instantiate the class must enable strict mode;
add a call to Set-StrictMode -Version 1.0 as the first statement inside each
InModuleScope -ScriptBlock before invoking the tested code (e.g., before
[RDSReason]::new() in the block that creates $script:mockRDSReasonInstance and
any other InModuleScope blocks in the RDSReason tests). Ensure the
Set-StrictMode call is placed inside the scriptblock so it applies to the code
under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 11: Update the changelog entry that currently reads "Converted to class
based resource" to use hyphenated wording "class-based" for correct grammar;
locate the line containing the phrase "Converted to class based resource [Issue
`#151`]" and replace "class based" with "class-based" so the entry reads
"Converted to class-based resource [Issue `#151`]".

In `@source/Classes/020.RDSessionDeployment.ps1`:
- Around line 40-42: The RDSessionDeployment constructor currently doesn't
initialize the $this.ExcludeDscProperties array; update the RDSessionDeployment
() : base ($PSScriptRoot) constructor to set $this.ExcludeDscProperties to an
empty array (or the required default values) so the property is always defined
per coding guidelines—locate the RDSessionDeployment constructor and add
initialization of $this.ExcludeDscProperties there.
- Line 21: The DSC class attribute `[DscResource()]` on the RDSessionDeployment
class is missing the RunAsCredential setting; update the attribute to include a
RunAsCredential value (e.g., change `[DscResource()]` to
`[DscResource(RunAsCredential = 'Optional')]` or `'Mandatory'` as required for
RDSessionDeployment) so the class-based resource declares its run-as credential
policy; locate the `[DscResource]` attribute above the RDSessionDeployment class
and replace it with the appropriate form.
- Around line 5-6: The .DESCRIPTION block for the DSC class resource
RDSessionDeployment is missing the required "## Requirements" and "## Known
issues" subsections; update the comment-based help in the .DESCRIPTION for the
RDSessionDeployment resource to add a "## Requirements" section listing
prerequisites (PowerShell/DSC versions, modules) and a "## Known issues" section
that includes a link to the repository's open issues page on GitHub and any
current caveats; ensure these headings are included in the same .DESCRIPTION
comment block so the DSC class-based resource follows the coding guidelines.

In `@source/en-US/RDSessionDeployment.strings.psd1`:
- Around line 4-6: Remove the literal single-quote characters that were included
in the localized values for RDSessionDeployment_OSRequirementNotMet,
RDSessionDeployment_AddingToDeployment, and
RDSessionDeployment_RemovingFromDeployment so the stored strings are plain
message text (e.g. The minimum OS requirement was not met. (RDSD0001)) and for
messages with placeholders (RDSessionDeployment_AddingToDeployment and
RDSessionDeployment_RemovingFromDeployment) ensure they use the
ConvertFrom-StringData placeholder syntax with {0}, {1} intact and no
surrounding quotes.

In `@source/RemoteDesktopServicesDsc.psd1`:
- Line 33: Replace the wildcard export by setting CmdletsToExport to an empty
array: update the manifest entry for CmdletsToExport (currently "'*'") to @() so
the module does not export any cmdlets; this should match the existing
FunctionsToExport (empty) and the nested module RemoteDesktopServicesDsc.Common
and follow DSC resource module best practices.

In `@tests/Unit/Classes/RDSessionDeployment.Tests.ps1`:
- Around line 303-313: The two test-case hashtables for Property =
'WebAccessServer' (one with Current = $null and the next with Current =
'webaccess.lan') are missing a separating comma; insert a comma after the
closing brace of the first hashtable so the array of hashtables is syntactically
valid (i.e., add a comma between the hashtable ending with Desired =
[System.String[]] @('webaccess1.lan', 'webaccess3.lan') and the following
hashtable).

In `@tests/Unit/Classes/RDSReason.Tests.ps1`:
- Line 59: The Context description string "When setting an reading values"
contains a typo; update the Context declaration in the test (the Context block
whose title is "When setting an reading values") to "When setting and reading
values" so the test description reads correctly.
- Around line 71-74: The test "It 'Should be able read the values from
instance'" mistakenly assigns to $mockRDSReasonInstance.Phrase instead of
asserting its value; replace the assignment ($mockRDSReasonInstance.Phrase =
'...') with an assertion that pipes the property into Should -Be with the
expected string (e.g., $mockRDSReasonInstance.Phrase | Should -Be 'The property
Ensure should be "Present", but was "Absent"') so the test actually verifies the
Phrase property.

---

Nitpick comments:
In `@source/prefix.ps1`:
- Around line 4-9: The two module path variables
($script:dscResourceCommonModulePath and
$script:remoteDesktopServicesDscCommonModulePath) use mixed path separators;
normalize them by using Join-Path consistently for both child paths (e.g.,
replace 'Modules/RemoteDesktopServicesDsc.Common' with
'Modules\RemoteDesktopServicesDsc.Common' or vice versa) so both Import-Module
calls are constructed with the same separator style and readability is
consistent; update the Join-Path childPath argument for the
remoteDesktopServicesDscCommonModulePath to match the pattern used for
dscResourceCommonModulePath.

In `@tests/Unit/Classes/RDSReason.Tests.ps1`:
- Around line 60-68: Add Set-StrictMode -Version 1.0 at the start of the
InModuleScope -ScriptBlock so the test runs with strict mode enabled;
specifically, inside the block that constructs $RDSReasonInstance (the block
using [RDSReason]::new() and assigning .Code and .Phrase), insert the strict
mode call before creating $RDSReasonInstance to ensure undefined variables/typos
are caught during the RDSReasonInstance setup.
- Around line 46-56: The InModuleScope scriptblocks that instantiate the class
must enable strict mode; add a call to Set-StrictMode -Version 1.0 as the first
statement inside each InModuleScope -ScriptBlock before invoking the tested code
(e.g., before [RDSReason]::new() in the block that creates
$script:mockRDSReasonInstance and any other InModuleScope blocks in the
RDSReason tests). Ensure the Set-StrictMode call is placed inside the
scriptblock so it applies to the code under test.
🪄 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: 81fdff2e-c4fc-469b-a9fe-11b7794d5286

📥 Commits

Reviewing files that changed from the base of the PR and between e1fffc5 and a610ce1.

📒 Files selected for processing (18)
  • CHANGELOG.md
  • RequiredModules.psd1
  • build.yaml
  • source/Classes/001.RDSReason.ps1
  • source/Classes/020.RDSessionDeployment.ps1
  • source/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.psm1
  • source/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.schema.mof
  • source/DSCResources/DSC_RDSessionDeployment/README.md
  • source/DSCResources/DSC_RDSessionDeployment/en-US/DSC_RDSessionDeployment.strings.psd1
  • source/Examples/Resources/RDSessionDeployment/1-CreateSessionDeployment.ps1
  • source/Examples/Resources/RDSessionDeployment/1-RDSessionDeployment_Full.ps1
  • source/RemoteDesktopServicesDsc.psd1
  • source/RemoteDesktopServicesDsc.psm1
  • source/en-US/RDSessionDeployment.strings.psd1
  • source/prefix.ps1
  • tests/Unit/Classes/RDSReason.Tests.ps1
  • tests/Unit/Classes/RDSessionDeployment.Tests.ps1
  • tests/Unit/DSC_RDSessionDeployment.Tests.ps1
💤 Files with no reviewable changes (6)
  • source/DSCResources/DSC_RDSessionDeployment/README.md
  • source/RemoteDesktopServicesDsc.psm1
  • source/DSCResources/DSC_RDSessionDeployment/en-US/DSC_RDSessionDeployment.strings.psd1
  • source/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.schema.mof
  • tests/Unit/DSC_RDSessionDeployment.Tests.ps1
  • source/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.psm1

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
source/Classes/020.RDSessionDeployment.ps1 (1)

76-80: Consider selecting single value for ConnectionBroker defensively.

Line 78 returns ConnectionBroker from a .Where() clause without ensuring a single value. While typically only one Connection Broker exists per deployment, the .Server property could return an array if multiple servers have the RDS-CONNECTION-BROKER role, causing a type mismatch with the [System.String] property.

For consistency with line 94 in Modify() (which uses Select-Object -First 1 for WebAccessServer), consider applying the same pattern here.

Proposed fix
         return @{
             SessionHost      = [System.String[]] ($deployed.Where({ $_.Roles -contains 'RDS-RD-SERVER' })).Server
-            ConnectionBroker = ($deployed.Where({ $_.Roles -contains 'RDS-CONNECTION-BROKER' })).Server
+            ConnectionBroker = ($deployed.Where({ $_.Roles -contains 'RDS-CONNECTION-BROKER' })).Server | Select-Object -First 1
             WebAccessServer  = [System.String[]] ($deployed.Where({ $_.Roles -contains 'RDS-WEB-ACCESS' })).Server
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@source/Classes/020.RDSessionDeployment.ps1` around lines 76 - 80, The
ConnectionBroker return value is taken from ($deployed.Where({ $_.Roles
-contains 'RDS-CONNECTION-BROKER' })).Server which can produce an array if
multiple servers match; change the construction to defensively select a single
server (e.g., use Select-Object -First 1 or index [0] and cast to
[System.String]) so ConnectionBroker is always a single string like
WebAccessServer is in Modify(); update the return hash's ConnectionBroker
expression accordingly, referencing the existing deployed variable, Roles,
Server property and the Modify() pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@source/Classes/020.RDSessionDeployment.ps1`:
- Around line 76-80: The ConnectionBroker return value is taken from
($deployed.Where({ $_.Roles -contains 'RDS-CONNECTION-BROKER' })).Server which
can produce an array if multiple servers match; change the construction to
defensively select a single server (e.g., use Select-Object -First 1 or index
[0] and cast to [System.String]) so ConnectionBroker is always a single string
like WebAccessServer is in Modify(); update the return hash's ConnectionBroker
expression accordingly, referencing the existing deployed variable, Roles,
Server property and the Modify() pattern.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: de81ddde-620a-4a0e-8764-1e219bbfa692

📥 Commits

Reviewing files that changed from the base of the PR and between a610ce1 and 7a5341c.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • source/Classes/020.RDSessionDeployment.ps1
  • source/en-US/RDSessionDeployment.strings.psd1
  • tests/Unit/Classes/RDSReason.Tests.ps1
  • tests/Unit/Classes/RDSessionDeployment.Tests.ps1
✅ Files skipped from review due to trivial changes (3)
  • CHANGELOG.md
  • source/en-US/RDSessionDeployment.strings.psd1
  • tests/Unit/Classes/RDSReason.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/Unit/Classes/RDSessionDeployment.Tests.ps1

@dan-hughes dan-hughes requested a review from raandree March 27, 2026 12:09
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
source/prefix.ps1 (1)

7-7: Track TODO with an issue reference.

Line 7 has a TODO in bootstrap code. Please attach an issue ID (or milestone) in the comment so this cleanup doesn’t become orphaned.

Do you want me to draft the issue text for this cleanup item?

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@source/prefix.ps1` at line 7, Update the TODO comment in source/prefix.ps1
(the line containing "TODO: The goal would be to remove this, when no classes
and public or private functions need it.") to include a tracker reference (issue
ID or milestone) and short issue link or number (e.g., "#1234" or
"issue:repo#1234"); ensure the comment briefly states the cleanup task and the
tracker reference so the TODO is not orphaned.
🤖 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/prefix.ps1`:
- Around line 5-9: The Import-Module calls that load the nested modules
($script:dscResourceCommonModulePath and
$script:remoteDesktopServicesDscCommonModulePath) should fail fast on errors;
update the two Import-Module invocations in prefix.ps1 to include the
-ErrorAction Stop parameter so that any import failure throws and prevents
partial/quiet initialization of the module.
- Line 1: The build fails because source/prefix.ps1 contains "using module
.\Modules\DscResource.Base" which requires the DscResource.Base module to exist
at parse-time; fix this by reordering the build pipeline in build.yaml so nested
modules are copied before parsing the main module — specifically ensure the task
sequence runs Build_NestedModules_ModuleBuilder before
Build_Module_ModuleBuilder (e.g., in the build steps run Clean,
Build_NestedModules_ModuleBuilder, Build_Module_ModuleBuilder,
Create_changelog_release_output) so the Modules/DscResource.Base is present when
the module parser runs.

---

Nitpick comments:
In `@source/prefix.ps1`:
- Line 7: Update the TODO comment in source/prefix.ps1 (the line containing
"TODO: The goal would be to remove this, when no classes and public or private
functions need it.") to include a tracker reference (issue ID or milestone) and
short issue link or number (e.g., "#1234" or "issue:repo#1234"); ensure the
comment briefly states the cleanup task and the tracker reference so the TODO is
not orphaned.
🪄 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: 28e36e7d-69d5-4106-b2a7-8fdafd2af738

📥 Commits

Reviewing files that changed from the base of the PR and between 7a5341c and 767458a.

📒 Files selected for processing (1)
  • source/prefix.ps1

@@ -0,0 +1,11 @@
using module .\Modules\DscResource.Base
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "1) Verify referenced files exist"
fd -a "^prefix\.ps1$" source
fd -a "^build\.yaml$|^RequiredModules\.psd1$"

echo "2) Show parse-time using-module line"
nl -ba source/prefix.ps1 | sed -n '1,20p'

echo "3) Confirm DscResource.Base provisioning config"
rg -n "DscResource\\.Base|Prefix:|Modules/DscResource\\.Base|copy" build.yaml RequiredModules.psd1

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 394


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 1. Read source/prefix.ps1 ==="
cat -n source/prefix.ps1

echo -e "\n=== 2. Check RequiredModules.psd1 for DscResource.Base ==="
grep -A 5 -B 5 "DscResource\.Base" RequiredModules.psd1 || echo "Not found (may not be declared yet)"

echo -e "\n=== 3. Search build.yaml for DscResource.Base provisioning ==="
cat build.yaml | grep -A 10 -B 5 "DscResource\.Base\|Prefix\|Modules" | head -100

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 3803


🏁 Script executed:

#!/bin/bash

echo "=== Checking build task sequence and standard ordering ==="
grep -A 10 "Tasks:" build.yaml | head -20

echo -e "\n=== Verify current build sequence ==="
grep -A 5 "^  build:" build.yaml

echo -e "\n=== Check if module bootstrap is documented ==="
grep -B 5 -A 5 "build.ps1\|./build.ps1" build.yaml | head -20

echo -e "\n=== Confirm source/Modules directory structure ==="
fd -t d "^Modules$" source

echo -e "\n=== Check for any module link or copy logic ==="
rg -n "source.*Modules|Modules.*source" build.yaml

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 964


🏁 Script executed:

#!/bin/bash

echo "=== Check if DscResource.Base currently exists in source/Modules ==="
ls -la source/Modules/ 2>/dev/null || echo "Directory exists but is empty or doesn't contain DscResource.Base yet"

echo -e "\n=== Verify the build.yaml task order is the issue ==="
echo "Current sequence:"
grep -A 4 "^  build:" build.yaml

echo -e "\nStandard ModuleBuilder pattern requires:"
echo "1. Build_NestedModules_ModuleBuilder (copy dependencies)"
echo "2. Build_Module_ModuleBuilder (parse files with 'using module')"

echo -e "\n=== Check ModuleBuilder/Sampler documentation in project ==="
fd -a "README" . -t f | head -5

echo -e "\n=== Verify this is a configuration issue, not a workaround ==="
rg -B 2 -A 2 "Build_Module_ModuleBuilder|Build_NestedModules" build.yaml | head -20

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 1304


Fix build task sequencing in build.yaml to copy nested modules before parsing the main module.

The using module .\Modules\DscResource.Base on line 1 requires parse-time module resolution, but DscResource.Base is not yet copied to source/Modules/ because the current task order runs Build_Module_ModuleBuilder before Build_NestedModules_ModuleBuilder. This causes a parse failure.

Reorder the build tasks to copy nested modules first:

Swap task sequence in build.yaml
  build:
    - Clean
    - Build_NestedModules_ModuleBuilder
    - Build_Module_ModuleBuilder
    - Create_changelog_release_output
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)

[error] 1-1: Could not find the module '.\Modules\DscResource.Base'.

(ModuleNotFoundDuringParse)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@source/prefix.ps1` at line 1, The build fails because source/prefix.ps1
contains "using module .\Modules\DscResource.Base" which requires the
DscResource.Base module to exist at parse-time; fix this by reordering the build
pipeline in build.yaml so nested modules are copied before parsing the main
module — specifically ensure the task sequence runs
Build_NestedModules_ModuleBuilder before Build_Module_ModuleBuilder (e.g., in
the build steps run Clean, Build_NestedModules_ModuleBuilder,
Build_Module_ModuleBuilder, Create_changelog_release_output) so the
Modules/DscResource.Base is present when the module parser runs.

Comment on lines +5 to +9
Import-Module -Name $script:dscResourceCommonModulePath

# TODO: The goal would be to remove this, when no classes and public or private functions need it.
$script:remoteDesktopServicesDscCommonModulePath = Join-Path -Path $PSScriptRoot -ChildPath 'Modules\RemoteDesktopServicesDsc.Common'
Import-Module -Name $script:remoteDesktopServicesDscCommonModulePath
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Import-Module calls in source/prefix.ps1:"
rg -nP --type=ps1 '\bImport-Module\b' source/prefix.ps1

echo "Import-Module calls with -ErrorAction Stop:"
rg -nP --type=ps1 '\bImport-Module\b.*-ErrorAction\s+Stop' source/prefix.ps1 || true

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 167


🏁 Script executed:

cat source/prefix.ps1

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 702


🏁 Script executed:

cat source/suffix.ps1

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 130


🏁 Script executed:

fd -e psd1 'RemoteDesktopServicesDsc' source/ | head -5

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 350


🏁 Script executed:

cat source/RemoteDesktopServicesDsc.psd1 | head -100

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 2459


🏁 Script executed:

head -30 source/RemoteDesktopServicesDsc.psm1

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 176


🏁 Script executed:

fd -name 'build.yaml' -o -name 'build.yml' | head -3

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 311


🏁 Script executed:

find . -name 'build.yaml' -o -name 'build.yml' 2>/dev/null | head -3

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 93


🏁 Script executed:

cat build.yaml | grep -A 20 -B 5 'prefix\|suffix' | head -60

Repository: dsccommunity/RemoteDesktopServicesDsc

Length of output: 1209


Add -ErrorAction Stop to nested module imports in prefix.ps1 to fail fast on import failures.

Lines 5 and 9 call Import-Module for nested modules without -ErrorAction Stop. If either import fails, the module initializes in a partially loaded state instead of failing fast. These are critical initialization imports and must not be silently skipped.

Proposed fix
 $script:dscResourceCommonModulePath = Join-Path -Path $PSScriptRoot -ChildPath 'Modules\DscResource.Common'
-Import-Module -Name $script:dscResourceCommonModulePath
+Import-Module -Name $script:dscResourceCommonModulePath -ErrorAction Stop

 # TODO: The goal would be to remove this, when no classes and public or private functions need it.
 $script:remoteDesktopServicesDscCommonModulePath = Join-Path -Path $PSScriptRoot -ChildPath 'Modules\RemoteDesktopServicesDsc.Common'
-Import-Module -Name $script:remoteDesktopServicesDscCommonModulePath
+Import-Module -Name $script:remoteDesktopServicesDscCommonModulePath -ErrorAction Stop
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@source/prefix.ps1` around lines 5 - 9, The Import-Module calls that load the
nested modules ($script:dscResourceCommonModulePath and
$script:remoteDesktopServicesDscCommonModulePath) should fail fast on errors;
update the two Import-Module invocations in prefix.ps1 to include the
-ErrorAction Stop parameter so that any import failure throws and prevents
partial/quiet initialization of the module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RDSessionDeployment - Convert to Class-Based

1 participant