AdcsOnlineResponder: Convert to class-based
#158
Draft
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
WalkthroughMigrates AdcsOnlineResponder from a MOF/module-based DSC resource to a class-based DSC resource: removes legacy MOF/module/strings/README and module-based tests, adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LCM as "DSC Engine (LCM)"
participant Class as "AdcsOnlineResponder (Class)"
participant ADCS as "ADCSDeployment Module"
User->>LCM: Apply configuration (AdcsOnlineResponder)
LCM->>Class: Invoke Get/Test/Set
Class->>ADCS: Call Install-AdcsOnlineResponder / Uninstall-AdcsOnlineResponder / Install-AdcsOnlineResponder -WhatIf
alt ADCS returns success
ADCS-->>Class: Success
Class-->>LCM: Return state / success
else ADCS throws OnlineResponderSetupException
ADCS-->>Class: OnlineResponderSetupException
Class-->>LCM: Interpret as "Present" / return state
else ADCS throws other error
ADCS-->>Class: Error
Class-->>LCM: Throw localized error (ErrorGetCurrentState)
end
LCM-->>User: Report status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(3 hunks)source/Classes/020.AdcsOnlineResponder.ps1(1 hunks)source/DSCResources/DSC_AdcsOnlineResponder/DSC_AdcsOnlineResponder.psm1(0 hunks)source/DSCResources/DSC_AdcsOnlineResponder/DSC_AdcsOnlineResponder.schema.mof(0 hunks)source/DSCResources/DSC_AdcsOnlineResponder/README.md(0 hunks)source/DSCResources/DSC_AdcsOnlineResponder/en-US/DSC_AdcsOnlineResponder.strings.psd1(0 hunks)source/en-US/AdcsOnlineResponder.strings.psd1(1 hunks)tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1(1 hunks)tests/Unit/DSC_AdcsOnlineResponder.Tests.ps1(0 hunks)
💤 Files with no reviewable changes (5)
- source/DSCResources/DSC_AdcsOnlineResponder/en-US/DSC_AdcsOnlineResponder.strings.psd1
- source/DSCResources/DSC_AdcsOnlineResponder/README.md
- source/DSCResources/DSC_AdcsOnlineResponder/DSC_AdcsOnlineResponder.schema.mof
- tests/Unit/DSC_AdcsOnlineResponder.Tests.ps1
- source/DSCResources/DSC_AdcsOnlineResponder/DSC_AdcsOnlineResponder.psm1
🔇 Additional comments (1)
CHANGELOG.md (1)
10-10: LGTM! Changelog updates are properly formatted and documented.The addition of backticks for code formatting and the new entry documenting the conversion to a class-based resource are appropriate and follow the established changelog format.
Also applies to: 25-25, 38-39
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
===================================
- Coverage 98% 98% -1%
===================================
Files 8 10 +2
Lines 514 477 -37
===================================
- Hits 505 468 -37
Misses 9 9 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1 (2)
473-571: Remove skipped tests that reference non-existent properties.These tests reference
AiaUriandOcspUriproperties that don't belong to theAdcsOnlineResponderclass. They appear to be copied fromAdcsAuthorityInformationAccesstests and are not applicable here.
573-854: Remove skipped tests that reference non-existent properties.These
Modify()tests also referenceAiaUriandOcspUriproperties and CA Authority Information Access commands that are not relevant to theAdcsOnlineResponderclass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/Unit/Classes/AdcsAuthorityInformationAccess.Tests.ps1(1 hunks)tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1(1 hunks)
🔇 Additional comments (1)
tests/Unit/Classes/AdcsAuthorityInformationAccess.Tests.ps1 (1)
597-597: Good catch! Fixed missing closing brace.This corrects the block structure by properly closing the
BeforeAllblock.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1 (4)
304-304: Add missing space before opening brace.-Describe 'AdcsOnlineResponder\Set()' -Tag 'Set' { +Describe 'AdcsOnlineResponder\Set()' -Tag 'Set' {
473-571: Remove skipped tests that reference non-existent properties.These tests reference
AiaUriandOcspUriproperties that don't exist in the AdcsOnlineResponder class. They appear to be copied from another resource and should be removed entirely as they're not applicable to this resource.
573-854: Remove skipped Modify() tests that use incorrect properties.Like the GetCurrentState() tests, these Modify() tests reference non-existent
AiaUriandOcspUriproperties and mock commands that aren't relevant to AdcsOnlineResponder. These tests should be removed or completely rewritten to test the actual functionality of the AdcsOnlineResponder resource.
888-888: Fix incorrect test description.The test description says "Should throw an error" but the test expects it NOT to throw.
- It 'Should throw an error' { + It 'Should not throw an error' {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1(1 hunks)
🔇 Additional comments (3)
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1 (3)
60-88: Well-structured class instantiation tests.The tests correctly verify that the AdcsOnlineResponder class can be instantiated and has the expected type.
90-302: Comprehensive Get() method test coverage.The tests properly cover all scenarios for the Get() method including desired/not desired states and installed/not installed conditions. Good use of mocking for internal methods.
393-471: Well-implemented Test() method tests.The tests correctly verify the Test() method behavior for both desired and not desired states.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
source/en-US/AdcsOnlineResponder.strings.psd1 (1)
7-13: String resource added and correctly keyed.ErrorGetCurrentState is now active and matches the tests’ expectation. Thanks for closing the loop.
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1 (1)
574-854: Remove the skipped Modify() tests that reference non-existent AIA/OCSP properties.They’re from another resource and add noise. Either replace with AdcsOnlineResponder-specific Modify() tests or delete.
Apply this diff to remove the irrelevant block:
-Describe 'AdcsOnlineResponder\Modify()' -Tag 'HiddenMember' -Skip:$true { - # ... AIA/OCSP tests ... -}
🧹 Nitpick comments (5)
RequiredModules.psd1 (1)
41-46: Consider pinning to a known-good version to keep CI deterministic.Using 'latest' + prerelease can introduce breaking changes unexpectedly. If stability is preferred, pin a specific version or minimum compatible range once validated.
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1 (4)
62-68: Avoid “Should -Not -Throw” per test guidelines; this test is redundant.Type/constructor existence is already covered below. Recommend removing this It block.
Apply this diff:
- It 'Should not throw an exception' { - InModuleScope -ScriptBlock { - Set-StrictMode -Version 1.0 - - { [AdcsOnlineResponder]::new() } | Should -Not -Throw - } - }
516-519: Throw a typed exception instead of a string to accurately exercise catch logic.Mocking with a string throws RuntimeException; if the resource catches OnlineResponderSetupException by type, this test may be misleading.
Apply this diff here:
- Mock -CommandName Install-AdcsOnlineResponder -MockWith { - throw 'Microsoft.CertificateServices.Deployment.Common.OCSP.OnlineResponderSetupException' - } + Mock -CommandName Install-AdcsOnlineResponder -MockWith { + throw [Microsoft.CertificateServices.Deployment.Common.OCSP.OnlineResponderSetupException]::new('Already installed') + }And define the mock exception type in AdcsDeploymentStub.psm1:
Add-Type -TypeDefinition @' namespace Microsoft.CertificateServices.Deployment.Common.OCSP { public class OnlineResponderSetupException : System.Exception { public OnlineResponderSetupException() {} public OnlineResponderSetupException(string message) : base(message) {} } } '@
420-425: Return a value from mocked Get() for clarity.Base Test() may read Get()’s output; returning an explicit state makes the tests more robust.
Apply this diff in both Get() mocks:
Add-Member -Force -MemberType 'ScriptMethod' -Name 'Get' -Value { $script:getMethodCallCount += 1 + return @{ + IsSingleInstance = 'Yes' + Ensure = [Ensure]::Present + Reasons = @() + } }Also applies to: 446-449
35-37: Remove commented-out code or implement the mock exception properly.Keep tests clean: either define the exception in the stub (see suggestion above) or delete this commented line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
RequiredModules.psd1(1 hunks)source/Classes/020.AdcsOnlineResponder.ps1(1 hunks)source/en-US/AdcsOnlineResponder.strings.psd1(1 hunks)tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/Classes/020.AdcsOnlineResponder.ps1
🧰 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
- Run in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
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 guidelines over existing code patterns
- 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
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
source/en-US/AdcsOnlineResponder.strings.psd1tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1RequiredModules.psd1
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # 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.
- OUTPUTS: Lis...
Files:
source/en-US/AdcsOnlineResponder.strings.psd1tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1RequiredModules.psd1
**/*.[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
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- Prefer
-BeTrue/-BeFalseover-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
MockinsideInModuleScope-blockFile Organization
- Class resources:
tests/Unit/Classes/{Name}.Tests.ps1- Public commands:
tests/Unit/Public/{Name}.Tests.ps1- Private functions:
tests/Unit/Private/{Name}.Tests.ps1Data-Driven Tests
- Define variables in separate
BeforeDiscoveryfor-ForEach(close to usage)-ForEachal...
Files:
tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1
🔇 Additional comments (1)
RequiredModules.psd1 (1)
41-46: Allowing prerelease for DscResource.Common looks correct for class-based resources.This aligns with the class-based migration and matches how DscResource.Base is handled. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/Unit/Stubs/AdcsDeploymentStub.psm1 (1)
10-17: Guard Add-Type and add inner-exception ctor for fidelity.Prevents redefinition errors across repeated imports and better mirrors real .NET exceptions.
-Add-Type -TypeDefinition @' +if (-not ('Microsoft.CertificateServices.Deployment.Common.OCSP.OnlineResponderSetupException' -as [type])) +{ +Add-Type -TypeDefinition @' namespace Microsoft.CertificateServices.Deployment.Common.OCSP { public class OnlineResponderSetupException : System.Exception { - public OnlineResponderSetupException() {} - public OnlineResponderSetupException(string message) : base(message) {} + public OnlineResponderSetupException() {} + public OnlineResponderSetupException(string message) : base(message) {} + public OnlineResponderSetupException(string message, System.Exception inner) : base(message, inner) {} } } '@ +}source/Classes/020.AdcsOnlineResponder.ps1 (3)
6-11: Add Required “Requirements” and “Known issues” subsections in DESCRIPTION.Needed by repo guidelines; include link to open issues.
.DESCRIPTION This resource can be used to install an ADCS Online Responder after the feature has been installed on the server. Using this DSC Resource to configure an ADCS Certificate Authority assumes that - the ```ADCS-Online-Responder``` feature has already been installed. + the 'ADCS-Online-Responder' feature has already been installed. + + ## Requirements + - Windows Server with ADCSDeployment module available. + - DscResource.Base and DscResource.Common modules. + + ## Known issues + All issues are not listed here, see all open issues: + https://github.com/dsccommunity/ActiveDirectoryCSDsc/issues?q=is%3Aissue+is%3Aopen+in%3Atitle+AdcsOnlineResponder
140-143: Validate IsSingleInstance value in AssertProperties.Ensure only 'Yes' is accepted; fail fast with localized message.
hidden [void] AssertProperties([System.Collections.Hashtable] $properties) { Assert-Module -ModuleName 'ADCSDeployment' + + if ($properties.ContainsKey('IsSingleInstance') -and $properties.IsSingleInstance -ne 'Yes') + { + # Example key: ErrorIsSingleInstance = "IsSingleInstance must be 'Yes'. Actual: {0} (AOR0003)" + New-InvalidDataException -Message ($this.localizedData.ErrorIsSingleInstance -f $properties.IsSingleInstance) + } }
28-35: Add EXAMPLE section(s) to comment-based help.Required by repo guidelines; include at least Present and Absent examples.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
source/Classes/020.AdcsOnlineResponder.ps1(1 hunks)tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1(1 hunks)tests/Unit/Stubs/AdcsDeploymentStub.psm1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Classes/AdcsOnlineResponder.Tests.ps1
🧰 Additional context used
📓 Path-based instructions (4)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
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 guidelines over existing code patterns
- 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
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
tests/Unit/Stubs/AdcsDeploymentStub.psm1source/Classes/020.AdcsOnlineResponder.ps1
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # 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.
- OUTPUTS: Lis...
Files:
tests/Unit/Stubs/AdcsDeploymentStub.psm1source/Classes/020.AdcsOnlineResponder.ps1
source/**/*.ps1
⚙️ CodeRabbit configuration file
source/**/*.ps1: # Localization GuidelinesRequirements
- Localize all Write-Debug, Write-Verbose, Write-Error, Write-Warning and $PSCmdlet.ThrowTerminatingError() messages
- Use localized string keys, not hardcoded strings
- Assume
$script:localizedDatais availableString Files
- Commands/functions:
source/en-US/{MyModuleName}.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming Patterns
- Format:
Verb_FunctionName_Action(underscore separators), e.g.Get_Database_ConnectingToDatabaseString Format
ConvertFrom-StringData @' KeyName = Message with {0} placeholder. (PREFIX0001) '@String IDs
- Format:
(PREFIX####)- PREFIX: First letter of each word in class or function name (SqlSetup → SS, Get-SqlDscDatabase → GSDD)
- Number: Sequential from 0001
Usage
Write-Verbose -Message ($script:localizedData.KeyName -f $value1)
Files:
source/Classes/020.AdcsOnlineResponder.ps1
source/[cC]lasses/**/*.ps1
⚙️ CodeRabbit configuration file
source/[cC]lasses/**/*.ps1: # DSC Class-Based Resource GuidelinesApplies to: Classes with
[DscResource(...)]decoration only.Requirements
- File:
source/Classes/020.{ResourceName}.ps1- Decoration:
[DscResource(RunAsCredential = 'Optional')](replace with'Mandatory'if required)- Inheritance: Must inherit
ResourceBase(part of module DscResource.Base)$this.localizedDatahashtable auto-populated byResourceBasefrom localization file- value-type properties: Use
[Nullable[{FullTypeName}]](e.g.,[Nullable[System.Int32]])Required constructor
MyResourceName () : base ($PSScriptRoot) { # Property names where state cannot be enforced, e.g. IsSingleInstance, Force $this.ExcludeDscProperties = @() }Required Method Pattern
[MyResourceName] Get() { # Call base implementation to get current state $currentState = ([ResourceBase] $this).Get() # If needed, post-processing on current state that can not be handled by GetCurrentState() return $currentState } [System.Boolean] Test() { # Call base implementation to test current state $inDesiredState = ([ResourceBase] $this).Test() # If needed, post-processing on test result that can not be handled by base Test() return $inDesiredState } [void] Set() { # Call base implementation to set desired state ([ResourceBase] $this).Set() # If needed, additional state changes that can not be handled by Modify() } hidden [System.Collections.Hashtable] GetCurrentState([System.Collections.Hashtable] $properties) { # Always return current state as hashtable, $properties contains key properties } hidden [void] Modify([System.Collections.Hashtable] $properties) { # Always set desired state, $properties contain those that must change state }Optional Method Pattern
hidden [void] AssertProperties([System.Collections.Hashtable] $properties) { # Validate user-provided properties, $p...
Files:
source/Classes/020.AdcsOnlineResponder.ps1
🧠 Learnings (2)
📓 Common learnings
Learnt from: dan-hughes
PR: dsccommunity/ActiveDirectoryCSDsc#158
File: source/Classes/020.AdcsOnlineResponder.ps1:0-0
Timestamp: 2025-08-30T18:18:31.483Z
Learning: For ADCS resources in ActiveDirectoryCSDsc, when catching exceptions from Install-Adcs* commands to determine installation state, use string matching on the exception message (e.g., `$_.Exception.ToString() -match 'OnlineResponderSetupException$'`) instead of catching specific exception types. This avoids dependency on importing types that may not be available in all environments like CI systems or DSC authoring machines.
📚 Learning: 2025-08-30T18:18:31.483Z
Learnt from: dan-hughes
PR: dsccommunity/ActiveDirectoryCSDsc#158
File: source/Classes/020.AdcsOnlineResponder.ps1:0-0
Timestamp: 2025-08-30T18:18:31.483Z
Learning: For ADCS resources in ActiveDirectoryCSDsc, when catching exceptions from Install-Adcs* commands to determine installation state, use string matching on the exception message (e.g., `$_.Exception.ToString() -match 'OnlineResponderSetupException$'`) instead of catching specific exception types. This avoids dependency on importing types that may not be available in all environments like CI systems or DSC authoring machines.
Applied to files:
tests/Unit/Stubs/AdcsDeploymentStub.psm1source/Classes/020.AdcsOnlineResponder.ps1
⏰ 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). (4)
- GitHub Check: dsccommunity.ActiveDirectoryCSDsc (Test Unit)
- GitHub Check: dsccommunity.ActiveDirectoryCSDsc (Test HQRM)
- GitHub Check: dsccommunity.ActiveDirectoryCSDsc (Test Integration)
- GitHub Check: dsccommunity.ActiveDirectoryCSDsc (Build Package Module)
Comment on lines
104
to
128
| hidden [void] Modify([System.Collections.Hashtable] $properties) | ||
| { | ||
| if ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq [Ensure]::Absent) | ||
| { | ||
| try | ||
| { | ||
| Uninstall-AdcsOnlineResponder -Force | ||
| } | ||
| catch | ||
| { | ||
| New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_ | ||
| } | ||
| } | ||
| else | ||
| { | ||
| try | ||
| { | ||
| Install-AdcsOnlineResponder -Credential $this.Credential -Force | ||
| } | ||
| catch | ||
| { | ||
| New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_ | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Localize error messages and promote non-terminating errors to terminating.
Use -ErrorAction Stop and localized strings with placeholders; avoid leaking raw exception text directly.
hidden [void] Modify([System.Collections.Hashtable] $properties)
{
if ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq [Ensure]::Absent)
{
try
{
- Uninstall-AdcsOnlineResponder -Force
+ Uninstall-AdcsOnlineResponder -Force -ErrorAction Stop
}
catch
{
- New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_
+ # Example key: ErrorModifyUninstall = "Failed to uninstall Online Responder. Details: {0} (AOR0001)"
+ New-InvalidOperationException -Message ($this.localizedData.ErrorModifyUninstall -f $_.Exception.Message) -ErrorRecord $_
}
}
else
{
try
{
- Install-AdcsOnlineResponder -Credential $this.Credential -Force
+ Install-AdcsOnlineResponder -Credential $this.Credential -Force -ErrorAction Stop
}
catch
{
- New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_
+ # Example key: ErrorModifyInstall = "Failed to install Online Responder. Details: {0} (AOR0002)"
+ New-InvalidOperationException -Message ($this.localizedData.ErrorModifyInstall -f $_.Exception.Message) -ErrorRecord $_
}
}
}Add the suggested keys to source/en-US/AdcsOnlineResponder.strings.psd1. I can draft them if helpful.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hidden [void] Modify([System.Collections.Hashtable] $properties) | |
| { | |
| if ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq [Ensure]::Absent) | |
| { | |
| try | |
| { | |
| Uninstall-AdcsOnlineResponder -Force | |
| } | |
| catch | |
| { | |
| New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_ | |
| } | |
| } | |
| else | |
| { | |
| try | |
| { | |
| Install-AdcsOnlineResponder -Credential $this.Credential -Force | |
| } | |
| catch | |
| { | |
| New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_ | |
| } | |
| } | |
| } | |
| hidden [void] Modify([System.Collections.Hashtable] $properties) | |
| { | |
| if ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq [Ensure]::Absent) | |
| { | |
| try | |
| { | |
| Uninstall-AdcsOnlineResponder -Force -ErrorAction Stop | |
| } | |
| catch | |
| { | |
| # Example key in source/en-US/AdcsOnlineResponder.strings.psd1: | |
| # ErrorModifyUninstall = "Failed to uninstall Online Responder. Details: {0} (AOR0001)" | |
| New-InvalidOperationException -Message ($this.localizedData.ErrorModifyUninstall -f $_.Exception.Message) -ErrorRecord $_ | |
| } | |
| } | |
| else | |
| { | |
| try | |
| { | |
| Install-AdcsOnlineResponder -Credential $this.Credential -Force -ErrorAction Stop | |
| } | |
| catch | |
| { | |
| # Example key in source/en-US/AdcsOnlineResponder.strings.psd1: | |
| # ErrorModifyInstall = "Failed to install Online Responder. Details: {0} (AOR0002)" | |
| New-InvalidOperationException -Message ($this.localizedData.ErrorModifyInstall -f $_.Exception.Message) -ErrorRecord $_ | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In source/Classes/020.AdcsOnlineResponder.ps1 around lines 104-128, the Modify
method currently calls Install-AdcsOnlineResponder/Uninstall-AdcsOnlineResponder
without forcing terminating errors and logs raw exception text; update both
cmdlet calls to include -ErrorAction Stop so non-terminating errors become
terminating, replace direct use of $_.Exception.Message with localized string
lookups containing placeholders (add corresponding keys to
source/en-US/AdcsOnlineResponder.strings.psd1), and in each catch construct and
throw a terminating error using New-InvalidOperationException with the formatted
localized message and the original ErrorRecord so callers receive a proper
terminating error and no raw exception text is leaked.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
source/Classes/020.AdcsOnlineResponder.ps1 (3)
28-29: Use RunAsCredential = 'Optional' to align with module guidelines.Guidelines require 'Optional' (or 'Mandatory' if needed); 'NotSupported' deviates.
-[DscResource(RunAsCredential = 'NotSupported')] +[DscResource(RunAsCredential = 'Optional')] class AdcsOnlineResponder : ResourceBase
95-119: Promote non-terminating errors to terminating and localize error messages in Modify().
- Add -ErrorAction Stop for both cmdlets.
- Use localized messages with placeholders; avoid leaking raw exception text.
hidden [void] Modify([System.Collections.Hashtable] $properties) { if ($properties.ContainsKey('Ensure') -and $properties.Ensure -eq [Ensure]::Absent) { try { - $null = Uninstall-AdcsOnlineResponder -Force + $null = Uninstall-AdcsOnlineResponder -Force -ErrorAction Stop } catch { - New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_ + # Example key: ErrorModifyUninstall = "Failed to uninstall Online Responder. Details: {0} (ADCSOR0002)" + New-InvalidOperationException -Message ($this.localizedData.ErrorModifyUninstall -f $_.Exception.Message) -ErrorRecord $_ } } else { try { - $null = Install-AdcsOnlineResponder -Credential $this.Credential -Force + $null = Install-AdcsOnlineResponder -Credential $this.Credential -Force -ErrorAction Stop } catch { - New-InvalidOperationException -Message $_.Exception.Message -ErrorRecord $_ + # Example key: ErrorModifyInstall = "Failed to install Online Responder. Details: {0} (ADCSOR0003)" + New-InvalidOperationException -Message ($this.localizedData.ErrorModifyInstall -f $_.Exception.Message) -ErrorRecord $_ } } }If you want, I can draft the en-US string entries (ErrorModifyInstall/Uninstall) and update the unit tests accordingly.
63-82: Make GetCurrentState robust: force terminating errors, avoid type dependency, and do not return from New-InvalidOperationException.
- Add -ErrorAction Stop so errors are catchable.
- Match by message/type text (per learning) rather than CLR type.
- New-* helper throws; don’t return its result.
[System.Collections.Hashtable] GetCurrentState([System.Collections.Hashtable] $properties) { try { - $null = Install-AdcsOnlineResponder -Credential $this.Credential -WhatIf + $null = Install-AdcsOnlineResponder -Credential $this.Credential -WhatIf -ErrorAction Stop return @{} } catch { - if ($_.Exception.GetType().FullName -eq 'Microsoft.CertificateServices.Deployment.Common.OCSP.OnlineResponderSetupException') + if ($_.Exception.ToString() -match 'OnlineResponderSetupException$') { return @{ IsSingleInstance = $this.IsSingleInstance } } - return New-InvalidOperationException -Message $this.localizedData.ErrorGetCurrentState -ErrorRecord $_ + New-InvalidOperationException -Message $this.localizedData.ErrorGetCurrentState -ErrorRecord $_ } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
source/Classes/020.AdcsOnlineResponder.ps1(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
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 guidelines over existing code patterns
- 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
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
source/Classes/020.AdcsOnlineResponder.ps1
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # 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.
- OUTPUTS: Lis...
Files:
source/Classes/020.AdcsOnlineResponder.ps1
source/**/*.ps1
⚙️ CodeRabbit configuration file
source/**/*.ps1: # Localization GuidelinesRequirements
- Localize all Write-Debug, Write-Verbose, Write-Error, Write-Warning and $PSCmdlet.ThrowTerminatingError() messages
- Use localized string keys, not hardcoded strings
- Assume
$script:localizedDatais availableString Files
- Commands/functions:
source/en-US/{MyModuleName}.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming Patterns
- Format:
Verb_FunctionName_Action(underscore separators), e.g.Get_Database_ConnectingToDatabaseString Format
ConvertFrom-StringData @' KeyName = Message with {0} placeholder. (PREFIX0001) '@String IDs
- Format:
(PREFIX####)- PREFIX: First letter of each word in class or function name (SqlSetup → SS, Get-SqlDscDatabase → GSDD)
- Number: Sequential from 0001
Usage
Write-Verbose -Message ($script:localizedData.KeyName -f $value1)
Files:
source/Classes/020.AdcsOnlineResponder.ps1
source/[cC]lasses/**/*.ps1
⚙️ CodeRabbit configuration file
source/[cC]lasses/**/*.ps1: # DSC Class-Based Resource GuidelinesApplies to: Classes with
[DscResource(...)]decoration only.Requirements
- File:
source/Classes/020.{ResourceName}.ps1- Decoration:
[DscResource(RunAsCredential = 'Optional')](replace with'Mandatory'if required)- Inheritance: Must inherit
ResourceBase(part of module DscResource.Base)$this.localizedDatahashtable auto-populated byResourceBasefrom localization file- value-type properties: Use
[Nullable[{FullTypeName}]](e.g.,[Nullable[System.Int32]])Required constructor
MyResourceName () : base ($PSScriptRoot) { # Property names where state cannot be enforced, e.g. IsSingleInstance, Force $this.ExcludeDscProperties = @() }Required Method Pattern
[MyResourceName] Get() { # Call base implementation to get current state $currentState = ([ResourceBase] $this).Get() # If needed, post-processing on current state that can not be handled by GetCurrentState() return $currentState } [System.Boolean] Test() { # Call base implementation to test current state $inDesiredState = ([ResourceBase] $this).Test() # If needed, post-processing on test result that can not be handled by base Test() return $inDesiredState } [void] Set() { # Call base implementation to set desired state ([ResourceBase] $this).Set() # If needed, additional state changes that can not be handled by Modify() } hidden [System.Collections.Hashtable] GetCurrentState([System.Collections.Hashtable] $properties) { # Always return current state as hashtable, $properties contains key properties } hidden [void] Modify([System.Collections.Hashtable] $properties) { # Always set desired state, $properties contain those that must change state }Optional Method Pattern
hidden [void] AssertProperties([System.Collections.Hashtable] $properties) { # Validate user-provided properties, $p...
Files:
source/Classes/020.AdcsOnlineResponder.ps1
🧠 Learnings (2)
📓 Common learnings
Learnt from: dan-hughes
PR: dsccommunity/ActiveDirectoryCSDsc#158
File: source/Classes/020.AdcsOnlineResponder.ps1:0-0
Timestamp: 2025-08-30T18:18:31.483Z
Learning: For ADCS resources in ActiveDirectoryCSDsc, when catching exceptions from Install-Adcs* commands to determine installation state, use string matching on the exception message (e.g., `$_.Exception.ToString() -match 'OnlineResponderSetupException$'`) instead of catching specific exception types. This avoids dependency on importing types that may not be available in all environments like CI systems or DSC authoring machines.
📚 Learning: 2025-08-30T18:18:31.483Z
Learnt from: dan-hughes
PR: dsccommunity/ActiveDirectoryCSDsc#158
File: source/Classes/020.AdcsOnlineResponder.ps1:0-0
Timestamp: 2025-08-30T18:18:31.483Z
Learning: For ADCS resources in ActiveDirectoryCSDsc, when catching exceptions from Install-Adcs* commands to determine installation state, use string matching on the exception message (e.g., `$_.Exception.ToString() -match 'OnlineResponderSetupException$'`) instead of catching specific exception types. This avoids dependency on importing types that may not be available in all environments like CI systems or DSC authoring machines.
Applied to files:
source/Classes/020.AdcsOnlineResponder.ps1
Comment on lines
+6
to
+11
| .DESCRIPTION | ||
| This resource can be used to install an ADCS Online Responder after the feature | ||
| has been installed on the server. | ||
| Using this DSC Resource to configure an ADCS Certificate Authority assumes that | ||
| the ```ADCS-Online-Responder``` feature has already been installed. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inaccurate wording and add required “Requirements” and “Known issues” sections in .DESCRIPTION.
“Certificate Authority” is incorrect here; this resource configures the Online Responder. Also add the required sections per guidelines.
.DESCRIPTION
- This resource can be used to install an ADCS Online Responder after the feature
- has been installed on the server.
- Using this DSC Resource to configure an ADCS Certificate Authority assumes that
- the ```ADCS-Online-Responder``` feature has already been installed.
+ This resource can be used to install and configure an ADCS Online Responder after
+ the Windows feature has been installed on the server.
+ Using this DSC resource to configure an ADCS Online Responder assumes that the
+ 'ADCS-Online-Responder' Windows feature is already installed.
+
+ ## Requirements
+ - Windows Server with Active Directory Certificate Services available.
+ - ADCSDeployment PowerShell module present on the node.
+ - 'ADCS-Online-Responder' feature installed.
+
+ ## Known issues
+ All issues are not listed here, see all open issues:
+ https://github.com/dsccommunity/ActiveDirectoryCSDsc/issues?q=is%3Aissue+is%3Aopen+in%3Atitle+AdcsOnlineResponder🤖 Prompt for AI Agents
In source/Classes/020.AdcsOnlineResponder.ps1 around lines 6 to 11, the
.DESCRIPTION text incorrectly refers to a "Certificate Authority" and lacks
required "Requirements" and "Known issues" sections; update the description to
say this resource installs and configures an ADCS Online Responder after the
Windows feature is installed, change wording to reference the
'ADCS-Online-Responder' Windows feature (no triple-backticks), and append a
Requirements section listing: Windows Server with AD CS available;
ADCSDeployment PowerShell module present on the node; 'ADCS-Online-Responder'
feature installed, plus a Known issues section that points to the repository
issues URL for AdcsOnlineResponder.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Integration/DSC_AdcsCertificationAuthority.Integration.Tests.ps1 (2)
248-251: DRY this by moving AfterEach to the Describe scope.Define a single AfterEach at the Describe 'DSC_AdcsAuthorityInformationAccess_Integration' level to cover both contexts, and remove the two duplicated blocks.
Apply within these lines:
- AfterEach { - Wait-ForIdleLcm - }Add once inside the Describe (outside the contexts), for example right after the config import:
AfterEach { Wait-ForIdleLcm }
297-300: Same here — consolidate AfterEach at Describe level.Remove this duplicate AfterEach if you adopt a single Describe-scoped hook.
Apply within these lines:
- AfterEach { - Wait-ForIdleLcm - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/Integration/DSC_AdcsCertificationAuthority.Integration.Tests.ps1(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
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 guidelines over existing code patterns
- 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
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
tests/Integration/DSC_AdcsCertificationAuthority.Integration.Tests.ps1
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # 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.
- OUTPUTS: Lis...
Files:
tests/Integration/DSC_AdcsCertificationAuthority.Integration.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
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- Prefer
-BeTrue/-BeFalseover-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
MockinsideInModuleScope-blockFile Organization
- Class resources:
tests/Unit/Classes/{Name}.Tests.ps1- Public commands:
tests/Unit/Public/{Name}.Tests.ps1- Private functions:
tests/Unit/Private/{Name}.Tests.ps1Data-Driven Tests
- Define variables in separate
BeforeDiscoveryfor-ForEach(close to usage)-ForEachal...
Files:
tests/Integration/DSC_AdcsCertificationAuthority.Integration.Tests.ps1
tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1
⚙️ CodeRabbit configuration file
tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1: # Integration Tests GuidelinesRequirements
- Location Commands:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1- Location Resources:
tests/Integration/Resources/{ResourceName}.Integration.Tests.ps1- No mocking - real environment only
- Cover all scenarios and code paths
- Use
Get-ComputerNamefor computer names in CI- Avoid
ExpectedMessageforShould -Throwassertions- Only run integration tests in CI unless explicitly instructed.
- Call commands with
-Forceparameter where applicable (avoids prompting).- Use
-ErrorAction Stopon commands so failures surface immediatelyRequired Setup Block
[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 build" first.' } } BeforeAll { $script:moduleName = '{MyModuleName}' Import-Module -Name $script:moduleName -Force -ErrorAction 'Stop' }
Files:
tests/Integration/DSC_AdcsCertificationAuthority.Integration.Tests.ps1
🧠 Learnings (1)
📓 Common learnings
Learnt from: dan-hughes
PR: dsccommunity/ActiveDirectoryCSDsc#158
File: source/Classes/020.AdcsOnlineResponder.ps1:0-0
Timestamp: 2025-08-30T18:18:31.483Z
Learning: For ADCS resources in ActiveDirectoryCSDsc, when catching exceptions from Install-Adcs* commands to determine installation state, use string matching on the exception message (e.g., `$_.Exception.ToString() -match 'OnlineResponderSetupException$'`) instead of catching specific exception types. This avoids dependency on importing types that may not be available in all environments like CI systems or DSC authoring machines.
🔇 Additional comments (1)
tests/Integration/DSC_AdcsCertificationAuthority.Integration.Tests.ps1 (1)
248-251: Solid stability improvement: waiting for LCM between tests.This reduces flakiness from overlapping LCM runs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
source/ActiveDirectoryCSDsc.psd1 (2)
30-30: Avoid wildcard export for CmdletsToExport; use empty arrayNo binary cmdlets are present; wildcard hurts manifest hygiene and (slightly) perf. Prefer explicit or empty export.
- CmdletsToExport = '*' + CmdletsToExport = @()
38-40: Explicitly export class-based DSC resourcesList the new AdcsOnlineResponder to aid discovery and keep manifest explicit. Verify the class name matches exactly.
- DscResourcesToExport = @() + DscResourcesToExport = @( + 'AdcsOnlineResponder' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
RequiredModules.psd1(1 hunks)source/ActiveDirectoryCSDsc.psd1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- RequiredModules.psd1
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
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 guidelines over existing code patterns
- 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
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
source/ActiveDirectoryCSDsc.psd1
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # 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.
- OUTPUTS: Lis...
Files:
source/ActiveDirectoryCSDsc.psd1
🧠 Learnings (1)
📓 Common learnings
Learnt from: dan-hughes
PR: dsccommunity/ActiveDirectoryCSDsc#158
File: source/Classes/020.AdcsOnlineResponder.ps1:0-0
Timestamp: 2025-08-30T18:18:31.483Z
Learning: For ADCS resources in ActiveDirectoryCSDsc, when catching exceptions from Install-Adcs* commands to determine installation state, use string matching on the exception message (e.g., `$_.Exception.ToString() -match 'OnlineResponderSetupException$'`) instead of catching specific exception types. This avoids dependency on importing types that may not be available in all environments like CI systems or DSC authoring machines.
Member
|
@dan-hughes is this ready to be reviewed? |
Contributor
Author
|
No, I need to test on a real system. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
Convert AdcsOnlineResponder to class-based resource
This Pull Request (PR) fixes the following issues
AdcsOnlineResponder: Convert to class-based resource #155Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is