ADDomainController: Allow UseExistingAccount to work when using pre-staged computer account #748
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
WalkthroughAdds a public read-only Enabled property to ADReadOnlyDomainControllerAccount; moves OS/domain-controller presence checks into ADDomainController (uses Test-IsDomainController), rehomes domain-controller lookup behavior from ActiveDirectoryDsc.Common into ADDomainController, updates localization/messages and parameter docs, and adapts unit tests to the new control flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Node
participant ADDC as ADDomainController
participant Common as ActiveDirectoryDsc.Common
participant AD as Active Directory
Note over ADDC: Get-TargetResource (updated)
Node->>ADDC: Invoke Get-TargetResource(...)
ADDC->>ADDC: Test-IsDomainController
alt Node is DC
ADDC->>Common: Get-DomainControllerObject(domain, computer)
Common->>AD: Query DC object
AD-->>Common: DC object
Common-->>ADDC: DC object
ADDC->>AD: Get-DomainObject / PRP / ManagedBy / registry/service checks
AD-->>ADDC: Domain/DC details
ADDC-->>Node: return target-state hashtable (includes DC data)
else Node not DC
ADDC->>AD: Resolve domain object (non-DC path)
AD-->>ADDC: Domain object/info
ADDC-->>Node: return target-state hashtable (minimal/defaults)
end
sequenceDiagram
autonumber
participant Node
participant RODC as ADReadOnlyDomainControllerAccount
participant AD as Active Directory
Note over RODC: Get-TargetResource (Enabled property)
Node->>RODC: Invoke Get-TargetResource(...)
RODC->>AD: Query DC object for node
AD-->>RODC: DC object (includes IsReadOnly, Enabled)
alt DC object is read-only
RODC-->>Node: Return {..., Enabled = DC.Enabled}
else Not read-only
RODC-->>Node: Return {..., Enabled = $false}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (5 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 (1)
12-47: Add OUTPUTS section to comment-based help.The comment-based help for
Get-TargetResourceis missing an OUTPUTS section. Per coding guidelines, all functions must document their return types.Apply this diff to add the OUTPUTS section:
.PARAMETER InstallDns Specifies if the DNS Server service should be installed and configured on the read only domain controller. If this is not set the default value of the parameter InstallDns of the cmdlet Add-ADDSReadOnlyDomainControllerAccount is used. The parameter `InstallDns` is only used during the provisioning of a read only domain controller. The parameter cannot be used to install or uninstall the DNS server on an already provisioned read only domain controller. Not used in Get-TargetResource. + .OUTPUTS + System.Collections.Hashtable + + Returns a hashtable containing the current state of the read only domain controller account. + .NOTES Used Functions: Name | Module
🧹 Nitpick comments (3)
source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 (3)
127-127: Convert Boolean to String for MOF schema compatibility.The schema defines
Enabledas String (line 14 in schema.mof), but$domainControllerObject.Enabledlikely returns a Boolean. While PowerShell auto-converts Boolean to String, explicit conversion ensures schema compliance and clarity.Apply this diff to explicitly convert to String:
- Enabled = $domainControllerObject.Enabled + Enabled = $domainControllerObject.Enabled.ToString()
146-146: Convert Boolean to String for MOF schema compatibility.The schema defines
Enabledas String (line 14 in schema.mof), but$falseis a Boolean. Apply explicit conversion to ensure schema compliance.Apply this diff to explicitly convert to String:
- Enabled = $false + Enabled = $false.ToString()
12-47: Add .OUTPUTS section to comment-based help.The comment-based help is missing an
.OUTPUTSsection documenting the returned hashtable and its properties (including the newly addedEnabledproperty).Add the following section to the comment-based help:
.NOTES Used Functions: Name | Module ------------------------------------------------|-------------------------- Get-ADDomain | ActiveDirectory Get-ADDomainControllerPasswordReplicationPolicy | ActiveDirectory Get-DomainControllerObject | ActiveDirectoryDsc.Common Assert-Module | DscResource.Common New-ObjectNotFoundException | DscResource.Common + + .OUTPUTS + System.Collections.Hashtable + + Returns a hashtable containing the current state of the Read Only Domain Controller Account with properties: AllowPasswordReplicationAccountName, Credential, DelegatedAdministratorAccountName, DenyPasswordReplicationAccountName, DomainControllerAccountName, DomainName, Enabled, Ensure, InstallDns, IsGlobalCatalog, and SiteName. #>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1(5 hunks)source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.schema.mof(1 hunks)source/DSCResources/MSFT_ADDomainController/en-US/MSFT_ADDomainController.strings.psd1(1 hunks)source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1(2 hunks)source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof(1 hunks)source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1(2 hunks)source/Modules/ActiveDirectoryDsc.Common/README.md(1 hunks)source/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1(1 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:
source/Modules/ActiveDirectoryDsc.Common/README.mdsource/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mofsource/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1source/DSCResources/MSFT_ADDomainController/en-US/MSFT_ADDomainController.strings.psd1source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.schema.mofsource/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1CHANGELOG.mdsource/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.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:
source/Modules/ActiveDirectoryDsc.Common/README.mdCHANGELOG.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/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1source/DSCResources/MSFT_ADDomainController/en-US/MSFT_ADDomainController.strings.psd1source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.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_ADDomainController/MSFT_ADDomainController.psm1source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1
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
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Test Unit)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Test HQRM)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Build Package Module)
🔇 Additional comments (12)
source/Modules/ActiveDirectoryDsc.Common/README.md (1)
65-65: Doc tweak looks goodUpdated summary matches the streamlined function behavior.
source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 (2)
1286-1289: LGTM!The documentation wording is now more precise and aligned with Microsoft Active Directory terminology.
1353-1354: LGTM!The error message is now more descriptive, including both computer name and domain for better diagnostics. The validation logic has been appropriately moved to the calling code in ADDomainController resource.
source/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1 (1)
4-4: LGTM!The new localization key provides clearer error messaging with both computer and domain context. The message is properly formatted with parameter placeholders.
source/DSCResources/MSFT_ADDomainController/en-US/MSFT_ADDomainController.strings.psd1 (1)
4-7: LGTM!The localization updates support the new control flow effectively:
- IsDomainController provides initial OS-level detection feedback
- WasExpectingDomainController handles the edge case where OS indicates DC but object retrieval fails
- FoundDomainControllerObject confirms successful retrieval with domain context
The messages are clear and provide appropriate diagnostic information at each stage of the verification process.
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.schema.mof (1)
10-19: Usage constraints documented but not enforced.The schema correctly documents that these parameters should not be used with
UseExistingAccount. However, as noted in the PR description, no hard enforcement is implemented. Consider whether parameter validation rules should be added to prevent invalid parameter combinations at runtime.Do you want me to generate validation logic that enforces these constraints in the
*-TargetResourcefunctions?source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (4)
106-114: LGTM!The password replication policy and service configuration retrieval logic is correct:
- Uses appropriate AD cmdlets for RODC policies
- Registry paths are correct for NTDS and NETLOGON services
- DNS service check properly handles service not found with
ErrorAction SilentlyContinue
136-139: Appropriate error handling for edge case.The error path correctly handles the scenario where:
- The OS indicates the node is a domain controller (Test-IsDomainController returns
$true)- But Get-DomainControllerObject returns
$nullor failsThis could occur during initial promotion or in error conditions. The error message clearly describes the situation.
195-233: LGTM!The parameter descriptions correctly document the constraint that these parameters should not be used with
UseExistingAccount. This is consistent with the schema.mof documentation and helps users understand the limitation when working with pre-staged RODC accounts.
681-719: LGTM!The parameter descriptions are consistently documented across all three functions (Get/Set/Test-TargetResource) and match the schema.mof documentation. This provides clear guidance to users about parameter usage constraints with pre-staged accounts.
source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 (2)
127-127: LGTM!The addition of the
Enabledproperty correctly surfaces the read-only domain controller account's enabled state from the AD object.
146-146: LGTM!The fallback value of
$falseforEnabledis appropriate when the account is not a read-only domain controller.
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Show resolved
Hide resolved
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Show resolved
Hide resolved
...ces/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…T_ADReadOnlyDomainControllerAccount.schema.mof Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
12-13: Consider refining wording for clarity.The phrase "stating whether" could be replaced with "indicating whether" for more idiomatic English.
Apply this diff:
- ADReadOnlyDomainControllerAccount - - Added read-only value Enabled stating whether a pre-staged account is Enabled or Disabled (Unoccupied). + - Added read-only value Enabled indicating whether a pre-staged account is Enabled or Disabled (Unoccupied).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ 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_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof
**/*.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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Test Unit)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Test HQRM)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Build Package Module)
🔇 Additional comments (2)
source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof (1)
14-14: LGTM! Type mismatch resolved.The
Enabledproperty is now correctly declared asBoolean, matching the implementation that returns Boolean values fromGet-TargetResource.CHANGELOG.md (1)
19-24: LGTM! Fixed section properly formatted.The Fixed section entries correctly describe the changes with appropriate detail and issue references. The blank line after the heading is present, resolving the previous markdown lint issue.
Borgquite
changed the title
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 (3)
CHANGELOG.md(1 hunks)source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof(1 hunks)tests/Unit/MSFT_ADDomainController.Tests.ps1(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.schema.mof
🧰 Additional context used
📓 Path-based instructions (6)
**
⚙️ 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.mdtests/Unit/MSFT_ADDomainController.Tests.ps1
**/*.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
tests/[Uu]nit/**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
tests/[Uu]nit/**/*.[Tt]ests.ps1: # Unit Tests Guidelines
- Test with localized strings: Use
InModuleScope -ScriptBlock { $script:localizedData.Key }- Mock files: Use
$TestDrivevariable (path to the test drive)- All public commands require parameter set validation tests
- After modifying classes, always run tests in new session (for changes to take effect)
Test Setup Requirements
Use this exact setup block before
Describe:[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param () BeforeDiscovery { try { if (-not (Get-Module -Name 'DscResource.Test')) { # 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 have 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 noop" first.' } } BeforeAll { $script:moduleName = '{MyModuleName}' Import-Module -Name $script:moduleName -Force -ErrorAction 'Stop' $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Mock:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Should:ModuleName'] = $script:moduleName } AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleNam...
Files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
{**/*.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:
tests/Unit/MSFT_ADDomainController.Tests.ps1
**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
**/*.[Tt]ests.ps1: # Tests GuidelinesCore Requirements
- All public commands, private functions and classes must have unit tests
- All public commands and class-based resources must have integration tests
- Use Pester v5 syntax only
- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test verbose messages, debug messages or parameter binding behavior
- Pass all mandatory parameters to avoid prompts
Requirements
- Inside
Itblocks, assign unused return objects to$null(unless part of pipeline)- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cycles
Naming
- One
Describeblock per file matching the tested entity nameContextdescriptions start with 'When'Itdescriptions start with 'Should', must not contain 'when'- Mock variables prefix: 'mock'
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- Each class method = separate
Contextblock- Each scenario = separate
Contextblock- Use nested
Contextblocks for complex scenarios- Mocking in
BeforeAll(BeforeEachonly when required)- Setup/teardown in
BeforeAll,BeforeEach/AfterAll,AfterEachclose to usageSyntax Rules
- PascalCase:
Describe,Context,It,Should,BeforeAll,BeforeEach,AfterAll,AfterEach- Use
-BeTrue/-BeFalsenever-Be $true/-Be $false- Never use
Assert-MockCalled, useShould -Invokeinstead- No
Should -Not -Throw- invoke commands directly- Never add an empty
-MockWithblock- Omit
-MockWithwhen returning$null- Set
$PSDefaultParameterValuesforMock:ModuleName,Should:ModuleName,InModuleScope:ModuleName- Omit
-ModuleNameparameter on Pester commands- Never use
Mockinside `InModuleSc...
Files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
🧠 Learnings (2)
📚 Learning: 2025-09-12T13:21:31.054Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-pester.instructions.md:0-0
Timestamp: 2025-09-12T13:21:31.054Z
Learning: Applies to **/*.[Tt]ests.ps1 : Perform mocking in BeforeAll (use BeforeEach only when required)
Applied to files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
📚 Learning: 2025-09-12T13:21:31.054Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-pester.instructions.md:0-0
Timestamp: 2025-09-12T13:21:31.054Z
Learning: Applies to **/*.[Tt]ests.ps1 : Set $PSDefaultParameterValues for Mock:ModuleName, Should:ModuleName, and InModuleScope:ModuleName
Applied to files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
🔇 Additional comments (2)
tests/Unit/MSFT_ADDomainController.Tests.ps1 (2)
66-66: LGTM!The Test-IsDomainController mocks and assertions correctly verify domain controller state checks in scenarios where the node is a domain controller.
Also applies to: 88-88, 96-96, 151-151, 196-196, 279-279
293-293: LGTM!The context name change improves clarity by describing the actual state ("is not") rather than the desired state ("should not be").
Borgquite
commented
Oct 2, 2025
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Show resolved
Hide resolved
Borgquite
commented
Oct 2, 2025
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1 (1)
24-25: Align dependency error hint with guidelines ('noop' task).Use '-Tasks noop' in the guidance message for consistency.
As per coding guidelines
- throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks build" first.' + throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks noop" first.'tests/Unit/MSFT_ADDomainController.Tests.ps1 (1)
24-25: Align dependency error hint with guidelines ('noop' task).Use '-Tasks noop' in the guidance message for consistency.
As per coding guidelines
- throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks build" first.' + throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks noop" first.'source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (1)
96-107: Add defensive null checks for nested AD lookups.Guard ComputerObjectDN and the returned computer object before accessing ManagedBy; keeps behavior robust in edge AD states.
Apply:
- $domainControllerComputerObject = $domainControllerObject.ComputerObjectDN | - Get-ADComputer -Properties ManagedBy -Credential $Credential - if ($domainControllerComputerObject.ManagedBy) + if ($domainControllerObject.ComputerObjectDN) { - $domainControllerManagedByObject = $domainControllerComputerObject.ManagedBy | - Get-ADObject -Properties objectSid -Credential $Credential + $domainControllerComputerObject = $domainControllerObject.ComputerObjectDN | + Get-ADComputer -Properties ManagedBy -Credential $Credential + + if ($domainControllerComputerObject -and $domainControllerComputerObject.ManagedBy) + { + $domainControllerManagedByObject = $domainControllerComputerObject.ManagedBy | + Get-ADObject -Properties objectSid -Credential $Credential + } - if ($domainControllerManagedByObject.objectSid) + if ($domainControllerManagedByObject -and $domainControllerManagedByObject.objectSid) { $delegateAdministratorAccountName = Resolve-SamAccountName -ObjectSid $domainControllerManagedByObject.objectSid } }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1(6 hunks)tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1(2 hunks)tests/Unit/MSFT_ADDomainController.Tests.ps1(7 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:
tests/Unit/MSFT_ADDomainController.Tests.ps1source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1
tests/[Uu]nit/**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
tests/[Uu]nit/**/*.[Tt]ests.ps1: # Unit Tests Guidelines
- Test with localized strings: Use
InModuleScope -ScriptBlock { $script:localizedData.Key }- Mock files: Use
$TestDrivevariable (path to the test drive)- All public commands require parameter set validation tests
- After modifying classes, always run tests in new session (for changes to take effect)
Test Setup Requirements
Use this exact setup block before
Describe:[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param () BeforeDiscovery { try { if (-not (Get-Module -Name 'DscResource.Test')) { # 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 have 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 noop" first.' } } BeforeAll { $script:moduleName = '{MyModuleName}' Import-Module -Name $script:moduleName -Force -ErrorAction 'Stop' $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Mock:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Should:ModuleName'] = $script:moduleName } AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleNam...
Files:
tests/Unit/MSFT_ADDomainController.Tests.ps1tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1
{**/*.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:
tests/Unit/MSFT_ADDomainController.Tests.ps1source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1
**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
**/*.[Tt]ests.ps1: # Tests GuidelinesCore Requirements
- All public commands, private functions and classes must have unit tests
- All public commands and class-based resources must have integration tests
- Use Pester v5 syntax only
- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test verbose messages, debug messages or parameter binding behavior
- Pass all mandatory parameters to avoid prompts
Requirements
- Inside
Itblocks, assign unused return objects to$null(unless part of pipeline)- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cycles
Naming
- One
Describeblock per file matching the tested entity nameContextdescriptions start with 'When'Itdescriptions start with 'Should', must not contain 'when'- Mock variables prefix: 'mock'
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- Each class method = separate
Contextblock- Each scenario = separate
Contextblock- Use nested
Contextblocks for complex scenarios- Mocking in
BeforeAll(BeforeEachonly when required)- Setup/teardown in
BeforeAll,BeforeEach/AfterAll,AfterEachclose to usageSyntax Rules
- PascalCase:
Describe,Context,It,Should,BeforeAll,BeforeEach,AfterAll,AfterEach- Use
-BeTrue/-BeFalsenever-Be $true/-Be $false- Never use
Assert-MockCalled, useShould -Invokeinstead- No
Should -Not -Throw- invoke commands directly- Never add an empty
-MockWithblock- Omit
-MockWithwhen returning$null- Set
$PSDefaultParameterValuesforMock:ModuleName,Should:ModuleName,InModuleScope:ModuleName- Omit
-ModuleNameparameter on Pester commands- Never use
Mockinside `InModuleSc...
Files:
tests/Unit/MSFT_ADDomainController.Tests.ps1tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1
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_ADDomainController/MSFT_ADDomainController.psm1
🧠 Learnings (3)
📚 Learning: 2025-09-12T13:21:31.054Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-pester.instructions.md:0-0
Timestamp: 2025-09-12T13:21:31.054Z
Learning: Applies to **/*.[Tt]ests.ps1 : Perform mocking in BeforeAll (use BeforeEach only when required)
Applied to files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
📚 Learning: 2025-09-12T13:21:31.054Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-pester.instructions.md:0-0
Timestamp: 2025-09-12T13:21:31.054Z
Learning: Applies to **/*.[Tt]ests.ps1 : Set $PSDefaultParameterValues for Mock:ModuleName, Should:ModuleName, and InModuleScope:ModuleName
Applied to files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
📚 Learning: 2025-08-16T13:35:08.323Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:0-0
Timestamp: 2025-08-16T13:35:08.323Z
Learning: In PowerShell, users expect to receive $null when no objects are found or when a non-terminating error occurs, rather than empty arrays. This is normal PowerShell behavior and should be maintained in DSC commands.
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Build Package Module)
🔇 Additional comments (3)
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1 (1)
1118-1138: LGTM: streamlined DC-object tests and credential path.Contexts validate found-object and credential usage cleanly; mocks and invocation assertions are correct.
Also applies to: 1140-1166
tests/Unit/MSFT_ADDomainController.Tests.ps1 (1)
66-67: LGTM: switched to OS-level DC check with correct invocation counts.Mocks and Should -Invoke counts match the new control flow; skipping AD lookups when not a DC is correct.
Also applies to: 88-89, 96-98, 151-152, 196-197, 279-280, 295-296, 328-331
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (1)
68-75: LGTM: OS-level DC check avoids pre-staged RODC false positives.This correctly gates AD lookups and resolves issue #747.
Borgquite
commented
Oct 2, 2025
There was a problem hiding this comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Show resolved
Hide resolved
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 (3)
CHANGELOG.md(1 hunks)source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1(6 hunks)tests/Unit/MSFT_ADDomainController.Tests.ps1(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 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:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1tests/Unit/MSFT_ADDomainController.Tests.ps1
{**/*.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_ADDomainController/MSFT_ADDomainController.psm1tests/Unit/MSFT_ADDomainController.Tests.ps1
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_ADDomainController/MSFT_ADDomainController.psm1
tests/[Uu]nit/**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
tests/[Uu]nit/**/*.[Tt]ests.ps1: # Unit Tests Guidelines
- Test with localized strings: Use
InModuleScope -ScriptBlock { $script:localizedData.Key }- Mock files: Use
$TestDrivevariable (path to the test drive)- All public commands require parameter set validation tests
- After modifying classes, always run tests in new session (for changes to take effect)
Test Setup Requirements
Use this exact setup block before
Describe:[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param () BeforeDiscovery { try { if (-not (Get-Module -Name 'DscResource.Test')) { # 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 have 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 noop" first.' } } BeforeAll { $script:moduleName = '{MyModuleName}' Import-Module -Name $script:moduleName -Force -ErrorAction 'Stop' $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Mock:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Should:ModuleName'] = $script:moduleName } AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleNam...
Files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
**/*.[Tt]ests.ps1: # Tests GuidelinesCore Requirements
- All public commands, private functions and classes must have unit tests
- All public commands and class-based resources must have integration tests
- Use Pester v5 syntax only
- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test verbose messages, debug messages or parameter binding behavior
- Pass all mandatory parameters to avoid prompts
Requirements
- Inside
Itblocks, assign unused return objects to$null(unless part of pipeline)- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cycles
Naming
- One
Describeblock per file matching the tested entity nameContextdescriptions start with 'When'Itdescriptions start with 'Should', must not contain 'when'- Mock variables prefix: 'mock'
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- Each class method = separate
Contextblock- Each scenario = separate
Contextblock- Use nested
Contextblocks for complex scenarios- Mocking in
BeforeAll(BeforeEachonly when required)- Setup/teardown in
BeforeAll,BeforeEach/AfterAll,AfterEachclose to usageSyntax Rules
- PascalCase:
Describe,Context,It,Should,BeforeAll,BeforeEach,AfterAll,AfterEach- Use
-BeTrue/-BeFalsenever-Be $true/-Be $false- Never use
Assert-MockCalled, useShould -Invokeinstead- No
Should -Not -Throw- invoke commands directly- Never add an empty
-MockWithblock- Omit
-MockWithwhen returning$null- Set
$PSDefaultParameterValuesforMock:ModuleName,Should:ModuleName,InModuleScope:ModuleName- Omit
-ModuleNameparameter on Pester commands- Never use
Mockinside `InModuleSc...
Files:
tests/Unit/MSFT_ADDomainController.Tests.ps1
🧠 Learnings (1)
📚 Learning: 2025-08-16T13:35:08.323Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:0-0
Timestamp: 2025-08-16T13:35:08.323Z
Learning: In PowerShell, users expect to receive $null when no objects are found or when a non-terminating error occurs, rather than empty arrays. This is normal PowerShell behavior and should be maintained in DSC commands.
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Build Package Module)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
===================================
Coverage 99% 99%
===================================
Files 25 25
Lines 3647 3650 +3
===================================
+ Hits 3620 3623 +3
Misses 27 27
🚀 New features to boost your workflow:
|
Borgquite
commented
Oct 2, 2025
There was a problem hiding this comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (1)
68-73: Format IsDomainController verbose with both placeholdersString likely expects computer and domain; currently only computer is supplied. Pass DomainName too in both places.
As per coding guidelines
Apply this diff:
- Write-Verbose -Message ($script:localizedData.IsDomainController -f $env:COMPUTERNAME) + Write-Verbose -Message ($script:localizedData.IsDomainController -f $env:COMPUTERNAME, $DomainName)- Write-Verbose -Message ($script:localizedData.IsDomainController -f $env:COMPUTERNAME) + Write-Verbose -Message ($script:localizedData.IsDomainController -f $env:COMPUTERNAME, $DomainName)Run to confirm placeholder count in the localized string:
#!/bin/bash # Find IsDomainController string and show its value to verify placeholder count. rg -n "IsDomainController" -g "source/**/en-US/*.strings.psd1" -C2Also applies to: 469-470
🧹 Nitpick comments (5)
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1 (2)
1118-1138: Tighten success-path assertions; guard against OS-level regressionAssert that Credential is not forwarded and that no OS-level DC check is invoked (ensuring Get-DomainControllerObject stays generic).
Context 'When domain controller object could be found' { BeforeAll { Mock -CommandName Get-ADDomainController -MockWith { return @{ Site = 'MySite' Domain = 'contoso.com' IsGlobalCatalog = $true } } + # Guard: ensure Get-DomainControllerObject doesn't rely on OS-level checks + Mock -CommandName Test-IsDomainController } It 'Should return the correct values for each property' { $getDomainControllerObjectResult = Get-DomainControllerObject -DomainName 'contoso.com' $getDomainControllerObjectResult.Site | Should -Be 'MySite' $getDomainControllerObjectResult.Domain | Should -Be 'contoso.com' $getDomainControllerObjectResult.IsGlobalCatalog | Should -BeTrue - Should -Invoke -CommandName Get-ADDomainController -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Get-ADDomainController -ParameterFilter { + -not $PesterBoundParameters.ContainsKey('Credential') + } -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Test-IsDomainController -Exactly -Times 0 -Scope It } }
1140-1166: Verify credential pass-through and absence of OS-level checksKeep the existing credential check, and add a regression guard so OS-level DC checks aren’t reintroduced.
Context 'When domain controller object could be found, using specific credential' { BeforeAll { Mock -CommandName Get-ADDomainController -MockWith { return @{ Site = 'MySite' Domain = 'contoso.com' IsGlobalCatalog = $true } } + # Guard: ensure Get-DomainControllerObject doesn't rely on OS-level checks + Mock -CommandName Test-IsDomainController $mockAdministratorUser = 'admin@contoso.com' $mockAdministratorPassword = 'P@ssw0rd-12P@ssw0rd-12' | ConvertTo-SecureString -AsPlainText -Force $mockAdministratorCredential = New-Object -TypeName 'System.Management.Automation.PSCredential' -ArgumentList @($mockAdministratorUser, $mockAdministratorPassword) } It 'Should return the correct values for each property' { $getDomainControllerObjectResult = Get-DomainControllerObject -DomainName 'contoso.com' -Credential $mockAdministratorCredential $getDomainControllerObjectResult.Site | Should -Be 'MySite' $getDomainControllerObjectResult.Domain | Should -Be 'contoso.com' $getDomainControllerObjectResult.IsGlobalCatalog | Should -BeTrue Should -Invoke -CommandName Get-ADDomainController -ParameterFilter { $PesterBoundParameters.ContainsKey('Credential') -eq $true } -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Test-IsDomainController -Exactly -Times 0 -Scope It } }source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (2)
32-42: Sync “Used Functions” list with actual callsGet-TargetResource calls Get-DomainObject but the list doesn’t include it; it lists Get-ADDomain which isn’t used here.
As per coding guidelines
Suggested edit:
- Get-ADDomain | ActiveDirectory + Get-DomainObject | ActiveDirectoryDsc.Common
84-86: Avoid backticks and implicit pipeline identitiesBackticks aren’t recommended; also prefer explicit -Identity over piping DN/objects into AD cmdlets for clarity.
As per coding guidelines
Example refactor:
-$domainControllerObject = Get-DomainControllerObject ` - -DomainName $DomainName -ComputerName $env:COMPUTERNAME -Credential $Credential +$domainControllerObject = Get-DomainControllerObject -DomainName $DomainName -ComputerName $env:COMPUTERNAME -Credential $Credential-$domainControllerComputerObject = $domainControllerObject.ComputerObjectDN | - Get-ADComputer -Properties ManagedBy -Credential $Credential +$domainControllerComputerObject = Get-ADComputer -Identity $domainControllerObject.ComputerObjectDN -Properties ManagedBy -Credential $Credential-$domainControllerManagedByObject = $domainControllerComputerObject.ManagedBy | - Get-ADObject -Properties objectSid -Credential $Credential +$domainControllerManagedByObject = Get-ADObject -Identity $domainControllerComputerObject.ManagedBy -Properties objectSid -Credential $CredentialAlso applies to: 98-103
tests/Unit/MSFT_ADDomainController.Tests.ps1 (1)
1137-1153: Consider phasing out “Should -Not -Throw”Guidelines recommend invoking commands directly instead of asserting “-Not -Throw.” Adopt over time to reduce noise and align with repo standards.
As per coding guidelines
Also applies to: 1156-1172, 1175-1191, 1194-1210, 1237-1253, 1274-1291, 1293-1310, 1312-1329, 1331-1349, 1352-1370, 1373-1391, 1394-1412, 1415-1433, 1436-1452, 1455-1471, 1493-1507, 1550-1566, 1579-1594, 1610-1626, 1653-1676, 1696-1714, 1734-1752, 1779-1796, 1823-1840, 1865-1880, 1904-1919, 1933-1948, 1962-1977, 2005-2023, 2043-2061, 2074-2092
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1(7 hunks)tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1(2 hunks)tests/Unit/MSFT_ADDomainController.Tests.ps1(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 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:
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1tests/Unit/MSFT_ADDomainController.Tests.ps1
tests/[Uu]nit/**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
tests/[Uu]nit/**/*.[Tt]ests.ps1: # Unit Tests Guidelines
- Test with localized strings: Use
InModuleScope -ScriptBlock { $script:localizedData.Key }- Mock files: Use
$TestDrivevariable (path to the test drive)- All public commands require parameter set validation tests
- After modifying classes, always run tests in new session (for changes to take effect)
Test Setup Requirements
Use this exact setup block before
Describe:[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param () BeforeDiscovery { try { if (-not (Get-Module -Name 'DscResource.Test')) { # 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 have 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 noop" first.' } } BeforeAll { $script:moduleName = '{MyModuleName}' Import-Module -Name $script:moduleName -Force -ErrorAction 'Stop' $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Mock:ModuleName'] = $script:moduleName $PSDefaultParameterValues['Should:ModuleName'] = $script:moduleName } AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleNam...
Files:
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1tests/Unit/MSFT_ADDomainController.Tests.ps1
{**/*.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:
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1tests/Unit/MSFT_ADDomainController.Tests.ps1
**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
**/*.[Tt]ests.ps1: # Tests GuidelinesCore Requirements
- All public commands, private functions and classes must have unit tests
- All public commands and class-based resources must have integration tests
- Use Pester v5 syntax only
- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test verbose messages, debug messages or parameter binding behavior
- Pass all mandatory parameters to avoid prompts
Requirements
- Inside
Itblocks, assign unused return objects to$null(unless part of pipeline)- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cycles
Naming
- One
Describeblock per file matching the tested entity nameContextdescriptions start with 'When'Itdescriptions start with 'Should', must not contain 'when'- Mock variables prefix: 'mock'
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- Each class method = separate
Contextblock- Each scenario = separate
Contextblock- Use nested
Contextblocks for complex scenarios- Mocking in
BeforeAll(BeforeEachonly when required)- Setup/teardown in
BeforeAll,BeforeEach/AfterAll,AfterEachclose to usageSyntax Rules
- PascalCase:
Describe,Context,It,Should,BeforeAll,BeforeEach,AfterAll,AfterEach- Use
-BeTrue/-BeFalsenever-Be $true/-Be $false- Never use
Assert-MockCalled, useShould -Invokeinstead- No
Should -Not -Throw- invoke commands directly- Never add an empty
-MockWithblock- Omit
-MockWithwhen returning$null- Set
$PSDefaultParameterValuesforMock:ModuleName,Should:ModuleName,InModuleScope:ModuleName- Omit
-ModuleNameparameter on Pester commands- Never use
Mockinside `InModuleSc...
Files:
tests/Unit/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.Tests.ps1tests/Unit/MSFT_ADDomainController.Tests.ps1
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_ADDomainController/MSFT_ADDomainController.psm1
🧠 Learnings (4)
📚 Learning: 2025-08-16T13:35:08.323Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:0-0
Timestamp: 2025-08-16T13:35:08.323Z
Learning: In PowerShell, users expect to receive $null when no objects are found or when a non-terminating error occurs, rather than empty arrays. This is normal PowerShell behavior and should be maintained in DSC commands.
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
📚 Learning: 2025-08-29T17:22:23.268Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-localization.instructions.md:0-0
Timestamp: 2025-08-29T17:22:23.268Z
Learning: Applies to source/**/*.ps1 : When emitting messages, reference $script:localizedData.KeyName and format with the -f operator (e.g., Write-Verbose -Message ($script:localizedData.KeyName -f $value1))
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.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 : Use localized strings for all messages (e.g., Write-Verbose, Write-Error)
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
📚 Learning: 2025-08-17T10:48:15.384Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/suffix.ps1:24-24
Timestamp: 2025-08-17T10:48:15.384Z
Learning: In source/suffix.ps1, the Write-Verbose message in the catch block for Import-SqlDscPreferredModule does not need localization because the exception message from Import-SqlDscPreferredModule is already localized by that command, making it an edge case exception to the localization guidelines.
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.ActiveDirectoryDsc (Build Package Module)
🔇 Additional comments (2)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (1)
92-112: Good null-guards around RODC delegated admin resolutionChecks for ComputerObjectDN, ManagedBy, and objectSid prevent pipeline/property errors. Nicely hardened.
Based on learnings
tests/Unit/MSFT_ADDomainController.Tests.ps1 (1)
63-90: Tests correctly model the OS-level DC checkMocks and Should -Invoke counts for Test-IsDomainController are spot on and validate the new control flow paths.
Also applies to: 92-120, 147-188, 190-233, 235-319, 321-367
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (2)
82-84: Avoid backticks; use splatting and Get-ComputerNameBackticks are discouraged. Also prefer Get-ComputerName over $env:COMPUTERNAME (per guidelines, DscResource.Common is already imported).
- $domainControllerObject = Get-DomainControllerObject ` - -DomainName $DomainName -ComputerName $env:COMPUTERNAME -Credential $Credential + $getDomainControllerObjectParams = @{ + DomainName = $DomainName + ComputerName = Get-ComputerName + Credential = $Credential + } + $domainControllerObject = Get-DomainControllerObject @getDomainControllerObjectParamsAs per coding guidelines
467-467: Prefer Get-ComputerName in verbose messageReplace $env:COMPUTERNAME for cross‑platform correctness.
- Write-Verbose -Message ($script:localizedData.IsDomainControllerInDomain -f $env:COMPUTERNAME, $DomainName) + Write-Verbose -Message ($script:localizedData.IsDomainControllerInDomain -f (Get-ComputerName), $DomainName)As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1(7 hunks)source/DSCResources/MSFT_ADDomainController/en-US/MSFT_ADDomainController.strings.psd1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/DSCResources/MSFT_ADDomainController/en-US/MSFT_ADDomainController.strings.psd1
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ 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:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
{**/*.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_ADDomainController/MSFT_ADDomainController.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_ADDomainController/MSFT_ADDomainController.psm1
🧠 Learnings (4)
📚 Learning: 2025-08-16T13:35:08.323Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:0-0
Timestamp: 2025-08-16T13:35:08.323Z
Learning: In PowerShell, users expect to receive $null when no objects are found or when a non-terminating error occurs, rather than empty arrays. This is normal PowerShell behavior and should be maintained in DSC commands.
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
📚 Learning: 2025-08-29T17:22:23.268Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-localization.instructions.md:0-0
Timestamp: 2025-08-29T17:22:23.268Z
Learning: Applies to source/**/*.ps1 : When emitting messages, reference $script:localizedData.KeyName and format with the -f operator (e.g., Write-Verbose -Message ($script:localizedData.KeyName -f $value1))
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.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 : Use localized strings for all messages (e.g., Write-Verbose, Write-Error)
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
📚 Learning: 2025-08-17T10:48:15.384Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/suffix.ps1:24-24
Timestamp: 2025-08-17T10:48:15.384Z
Learning: In source/suffix.ps1, the Write-Verbose message in the catch block for Import-SqlDscPreferredModule does not need localization because the exception message from Import-SqlDscPreferredModule is already localized by that command, making it an edge case exception to the localization guidelines.
Applied to files:
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
🔇 Additional comments (5)
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1 (5)
68-81: Good: OS-level DC check before AD queriesThis fixes the pre-staged RODC false-positive by gating on actual OS DC state. No issues.
90-110: Delegated admin resolution now defensively guardedNull checks for ComputerObjectDN, ManagedBy, and objectSid look correct and prevent pipeline errors. LGTM.
112-121: Replication policy and service/registry readsLooks correct and idempotent; array casts are handled later. No changes needed.
122-141: Target hashtable assemblyProperties and types look consistent; wrapping lists with @() ensures stable array types. LGTM.
513-517: No change needed for ManagedBy parameter
Set-ADComputer -ManagedBy accepts SID strings (objectSid), so using Resolve-SecurityIdentifier is valid.
source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1
Show resolved
Hide resolved
|
@johlju Sorry know you're ultra-busy - if you did get a chance to merge this would really help as can't deploy any RODCs in our environment at present :) |
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
A few bugs discovered when I tried to add an RODC using a pre-staged account. This should fix them:
All seems to work OK in testing
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