-
Notifications
You must be signed in to change notification settings - Fork 9
New Resource WSManClientConfig #121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new WSManClientConfig DSC resource is added to manage WS-Man client configuration properties (network delay, authentication methods, trusted hosts). Implementation includes the resource class extending WSManConfigBase, example configuration, and comprehensive integration and unit test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
===================================
- Coverage 99% 98% -1%
===================================
Files 8 9 +1
Lines 193 218 +25
===================================
+ Hits 192 215 +23
- Misses 1 3 +2
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 18: The CHANGELOG.md entry containing "`WSManClientConfig` resource.
Fixes [`#41`](https://github.com/dsccommunity/WSManDsc/issues/41)." is longer than
80 characters; update the line by wrapping the issue link onto the next line so
the first line stays ≤80 chars and indent the continued line with two spaces
(preserve the text exactly, just break after the sentence and place the link on
the next line with a two-space indent).
In `@source/Classes/020.WSManClientConfig.ps1`:
- Around line 5-7: The class help for the WSManClientConfig class is missing the
required "## Requirements" and "## Known issues" sections in the .DESCRIPTION
block; update the .DESCRIPTION for the WSManClientConfig class in
source/Classes/020.WSManClientConfig.ps1 to include a "## Requirements"
subsection listing any prerequisites and a "## Known issues" subsection that
includes the open-issues link (or repo issues URL) and brief notes, ensuring
both headings are formatted exactly as specified for class-based DSC resources.
- Around line 39-86: The class WSManClientConfig must include the
RunAsCredential DSC property and use the standard constructor signature that
accepts $PSScriptRoot and $ExcludeDscProperties and forwards them to the base;
add a [DscProperty()] [System.Management.Automation.PSCredential]
$RunAsCredential property, change the constructor to WSManClientConfig([string]
$PSScriptRoot, [string[]] $ExcludeDscProperties) : base($PSScriptRoot,
$ExcludeDscProperties) and keep the existing initialization ($this.ResourceURI
and $this.HasAuthContainer) inside that constructor.
In `@source/Examples/Resources/WSManClientConfig/1-WSManClientConfig_Config.ps1`:
- Around line 20-24: The comment-based help in 1-WSManClientConfig_Config.ps1
only contains .DESCRIPTION; add a .SYNOPSIS line that briefly describes the
script purpose and add at least one .EXAMPLE block showing how to run the script
and expected outcome; update the header comment so it includes .SYNOPSIS,
.DESCRIPTION (keep existing text), and an .EXAMPLE that demonstrates invocation
and result (e.g., setting WS-Man client settings), ensuring the help follows
PowerShell script help guidelines.
In `@tests/Integration/DSC_WSManClientConfig.Integration.Tests.ps1`:
- Around line 1-6: Add a complete comment-based help block to the top of the
DSC_WSManClientConfig.Integration.Tests.ps1 script by adding the missing
.DESCRIPTION, .EXAMPLE, .INPUTS, and .OUTPUTS sections; for tests with no inputs
or outputs set .INPUTS and .OUTPUTS to "None." and provide a short description
under .DESCRIPTION plus at least one simple usage under .EXAMPLE so the script
complies with the required header format.
- Around line 12-32: This integration test containing the BeforeDiscovery block
that imports DscResource.Test and invokes build.ps1 must be moved into the
Integration Resources tests folder and renamed to follow the
{ResourceName}.Integration.Tests.ps1 convention (so CI discovery and relative
build.ps1 path assumptions work); after moving, verify and update the build.ps1
relative invocation inside BeforeDiscovery and any module import paths to match
the new location.
- Around line 121-130: The BeforeAll block incorrectly calls
Initialize-TestEnvironment with -ResourceType 'Mof' for the class-based
DSC_WSManClientConfig resource; remove the Initialize-TestEnvironment call and
instead import the module directly by replacing that block with a BeforeAll that
sets the module name (e.g., $script:moduleName = 'WSManDsc') and calls
Import-Module -Name $script:moduleName -ErrorAction 'Stop'; leave
BeforeDiscovery as-is (it already imports DscResource.Test). Also update the
error message referenced on line 30 to mention 'noop' instead of 'build' to
match guidelines.
- Around line 180-210: Replace hardcoded 'localhost' with the dynamic machine
name from Get-ComputerName: set the NodeName value inside the
$configData.AllNodes[0] hashtable to the result of Get-ComputerName and set the
ComputerName entry in the $startDscConfigurationParameters hashtable to
Get-ComputerName as well (update the block constructing $configData and the
startDscConfigurationParameters assignment so both reference Get-ComputerName
instead of the literal 'localhost').
In `@tests/Unit/Classes/WSManClientConfig.Tests.ps1`:
- Around line 6-40: The test setup must match the template: update the
SuppressMessageAttribute invocation to include a non-empty justification string,
change the Import-Module call to include -ErrorAction 'Stop', and normalize the
module variable name to $script:moduleName (replace $script:dscModuleName) so
Import-Module and the $PSDefaultParameterValues entries
('InModuleScope:ModuleName', 'Mock:ModuleName', 'Should:ModuleName') reference
$script:moduleName; ensure all references (variable assignment, Import-Module,
and PSDefaultParameterValues) are updated consistently.
🧹 Nitpick comments (2)
source/en-US/WSManClientConfig.strings.psd1 (1)
2-6: Clarify the synopsis to match a DSC resource strings file.The header currently says it is for “classes (that are not a DSC resource)”, which is misleading here. Consider aligning it with the actual resource use.
📝 Suggested tweak
- localized strings for private functions, public command, and - classes (that are not a DSC resource). + localized strings for the WSManClientConfig DSC resource class.tests/Unit/Classes/WSManClientConfig.Tests.ps1 (1)
172-174: Update the context label to match the property under test.The context says “MaxConnections” but the test is about
URLPrefix. Consider renaming for clarity.✏️ Suggested edit
- Context 'When property ''MaxConnections'' has the wrong value' { + Context 'When property ''URLPrefix'' has the wrong value' {
| - Added `DscResource.Base` class. | ||
| - `WSManConfigBase` | ||
| - Base class for WSMan*Config Resources fixes [#44](https://github.com/dsccommunity/WSManDsc/issues/44). | ||
| - `WSManClientConfig` resource. Fixes [#41](https://github.com/dsccommunity/WSManDsc/issues/41). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap the new changelog entry to 80 chars.
Line 18 exceeds the 80-character guideline; wrap the link to the next line with a 2‑space indent.
💡 Suggested wrap
- - `WSManClientConfig` resource. Fixes [`#41`](https://github.com/dsccommunity/WSManDsc/issues/41).
+ - `WSManClientConfig` resource. Fixes
+ [`#41`](https://github.com/dsccommunity/WSManDsc/issues/41).📝 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.
| - `WSManClientConfig` resource. Fixes [#41](https://github.com/dsccommunity/WSManDsc/issues/41). | |
| - `WSManClientConfig` resource. Fixes | |
| [`#41`](https://github.com/dsccommunity/WSManDsc/issues/41). |
🤖 Prompt for AI Agents
In `@CHANGELOG.md` at line 18, The CHANGELOG.md entry containing
"`WSManClientConfig` resource. Fixes
[`#41`](https://github.com/dsccommunity/WSManDsc/issues/41)." is longer than 80
characters; update the line by wrapping the issue link onto the next line so the
first line stays ≤80 chars and indent the continued line with two spaces
(preserve the text exactly, just break after the sentence and place the link on
the next line with a two-space indent).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
80 chars is an old limit, I believe it's something like 120 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dan-hughes, according to the coding guidelines for this repository, there are different line length limits for different file types:
- Markdown files (
**/*.md): Wrap lines at word boundaries when over 80 characters (except tables/code blocks) - PowerShell files (
**/*.{ps1,psm1,psd1}): Limit lines to 120 characters
Since CHANGELOG.md is a Markdown file, the 80-character limit applies per the guidelines. The 120-character limit you're thinking of is specifically for PowerShell code files.
That said, if you prefer to keep the current formatting, that's your call as the maintainer! I'm just following the repository's documented guidelines.
🧠 Learnings used
Learnt from: johlju
Repo: dsccommunity/SqlServerDsc PR: 2343
File: source/Classes/004.DatabaseFileGroupSpec.ps1:5-34
Timestamp: 2025-11-22T17:36:09.703Z
Learning: The comment-based help requirements for ## Requirements and ## Known issues sections only apply to class-based DSC resources (classes decorated with [DscResource(...)]) in source/Classes/**/*.ps1, not to regular helper classes, DTO classes, or specification classes in the same directory.
Learnt from: CR
Repo: dsccommunity/SqlServerDsc PR: 0
File: .github/instructions/dsc-community-style-guidelines-class-resource.instructions.md:0-0
Timestamp: 2025-11-27T17:58:20.404Z
Learning: Applies to source/[cC]lasses/**/*.ps1 : DSC class-based resources must inherit from `ResourceBase` class from the DscResource.Base module
Learnt from: CR
Repo: dsccommunity/SqlServerDsc PR: 0
File: .github/instructions/dsc-community-style-guidelines-class-resource.instructions.md:0-0
Timestamp: 2025-11-27T17:58:20.404Z
Learning: Applies to source/[cC]lasses/**/*.ps1 : DSC class-based resources must include comment-based help with .DESCRIPTION section containing Requirements and Known issues sections with a link to all open issues on GitHub
Learnt from: Borgquite
Repo: dsccommunity/UpdateServicesDsc PR: 78
File: source/DSCResources/MSFT_UpdateServicesComputerTargetGroup/MSFT_UpdateServicesComputerTargetGroup.psm1:9-22
Timestamp: 2025-10-03T15:27:24.417Z
Learning: In the UpdateServicesDsc repository, MOF-based DSC resources follow a minimal comment-based help convention that includes only .SYNOPSIS and .PARAMETER sections. The .DESCRIPTION, .INPUTS, and .OUTPUTS sections are intentionally omitted, even though functions have [OutputType()] attributes. This is consistent across all existing DSC resources: MSFT_UpdateServicesServer, MSFT_UpdateServicesCleanup, and MSFT_UpdateServicesApprovalRule.
Learnt from: CR
Repo: dsccommunity/SqlServerDsc PR: 0
File: .github/instructions/dsc-community-style-guidelines-class-resource.instructions.md:0-0
Timestamp: 2025-11-27T17:58:20.404Z
Learning: Applies to source/[cC]lasses/020.*.ps1 : DSC class-based resources must use the file naming pattern `source/Classes/020.{ResourceName}.ps1`
Learnt from: CR
Repo: dsccommunity/SqlServerDsc PR: 0
File: .github/instructions/dsc-community-style-guidelines-class-resource.instructions.md:0-0
Timestamp: 2025-11-27T17:58:20.404Z
Learning: Applies to source/[cC]lasses/**/*.ps1 : DSC class-based resources must implement a required constructor with signature `ResourceName () : base ($PSScriptRoot)` and populate `$this.ExcludeDscProperties` array
Learnt from: CR
Repo: dsccommunity/SqlServerDsc PR: 0
File: .github/instructions/dsc-community-style-guidelines-powershell.instructions.md:0-0
Timestamp: 2025-12-24T18:09:36.332Z
Learning: Applies to **/*.{ps1,psm1,psd1} : Limit lines to 120 characters
source/Examples/Resources/WSManClientConfig/1-WSManClientConfig_Config.ps1
Show resolved
Hide resolved
johlju
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johlju reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 3 of 14 files reviewed, 8 unresolved discussions (waiting on @dan-hughes).
RequiredModules.psd1 line 24 at r3 (raw file):
AllowPrerelease = $true } }
Push the release so you can switch back.
Suggestion:
'DscResource.Test' = 'latest'
Pull Request (PR) description
Add class-based resource WSManClientConfig.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is