Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jul 10, 2025

  • Add OutputType to Get-SqlDscServerProtocol
  • Create new Get-SqlDscManagedComputerInstance command with pipeline parameter sets
  • Update Get-SqlDscServerProtocol to use new parameter sets (ByServerName, ByManagedComputerObject, ByManagedComputerInstanceObject)
  • Fix error handling to use $PSCmdlet.ThrowTerminatingError() instead of New-InvalidOperationException
  • Fix Get-SqlDscServerProtocolName switch statement to use break statements
  • Move All parameter check outside switch statement to satisfy PSReviewUnusedParameter rule
  • Fix file endings to have only one blank line
  • Update unit tests to reflect new parameter sets and commands
  • All tests passing for both new commands
  • Fix variable casing in Get-SqlDscManagedComputerInstance
  • Remove unnecessary variable assignment line
  • Update changelog with proper command descriptions
  • Remove unused ServerProtocol_InstanceNotFound string
  • Update comment-based help with all parameters documented

Changes Made

New Public Commands

  • Get-SqlDscManagedComputerInstance: New public command that returns managed computer server instances
    • Supports multiple parameter sets: ByServerName and ByManagedComputerObject
    • Accepts pipeline input from Get-SqlDscManagedComputer
    • Includes comprehensive error handling for missing instances
  • Enhanced Get-SqlDscServerProtocol: Updated with new parameter sets
    • ByServerName: Standard server name and instance name parameters
    • ByManagedComputerObject: Accepts ManagedComputer objects from pipeline
    • ByManagedComputerInstanceObject: Accepts ServerInstance objects from pipeline
    • Proper error handling using $PSCmdlet.ThrowTerminatingError()
  • Enhanced Get-SqlDscServerProtocolName: Fixed switch logic and parameter usage
    • Uses break statements in switch for better performance
    • All parameter check moved outside switch to satisfy PSReviewUnusedParameter rule

Technical Improvements

  • Error Handling: All terminating errors now use $PSCmdlet.ThrowTerminatingError() with proper error records
  • Code Organization: Uses existing public commands as recommended in guidelines
  • File Standards: All files end with exactly one blank line as required
  • Testing: Comprehensive unit tests for all parameter sets and functionality
  • Documentation: Complete comment-based help following project guidelines
  • Code Quality: Fixed variable casing and removed unnecessary code

Backward Compatibility

No breaking changes - All existing functionality continues to work unchanged


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.


This change is Reviewable

Copilot AI changed the title [WIP] Resolve this issue @dsccommunity/SqlServerDsc/issues/2104 and send in a PR in a new branch Add Get-SqlDscServerProtocol public command with CIM support Jul 10, 2025
Copilot AI requested a review from johlju July 10, 2025 16:27
@codecov
Copy link

codecov bot commented Jul 10, 2025

Codecov Report

❌ Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94%. Comparing base (48e61cb) to head (edef06e).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ource/Public/Get-SqlDscManagedComputerInstance.ps1 94% 1 Missing ⚠️
source/Public/Get-SqlDscServerProtocol.ps1 96% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #2108   +/-   ##
====================================
  Coverage    94%     94%           
====================================
  Files       145     148    +3     
  Lines      9091    9162   +71     
====================================
+ Hits       8617    8686   +69     
- Misses      474     476    +2     
Flag Coverage Δ
unit 94% <97%> (+<1%) ⬆️
Files with missing lines Coverage Δ
...dules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 96% <ø> (ø)
source/Public/Get-SqlDscServerProtocolName.ps1 100% <100%> (ø)
...ource/Public/Get-SqlDscManagedComputerInstance.ps1 94% <94%> (ø)
source/Public/Get-SqlDscServerProtocol.ps1 96% <96%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 19, 2025
@github-actions
Copy link

github-actions bot commented Aug 3, 2025

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added the abandoned The pull request has been abandoned. label Aug 3, 2025
@coderabbitai
Copy link

coderabbitai bot commented Aug 3, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds two new public cmdlets and extends another: Get-SqlDscManagedComputerInstance, Get-SqlDscServerProtocolName, and enhanced Get-SqlDscServerProtocol. Also updates localization strings, unit and integration tests, SMO test stubs, CI pipeline test list, and CHANGELOG entry.

Changes

Cohort / File(s) Summary
Public cmdlets: managed computer & protocol
source/Public/Get-SqlDscManagedComputerInstance.ps1, source/Public/Get-SqlDscServerProtocol.ps1, source/Public/Get-SqlDscServerProtocolName.ps1
Adds Get-SqlDscManagedComputerInstance and Get-SqlDscServerProtocolName; extends Get-SqlDscServerProtocol with multiple parameter sets, pipeline support, and optional all-protocol return. Implements parameter validation, verbose localization strings, output types, and terminating errors for not-found cases.
Localization resources
source/en-US/SqlServerDsc.strings.psd1
Adds localization keys for the three cmdlets (multiple messages). Note: duplicate insertions of some blocks appear in the diff.
CI pipeline
azure-pipelines.yml
Inserts three new integration test scripts into the integration test job ordering.
Integration tests
tests/Integration/Commands/Get-SqlDscManagedComputerInstance.Integration.Tests.ps1, .../Get-SqlDscServerProtocolName.Integration.Tests.ps1, .../Get-SqlDscServerProtocol.Integration.Tests.ps1
Adds integration test suites covering parameter sets, positive/negative cases, pipeline scenarios, SMO validations, and lifecycle setup/teardown for the new/updated cmdlets.
Integration test index
tests/Integration/Commands/README.md
Adds entries for the three new integration tests with run order, dependencies, and instance usage.
Unit tests
tests/Unit/Public/Get-SqlDscManagedComputerInstance.Tests.ps1, .../Get-SqlDscServerProtocolName.Tests.ps1, .../Get-SqlDscServerProtocol.Tests.ps1
Adds unit tests validating parameter sets, localization, behaviors, error paths, pipeline usage, and interaction with SMO stubs and mocks.
Test SMO stub
tests/Unit/Stubs/SMO.cs
Reworks SMO stubs: replaces single-item Item properties with string indexers and dictionary-backed storage; adds enumeration support for ServerInstanceCollection; updates Count semantics.
Common module docs
source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1
Adds deprecation note in .NOTES for Get-ProtocolNameProperties (no functional change).
Changelog
CHANGELOG.md
Documents the two new cmdlets and enhanced Get-SqlDscServerProtocol in Unreleased section.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant G as Get-SqlDscManagedComputerInstance
  participant M as Get-SqlDscManagedComputer
  participant W as SMO WMI (ManagedComputer/ServerInstance)

  U->>G: Invoke (ServerName, [InstanceName]) or (ManagedComputerObject, [InstanceName])
  alt ByServerName
    G->>M: Get-SqlDscManagedComputer -ServerName
    M-->>G: ManagedComputerObject
  else ByManagedComputerObject
    Note over G: Uses provided ManagedComputerObject
  end
  alt Specific instance
    G->>W: ManagedComputerObject.ServerInstances[InstanceName]
    alt Found
      W-->>G: ServerInstance
      G-->>U: ServerInstance
    else Not found
      G-->>U: Terminating error (SqlServerInstanceNotFound)
    end
  else All instances
    G->>W: Enumerate ManagedComputerObject.ServerInstances
    W-->>G: ServerInstance[]
    G-->>U: ServerInstance[]
  end
Loading
sequenceDiagram
  autonumber
  actor U as User
  participant P as Get-SqlDscServerProtocol
  participant G as Get-SqlDscManagedComputerInstance
  participant N as Get-SqlDscServerProtocolName
  participant W as SMO WMI (ServerInstance/ServerProtocol)

  U->>P: Invoke via (ServerName,InstanceName[,ProtocolName]) or (ManagedComputerObject,InstanceName[,ProtocolName]) or (ManagedComputerInstanceObject[,ProtocolName])
  alt Needs instance resolution
    P->>G: Resolve ServerInstance
    G-->>P: ServerInstance
  else Instance provided
    Note over P: Uses ManagedComputerInstanceObject directly
  end
  alt ProtocolName specified
    P->>W: ServerInstance.ServerProtocols[ProtocolName]
    alt Found
      W-->>P: ServerProtocol
      P-->>U: ServerProtocol
    else Not found
      P-->>U: Terminating error (SqlServerProtocolNotFound)
    end
  else No ProtocolName
    P->>N: Get-SqlDscServerProtocolName -All
    N-->>P: [TcpIp, NamedPipes, SharedMemory]
    loop For each mapping
      P->>W: ServerInstance.ServerProtocols[mapping.Name]
      W-->>P: ServerProtocol
    end
    P-->>U: ServerProtocol[]
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title is a short, single sentence that clearly names two of the new public cmdlets introduced by this PR (Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName) and therefore is directly related to the changeset; however the PR also adds Get-SqlDscManagedComputerInstance plus integration/unit tests and localization updates that the title omits.
Description Check ✅ Passed The pull request description is detailed and directly describes the changes in the diff, including the new commands, updated parameter sets, error-handling fixes, added tests, localization additions, and changelog updates, so it is on-topic and informative for reviewers.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Add new public commands" accurately and concisely describes the primary change in the PR — introduction of new public cmdlets (and related updates to an existing cmdlet) — and is relevant to the changeset provided. It is short, clear, and suitable for a quick scan of history.
Description Check ✅ Passed The PR description is directly related to the changeset and provides a detailed checklist and summary of the new commands, parameter-set updates, error-handling fixes, tests, and documentation changes, matching the raw summary and objectives. It is sufficiently focused for this lenient check.

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

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

@copilot there have be big changes to the instructions for this project, go through all instructions and follow them explicitly.

@johlju johlju removed abandoned The pull request has been abandoned. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Sep 12, 2025
Copilot AI changed the title Add Get-SqlDscServerProtocol public command with CIM support Add Get-SqlDscServerProtocol public command Sep 12, 2025
Copilot AI requested a review from johlju September 12, 2025 05:38
Copilot AI changed the title Add Get-SqlDscServerProtocol public command Add Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName public commands Sep 12, 2025
Copilot AI requested a review from johlju September 12, 2025 07:50
@johlju
Copy link
Member

johlju commented Sep 12, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 12, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@johlju johlju changed the title Add Get-SqlDscServerProtocol and Get-SqlDscServerProtocolName public commands Add new public commands Sep 12, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
source/Public/Get-SqlDscManagedComputerInstance.ps1 (2)

63-77: Add ValidateNotNull for the pipeline parameter.

Prevents null pipeline inputs and improves diagnostics.

         [Parameter(Mandatory = $true, ValueFromPipeline = $true, ParameterSetName = 'ByManagedComputerObject')]
+        [ValidateNotNull()]
         [Microsoft.SqlServer.Management.Smo.Wmi.ManagedComputer]
         $ManagedComputerObject

118-125: Optional: Stream objects explicitly.

Returning the collection is fine (it’s enumerable), but you can stream instances explicitly for clarity.

-            return $ManagedComputerObject.ServerInstances
+            foreach ($instance in $ManagedComputerObject.ServerInstances)
+            {
+                $instance
+            }
tests/Unit/Stubs/SMO.cs (1)

1463-1493: Make ServerProtocolCollection enumerable and field readonly.

Aligns stub behavior with SMO collections and simplifies foreach usage in tests.

-    public class ServerProtocolCollection
+    public class ServerProtocolCollection : System.Collections.IEnumerable
     {
-        // Properties
-        public System.Int32 Count { get { return protocols.Count; } }
+        // Properties
+        public System.Int32 Count { get { return protocols.Count; } }
         public System.Boolean IsSynchronized { get; set; }
         public System.Object SyncRoot { get; set; }

-        // Collection of protocols
-        private System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerProtocol> protocols = new System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerProtocol>(System.StringComparer.OrdinalIgnoreCase);
+        // Collection of protocols
+        private readonly System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerProtocol> protocols =
+            new System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerProtocol>(System.StringComparer.OrdinalIgnoreCase);

         // Indexer
         public Microsoft.SqlServer.Management.Smo.Wmi.ServerProtocol this[string name]
         {
             get
             {
                 if (protocols.ContainsKey(name))
                 {
                     return protocols[name];
                 }
                 return null;
             }
             set
             {
                 if (value == null)
                 {
                     protocols.Remove(name);
                 }
                 else
                 {
                     protocols[name] = value;
                 }
             }
         }
+
+        // IEnumerable implementation
+        public System.Collections.IEnumerator GetEnumerator()
+        {
+            return protocols.Values.GetEnumerator();
+        }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9dd30e3 and 922046f.

📒 Files selected for processing (11)
  • CHANGELOG.md (1 hunks)
  • source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 (1 hunks)
  • source/Public/Get-SqlDscManagedComputerInstance.ps1 (1 hunks)
  • source/Public/Get-SqlDscServerProtocol.ps1 (1 hunks)
  • source/en-US/SqlServerDsc.strings.psd1 (2 hunks)
  • tests/Integration/Commands/Get-SqlDscManagedComputerInstance.Integration.Tests.ps1 (1 hunks)
  • tests/Integration/Commands/Get-SqlDscServerProtocol.Integration.Tests.ps1 (1 hunks)
  • tests/Integration/Commands/Get-SqlDscServerProtocolName.Integration.Tests.ps1 (1 hunks)
  • tests/Unit/Public/Get-SqlDscManagedComputerInstance.Tests.ps1 (1 hunks)
  • tests/Unit/Public/Get-SqlDscServerProtocol.Tests.ps1 (1 hunks)
  • tests/Unit/Stubs/SMO.cs (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/Integration/Commands/Get-SqlDscServerProtocolName.Integration.Tests.ps1
  • source/Public/Get-SqlDscServerProtocol.ps1
  • tests/Integration/Commands/Get-SqlDscManagedComputerInstance.Integration.Tests.ps1
  • tests/Unit/Public/Get-SqlDscServerProtocol.Tests.ps1
  • tests/Integration/Commands/Get-SqlDscServerProtocol.Integration.Tests.ps1
  • tests/Unit/Public/Get-SqlDscManagedComputerInstance.Tests.ps1
  • source/en-US/SqlServerDsc.strings.psd1
🧰 Additional context used
📓 Path-based instructions (5)
**

⚙️ CodeRabbit configuration file

**: # DSC Community Guidelines

Terminology

  • Command: Public command
  • Function: Private function
  • Resource: DSC class-based resource

Build & Test Workflow

  • Run scripts in pwsh; always from repository root
  • Build before running tests: .\build.ps1 -Tasks build
  • Always run tests in new PowerShell session: Invoke-Pester -Path @({test paths}) -Output Detailed

File 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.ps1

Requirements

  • 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.md
  • tests/Unit/Stubs/SMO.cs
  • source/Public/Get-SqlDscManagedComputerInstance.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 MD013 rule 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
**/*.ps?(m|d)1

⚙️ CodeRabbit configuration file

**/*.ps?(m|d)1: # PowerShell Guidelines

Naming

  • 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.ps1 format (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 array

Hashtables

  • 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/Public/Get-SqlDscManagedComputerInstance.ps1
source/**/*.ps1

⚙️ CodeRabbit configuration file

source/**/*.ps1: # Localization Guidelines

Requirements

  • Localize all Write-Debug, Write-Verbose, Write-Error, Write-Warning and $PSCmdlet.ThrowTerminatingError() messages
  • Use localized string keys, not hardcoded strings
  • Assume $script:localizedData is available

String Files

  • Commands/functions: source/en-US/{MyModuleName}.strings.psd1
  • Class resources: source/en-US/{ResourceClassName}.strings.psd1

Key Naming Patterns

  • Format: Verb_FunctionName_Action (underscore separators), e.g. Get_Database_ConnectingToDatabase

String 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/Public/Get-SqlDscManagedComputerInstance.ps1
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/SqlServerDsc-guidelines.instructions.md:0-0
Timestamp: 2025-08-29T17:20:42.266Z
Learning: Applies to tests/Integration/**/*.ps1 : Use DSCSQLTEST (Database Engine), SSRS (Reporting Services), and PBIRS (Power BI Report Server) instance names in CI integration tests
📚 Learning: 2025-08-29T17:20:51.246Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-changelog.instructions.md:0-0
Timestamp: 2025-08-29T17:20:51.246Z
Learning: Applies to CHANGELOG.md : Describe notable changes briefly, with no more than 2 items per change type

Applied to files:

  • CHANGELOG.md
📚 Learning: 2025-08-29T17:20:42.266Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/SqlServerDsc-guidelines.instructions.md:0-0
Timestamp: 2025-08-29T17:20:42.266Z
Learning: Applies to **/*.{ps1,psm1} : Public PowerShell commands must use the naming format Verb-SqlDsc{Noun}

Applied to files:

  • CHANGELOG.md
📚 Learning: 2025-08-29T17:20:42.266Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/SqlServerDsc-guidelines.instructions.md:0-0
Timestamp: 2025-08-29T17:20:42.266Z
Learning: Applies to tests/Stubs/SMO.cs : Ensure all SMO types used by unit-tested classes/commands are properly stubbed in SMO.cs

Applied to files:

  • tests/Unit/Stubs/SMO.cs
📚 Learning: 2025-09-12T13:22:48.920Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-powershell.instructions.md:0-0
Timestamp: 2025-09-12T13:22:48.920Z
Learning: Applies to **/*.{ps1,psm1} : Use declared variables more than once (avoid single-use locals)

Applied to files:

  • source/Public/Get-SqlDscManagedComputerInstance.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). (2)
  • GitHub Check: dsccommunity.SqlServerDsc (Build Package Module)
  • GitHub Check: PSScriptAnalyzer
🔇 Additional comments (5)
CHANGELOG.md (1)

121-131: LGTM: Added entries are concise, in the correct section, and follow style.

The three bullets accurately describe the new/updated commands and fit the existing Keep a Changelog structure. No duplicates found in Unreleased.

source/Public/Get-SqlDscManagedComputerInstance.ps1 (3)

1-55: Help section looks solid and matches behavior.

Examples, INPUTS/OUTPUTS, and indentation follow guidelines; aligns with parameter sets and return types.


81-88: LGTM: ServerName path.

Uses localized verbose and resolves the ManagedComputer via the public command with named parameter.


96-116: LGTM: Specific-instance lookup with proper terminating error.

Indexing by name, localized message, ObjectNotFound category, and consistent ErrorId are all correct.

tests/Unit/Stubs/SMO.cs (1)

1522-1561: LGTM: Case-insensitive indexer, accurate Count, and IEnumerable for ServerInstanceCollection.

Matches expected SMO semantics and supports name-based lookup plus enumeration in tests.

@johlju
Copy link
Member

johlju commented Sep 13, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 13, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/Unit/Stubs/SMO.cs (3)

1477-1482: Micro-optimization: avoid double dictionary lookup in indexer getter.

Apply:

-                if (protocols.ContainsKey(name))
-                {
-                    return protocols[name];
-                }
-                return null;
+                Microsoft.SqlServer.Management.Smo.Wmi.ServerProtocol value;
+                return protocols.TryGetValue(name, out value) ? value : null;

1529-1568: Make instances dictionary readonly for consistency and intent.

Apply:

-        private System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance> instances = new System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance>(System.StringComparer.OrdinalIgnoreCase);
+        private readonly System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance> instances =
+            new System.Collections.Generic.Dictionary<string, Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance>(System.StringComparer.OrdinalIgnoreCase);

1544-1549: Micro-optimization: use TryGetValue in ServerInstanceCollection getter.

Apply:

-                if (instances.ContainsKey(name))
-                {
-                    return instances[name];
-                }
-                return null;
+                Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance value;
+                return instances.TryGetValue(name, out value) ? value : null;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 922046f and 8be8c6d.

📒 Files selected for processing (3)
  • tests/Integration/Commands/Get-SqlDscServerProtocol.Integration.Tests.ps1 (1 hunks)
  • tests/Integration/Commands/Get-SqlDscServerProtocolName.Integration.Tests.ps1 (1 hunks)
  • tests/Unit/Stubs/SMO.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/Integration/Commands/Get-SqlDscServerProtocolName.Integration.Tests.ps1
  • tests/Integration/Commands/Get-SqlDscServerProtocol.Integration.Tests.ps1
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

**: # DSC Community Guidelines

Terminology

  • Command: Public command
  • Function: Private function
  • Resource: DSC class-based resource

Build & Test Workflow

  • Run scripts in pwsh; always from repository root
  • Build before running tests: .\build.ps1 -Tasks build
  • Always run tests in new PowerShell session: Invoke-Pester -Path @({test paths}) -Output Detailed

File 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.ps1

Requirements

  • 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/Stubs/SMO.cs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/SqlServerDsc-guidelines.instructions.md:0-0
Timestamp: 2025-08-29T17:20:42.266Z
Learning: Applies to tests/Integration/**/*.ps1 : Use DSCSQLTEST (Database Engine), SSRS (Reporting Services), and PBIRS (Power BI Report Server) instance names in CI integration tests
📚 Learning: 2025-08-29T17:20:42.266Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/SqlServerDsc-guidelines.instructions.md:0-0
Timestamp: 2025-08-29T17:20:42.266Z
Learning: Applies to tests/Stubs/SMO.cs : Ensure all SMO types used by unit-tested classes/commands are properly stubbed in SMO.cs

Applied to files:

  • tests/Unit/Stubs/SMO.cs
⏰ 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). (2)
  • GitHub Check: dsccommunity.SqlServerDsc (Build Package Module)
  • GitHub Check: PSScriptAnalyzer
🔇 Additional comments (1)
tests/Unit/Stubs/SMO.cs (1)

1461-1501: Great alignment with prior feedback on ServerProtocolCollection.

Count is read-only, keys are case-insensitive, null setter removes entries, and enumeration returns values. Matches SMO-like semantics and unblocks tests.

@johlju johlju marked this pull request as ready for review September 13, 2025 07:41
@johlju johlju requested a review from a team as a code owner September 13, 2025 07:41
…ion 'Stop' to critical commands and setting default value for ServerName if not provided.
…restore the previous value directly after invocation without try-catch-finally.
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

@johlju reviewed 3 of 6 files at r2, 2 of 12 files at r6, 3 of 13 files at r7, 10 of 10 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @copilot)


source/Public/Get-SqlDscServerProtocol.ps1 line 130 at r8 (raw file):

            'ByServerName'
            {
                $serverInstance = Get-SqlDscManagedComputerInstance -ServerName $ServerName -InstanceName $InstanceName

Need to call ErrorAction.

Code quote:

$serverInstance = Get-SqlDscManagedComputerInstance -ServerName $ServerName -InstanceName $InstanceName

source/Public/Get-SqlDscServerProtocol.ps1 line 135 at r8 (raw file):

            'ByManagedComputerObject'
            {
                $serverInstance = $ManagedComputerObject | Get-SqlDscManagedComputerInstance -InstanceName $InstanceName

Need to call ErrorAction.

Code quote:

$serverInstance = $ManagedComputerObject | Get-SqlDscManagedComputerInstance -InstanceName $InstanceName

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

@johlju reviewed 2 of 10 files at r9, all commit messages.
Reviewable status: 11 of 19 files reviewed, 4 unresolved discussions (waiting on @copilot)


source/Public/Get-SqlDscServerProtocol.ps1 line 130 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Need to call ErrorAction.

Done


source/Public/Get-SqlDscServerProtocol.ps1 line 135 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Need to call ErrorAction.

Done

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

:lgtm:

@johlju reviewed 8 of 10 files at r9.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @copilot)

Copilot AI requested a review from johlju September 13, 2025 08:27
Copilot stopped work on behalf of johlju due to an error September 13, 2025 08:27
@johlju johlju merged commit 1440fce into main Sep 13, 2025
26 of 28 checks passed
@johlju johlju deleted the copilot/fix-573e03dc-fd5a-4b2c-bee5-e2c7a757794d branch September 13, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants