Add new commands to enable and disable SQL Server logins with comprehensive testing #2138
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
Co-authored-by: johlju <[email protected]>
Copilot
AI
changed the title
[WIP] Add new commands to enable and disable a SQL Server login based on https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.login. Also add a command to test if a login is enabled or not.
Add new commands to enable and disable SQL Server logins
Aug 17, 2025
Member
|
@copilot fix new line in all files as per guidelines. Also add integration tests. |
Co-authored-by: johlju <[email protected]>
Contributor
Author
Fixed newlines in all files and added comprehensive integration tests for all three new login commands. Integration tests are configured in Group 2 of the SQL Server pipeline stage. All unit tests continue to pass. Changes in commit 68f0c4c. |
Copilot
AI
changed the title
Add new commands to enable and disable SQL Server logins
Add new commands to enable and disable SQL Server logins with comprehensive testing
Aug 17, 2025
johlju
reviewed
Aug 17, 2025
Member
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Reviewed 1 of 9 files at r1, 12 of 12 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @copilot-swe-agent[bot])
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughAdds three public cmdlets to enable, disable, and test SQL Server login state, localized ShouldProcess strings, changelog and CI entries, a minor PowerShell style EOF wording change, and new unit and integration tests for the commands. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant Enable as Enable-SqlDscLogin
participant Disable as Disable-SqlDscLogin
participant TestCmd as Test-SqlDscIsLoginEnabled
participant Get as Get-SqlDscLogin
participant Login as SMO.Login
rect rgba(230,245,255,0.6)
note right of Enable: ServerObject/Name path resolves login
U->>Enable: Invoke -ServerObject/-Name [-Refresh] [-Force]
Enable->>Get: Resolve login (ErrorAction=Stop)
Get-->>Enable: Login
Enable->>Enable: ShouldProcess (localized)
Enable->>Login: Enable()
end
rect rgba(255,240,230,0.6)
note right of Disable: ServerObject/Name path resolves login
U->>Disable: Invoke -ServerObject/-Name [-Refresh] [-Force]
Disable->>Get: Resolve login (ErrorAction=Stop)
Get-->>Disable: Login
Disable->>Disable: ShouldProcess (localized)
Disable->>Login: Disable()
end
rect rgba(240,255,240,0.6)
note right of TestCmd: ServerObject/Name path resolves login and returns state
U->>TestCmd: Invoke -ServerObject/-Name [-Refresh]
TestCmd->>Get: Resolve login (ErrorAction=Stop)
Get-->>TestCmd: Login
TestCmd->>TestCmd: return (-not Login.IsDisabled)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ 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 (
|
johlju
reviewed
Aug 17, 2025
tests/Integration/Commands/Disable-SqlDscLogin.Integration.Tests.ps1
Outdated
Show resolved
Hide resolved
… guidelines Co-authored-by: johlju <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
====================================
Coverage 94% 94%
====================================
Files 113 116 +3
Lines 8231 8269 +38
====================================
+ Hits 7792 7830 +38
Misses 439 439
🚀 New features to boost your workflow:
|
Member
|
@copilot rebase this PR. |
Contributor
Author
Successfully rebased the PR onto the latest main branch. The rebase resolved merge conflicts in |
Copilot stopped work on behalf of
johlju due to an error
August 20, 2025 15:48
johlju
requested changes
Aug 23, 2025
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
Outdated
Show resolved
Hide resolved
… and test coverage Co-authored-by: johlju <[email protected]>
johlju
requested changes
Aug 23, 2025
… use Should -Invoke assertions, and remove redundant tests Co-authored-by: johlju <[email protected]>
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
38-39: Remove unrelated “Added … Get-/New-/Remove-…” sentence from Unreleased.These commands pre-exist and are not part of this PR. Listing them as “Added” in the Unreleased section is misleading and breaks Keep a Changelog semantics.
- - Added `Get-SqlDscLogin`, `Get-SqlDscRole`, `New-SqlDscLogin`, `New-SqlDscRole`, `Remove-SqlDscRole`, and `Remove-SqlDscLogin` commands for retrieving and managing SQL Server logins and roles with support for refresh, pipeline input, and ShouldProcess.
♻️ Duplicate comments (7)
source/Public/Enable-SqlDscLogin.ps1 (1)
80-84: Restore $ConfirmPreference after Force-based suppression.Setting $ConfirmPreference = 'None' is fine, but it should be restored even if an exception occurs to avoid leaking preference changes within the function scope. Wrap the ShouldProcess block in try/finally.
- if ($Force.IsPresent -and -not $Confirm) - { - $ConfirmPreference = 'None' - } + $oldConfirmPreference = $ConfirmPreference + if ($Force.IsPresent -and -not $Confirm) + { + $ConfirmPreference = 'None' + } @@ - if ($PSCmdlet.ShouldProcess($verboseDescriptionMessage, $verboseWarningMessage, $captionMessage)) - { - $LoginObject.Enable() - } + try + { + if ($PSCmdlet.ShouldProcess($verboseDescriptionMessage, $verboseWarningMessage, $captionMessage)) + { + $LoginObject.Enable() + } + } + finally + { + $ConfirmPreference = $oldConfirmPreference + }Also applies to: 102-106
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1 (1)
77-90: Also assert pipeline acceptance on pipeline-capable parameters.Add checks for ValueFromPipeline on ServerObject and LoginObject to guard against accidental binder changes.
It 'Should have ServerObject parameter as mandatory in ServerObject parameter set' { $parameterInfo = (Get-Command -Name 'Enable-SqlDscLogin').Parameters['ServerObject'] $parameterInfo.Attributes.Mandatory | Should -BeTrue + ($parameterInfo.Attributes.ValueFromPipeline -or $parameterInfo.Attributes.ValueFromPipelineByPropertyName) | Should -BeTrue } @@ It 'Should have LoginObject parameter as mandatory in LoginObject parameter set' { $parameterInfo = (Get-Command -Name 'Enable-SqlDscLogin').Parameters['LoginObject'] $parameterInfo.Attributes.Mandatory | Should -BeTrue + ($parameterInfo.Attributes.ValueFromPipeline -or $parameterInfo.Attributes.ValueFromPipelineByPropertyName) | Should -BeTrue }Also applies to: 87-90
source/Public/Test-SqlDscIsLoginEnabled.ps1 (1)
99-104: Simplify login resolution to a single assignment.Get-SqlDscLogin returns a single login. Drop the intermediate variable and assign directly to $LoginObject.
- # If this command does not find the login it will throw an exception. - $loginObjectArray = Get-SqlDscLogin @getSqlDscLoginParameters - - # Pick the only object in the array. - $LoginObject = $loginObjectArray + # If this command does not find the login it will throw an exception. + $LoginObject = Get-SqlDscLogin @getSqlDscLoginParameterstests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1 (1)
166-168: Remove -ExpectedMessage in integration testsIntegration tests must not assert specific error messages. Assert only that an error is thrown.
Apply this diff:
- { Test-SqlDscIsLoginEnabled -ServerObject $script:serverObject -Name 'NonExistentLogin' -ErrorAction 'Stop' } | - Should -Throw -ExpectedMessage 'There is no login with the name ''NonExistentLogin''.' + { Test-SqlDscIsLoginEnabled -ServerObject $script:serverObject -Name 'NonExistentLogin' -ErrorAction 'Stop' } | + Should -Throwtests/Unit/Public/Disable-SqlDscLogin.Tests.ps1 (2)
172-178: Remove unused tracking variable
$script:mockGetSqlDscLoginWasRunis declared but unused;Should -Invoke ... -Times 0already covers the assertion.- $script:mockMethodDisableWasRun = 0 - $script:mockGetSqlDscLoginWasRun = 0 + $script:mockMethodDisableWasRun = 0
77-90: Add ValueFromPipeline attribute assertionsPer our unit test pattern for public commands, also assert ValueFromPipeline is true for ServerObject and LoginObject.
Apply this diff to add two tests:
It 'Should have ServerObject parameter as mandatory in ServerObject parameter set' { $parameterInfo = (Get-Command -Name 'Disable-SqlDscLogin').Parameters['ServerObject'] $parameterInfo.Attributes.Mandatory | Should -BeTrue } It 'Should have Name parameter as mandatory in ServerObject parameter set' { $parameterInfo = (Get-Command -Name 'Disable-SqlDscLogin').Parameters['Name'] $parameterInfo.Attributes.Mandatory | Should -BeTrue } It 'Should have LoginObject parameter as mandatory in LoginObject parameter set' { $parameterInfo = (Get-Command -Name 'Disable-SqlDscLogin').Parameters['LoginObject'] $parameterInfo.Attributes.Mandatory | Should -BeTrue } + + It 'Should accept ServerObject from pipeline in ServerObject parameter set' { + $parameterInfo = (Get-Command -Name 'Disable-SqlDscLogin').Parameters['ServerObject'] + $parameterInfo.Attributes.ValueFromPipeline | Should -BeTrue + } + + It 'Should accept LoginObject from pipeline in LoginObject parameter set' { + $parameterInfo = (Get-Command -Name 'Disable-SqlDscLogin').Parameters['LoginObject'] + $parameterInfo.Attributes.ValueFromPipeline | Should -BeTrue + }source/Public/Disable-SqlDscLogin.ps1 (1)
49-56: Align .OUTPUTS and OutputType; OutputType missing typeHelp says “None.” and
[OutputType()]is empty. Use[System.Void]consistently in both.Apply this diff:
@@ - .OUTPUTS - None. + .OUTPUTS + [System.Void] + + This command does not produce output. @@ - [OutputType()] + [OutputType([System.Void])]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (8)
CHANGELOG.md(1 hunks)source/Public/Disable-SqlDscLogin.ps1(1 hunks)source/Public/Enable-SqlDscLogin.ps1(1 hunks)source/Public/Test-SqlDscIsLoginEnabled.ps1(1 hunks)tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1(1 hunks)tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1(1 hunks)tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1(1 hunks)tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run project scripts in PowerShell from repository root
- Build after source changes:
.\build.ps1 -Tasks build- Test workflow: Build →
Invoke-Pester -Path @('<test paths>') -Output Detailed- New session required after class changes
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.ps1Requirements
- Always update CHANGELOG.md Unreleased section
- Localize all strings using 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/Public/Disable-SqlDscLogin.ps1tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1CHANGELOG.mdtests/Unit/Public/Disable-SqlDscLogin.Tests.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1source/Public/Enable-SqlDscLogin.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: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:
@{}- Multi-line: 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: List each return type (one per line) with a 1‑line description. Must match both [OutputType()] and actual ...
Files:
source/Public/Disable-SqlDscLogin.ps1tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1source/Public/Enable-SqlDscLogin.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/SqlServerDsc.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming
- Format:
FunctionName_Description(underscore separators)- Example:
Get_SqlDscDatabase_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/Public/Disable-SqlDscLogin.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1source/Public/Enable-SqlDscLogin.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
- One
Describeblock per file matching the tested entity name- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test
Write-Verbose,Write-Debug, or parameter binding behavior- Pass all mandatory parameters to avoid prompts
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- 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,AfterEachItdescriptions start with 'Should'Contextdescriptions start with 'When'- Mock variables prefix: 'mock'
- Prefer
-BeTrue/-BeFalseover-Be $true/-Be $false- No
Should -Not -Throw- invoke commands directlyFile 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)-ForEachallowed onContextandItblocks- Keep scope close to usage context
Best Practices
- Assign unused return objects to
$null- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Cover all scenarios and code paths
- Use
BeforeEachandAfterEachsparingly- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cyc...
Files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.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).Required 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:dscModuleName = 'SqlServerDsc' Import-Module -Name $script:dscModuleName -Force -ErrorAction 'Stop' }
Files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- 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_number>
- No empty lines between list items in same section
- Do not add item if there are already an existing item for the same change
Files:
CHANGELOG.md
🧠 Learnings (31)
📓 Common learnings
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/en-US/SqlServerDsc.strings.psd1 : All localized strings for public commands and private functions must be added to source/en-US/SqlServerDsc.strings.psd1 following existing key patterns
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/Public/*.ps1 : Public commands must localize Write-Debug/Verbose/Error/Warning and other messages using keys from source/en-US/SqlServerDsc.strings.psd1; keys prefixed with function name using underscores; data available as $script:localizedData
📚 Learning: 2025-08-16T13:15:17.108Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:27-38
Timestamp: 2025-08-16T13:15:17.108Z
Learning: For PowerShell comment-based help .OUTPUTS sections: Each distinct return type should have its own .OUTPUTS entry with a description of when that type is returned, even if the underlying implementation uses array casting. This follows official PowerShell documentation standards where you list each return type on its own line under .OUTPUTS with clear descriptions of the conditions for each type.
Applied to files:
source/Public/Disable-SqlDscLogin.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-16T13:15:17.108Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:27-38
Timestamp: 2025-08-16T13:15:17.108Z
Learning: For PowerShell comment-based help in .OUTPUTS sections: Each distinct type that can be returned should have its own .OUTPUTS entry, even if the implementation technically always returns an array. This follows official PowerShell documentation standards and helps users understand what to expect in different usage scenarios.
Applied to files:
source/Public/Disable-SqlDscLogin.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-18T13:50:53.789Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2132
File: tests/Unit/Public/New-SqlDscLogin.Tests.ps1:0-0
Timestamp: 2025-08-18T13:50:53.789Z
Learning: In SqlServerDsc unit tests, SMO stub objects can be used to verify method calls like Create() on Login objects by adding mock verifications with Should -Invoke, providing more robust testing than just checking for no exceptions.
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1source/Public/Enable-SqlDscLogin.ps1
📚 Learning: 2025-08-17T10:13:30.079Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/Public/Remove-SqlDscLogin.ps1:104-108
Timestamp: 2025-08-17T10:13:30.079Z
Learning: In SqlServerDsc unit tests, SMO object stubs (like Login objects) should have properly mocked Parent properties with correct Server stub types and valid InstanceName values, rather than handling null Parent scenarios in production code.
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/Public/*.Tests.ps1 : Public command unit tests must include tests to validate parameter properties (e.g., Mandatory, ValueFromPipeline)
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/Public/*.Tests.ps1 : Public command unit tests must include parameter set validation using the provided template/pattern
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-16T13:22:15.230Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: tests/Unit/Public/Get-SqlDscLogin.Tests.ps1:78-93
Timestamp: 2025-08-16T13:22:15.230Z
Learning: In PowerShell unit tests for parameter validation in SqlServerDsc, accessing parameter attributes directly like `$cmd.Parameters['ParameterName'].Attributes.Mandatory` works correctly because PowerShell automatically iterates through the array and returns the property values. Additional filtering to ParameterAttribute is not necessary when the parameter structure is known and controlled.
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private}/*.ps1 : Every parameter in public commands and private functions must include a [Parameter()] attribute
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private}/*.ps1 : Only mandatory parameters should have [Parameter(Mandatory = $true)]; non-mandatory parameters must not set Mandatory
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T10:59:03.955Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/Public/Remove-SqlDscLogin.ps1:59-77
Timestamp: 2025-08-17T10:59:03.955Z
Learning: In PowerShell, mandatory parameters with [Parameter(Mandatory = $true)] automatically guard against $null values and don't require additional [ValidateNotNull()] or [ValidateNotNullOrEmpty()] attributes, as these validations are redundant with the mandatory parameter behavior.
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private}/*.ps1 : For parameters, place attributes, datatype, and name on separate lines; separate parameters by a single blank line
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T10:15:48.194Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: tests/Unit/Public/Remove-SqlDscLogin.Tests.ps1:36-39
Timestamp: 2025-08-17T10:15:48.194Z
Learning: Public command unit tests guideline: Never use InModuleScope unless accessing localized strings from $script:localizedData. PSDefaultParameterValues for InModuleScope should be kept in public command tests to support localized string retrieval when necessary.
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Use localized strings in tests only when necessary, retrieving values from $script:localizedData inside InModuleScope
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/**/*.Tests.ps1 : Never test, mock, or use Should -Invoke for Write-Verbose or Write-Debug in unit tests
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Test code must not exist outside of a Describe block
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Each scenario/path should be in Context blocks starting with 'When'; It block descriptions should start with 'Should' and must call the subject; results and assertions stay within the same It; BeforeAll/BeforeEach must not call the subject
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/**/*.Tests.ps1 : Do not use Should -Not -Throw; invoke the command directly
Applied to files:
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Include the standard integration test setup block (BeforeDiscovery/BeforeAll with module loading) before the first Describe block
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to azure-pipelines.yml : Add each public command integration test script to a group within the Integration_Test_Commands_SqlServer stage in ./azure-pipelines.yml, choosing the appropriate group
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Integration tests must exist for all public commands, not mock any commands, be placed at tests/Integration/Commands, and be named <Command>.Integration.Tests.ps1
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T10:48:15.384Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/suffix.ps1:24-24
Timestamp: 2025-08-17T10:48:15.384Z
Learning: In source/suffix.ps1, the Write-Verbose message in the catch block for Import-SqlDscPreferredModule does not need localization because the exception message from Import-SqlDscPreferredModule is already localized by that command, making it an edge case exception to the localization guidelines.
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : In CI, integration tests must use Get-ComputerName when the computer name is needed
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Do not test parameter validation
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/Public/*.Tests.ps1 : Unit tests for public commands must be placed in tests/Unit/Public and named <Command>.Tests.ps1
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/Public/*-SqlDsc*.ps1 : All public command names must have the noun prefixed with 'SqlDsc' (Verb-SqlDscNoun)
Applied to files:
CHANGELOG.md
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : All tests must use Pester and Pester v5.0 syntax
Applied to files:
tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private,Classes}/*.ps1 : Comment-based help .DESCRIPTION text must be descriptive and > 40 characters; .SYNOPSIS should be short; include at least one example
Applied to files:
source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private,Classes}/*.ps1 : Comment-based help must precede each function/class with a comment block using at least .SYNOPSIS, .DESCRIPTION, .PARAMETER, .EXAMPLE, .NOTES
Applied to files:
source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-18T10:44:38.990Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2132
File: source/Public/New-SqlDscLogin.ps1:254-257
Timestamp: 2025-08-18T10:44:38.990Z
Learning: When implementing Force and Confirm parameter interaction in PowerShell functions with SupportsShouldProcess, use the pattern `if ($Force.IsPresent -and -not $Confirm)` to suppress confirmations. This ensures that explicit `-Confirm:$true` is respected even when `-Force` is present, preserving user intent when they explicitly request confirmation alongside Force.
Applied to files:
source/Public/Enable-SqlDscLogin.ps1
📚 Learning: 2025-08-17T10:03:59.993Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/Public/Remove-SqlDscLogin.ps1:83-86
Timestamp: 2025-08-17T10:03:59.993Z
Learning: In PowerShell functions with SupportsShouldProcess, the $Confirm automatic variable defaults to $false when no -Confirm parameter is passed, and only becomes $true when -Confirm or -Confirm:$true is explicitly passed. The logic `if ($Force.IsPresent -and -not $Confirm)` correctly handles both cases where no -Confirm is passed and where -Confirm:$false is explicitly passed.
Applied to files:
source/Public/Enable-SqlDscLogin.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). (6)
- GitHub Check: PSScriptAnalyzer
- GitHub Check: PSScriptAnalyzer
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Unit)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Determine DSC Resource Test Requirements)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test HQRM)
- GitHub Check: dsccommunity.SqlServerDsc (Build Package Module)
🔇 Additional comments (15)
source/Public/Enable-SqlDscLogin.ps1 (2)
1-49: Comment-based help looks solid.SYNOPSIS/DESCRIPTION/INPUTS/OUTPUTS/EXAMPLES are present, parameter docs are clear, and examples cover both parameter sets including pipeline usage. Nice.
98-101: Localized string keys verifiedThe
Login_Enable_ShouldProcessVerboseDescription,Login_Enable_ShouldProcessVerboseWarning, andLogin_Enable_ShouldProcessCaptionentries are present insource/en-US/SqlServerDsc.strings.psd1(lines 78–81), matching the established key format. No further action needed.tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1 (1)
52-72: Good parameter-set surface verification.The test asserts both parameter-set names and their string representations. This guards binder regressions.
tests/Unit/Public/Enable-SqlDscLogin.Tests.ps1 (2)
55-75: Solid parameter-set validation for ShouldProcess-capable command.Asserting both parameter sets with [-WhatIf] [-Confirm] in the lists is helpful for catching ShouldProcess wiring regressions.
184-191: Great: Asserting Get-SqlDscLogin is not used in LoginObject set.This ensures the code path does not perform unnecessary lookups when a Login object is provided.
source/Public/Test-SqlDscIsLoginEnabled.ps1 (1)
24-37: INPUTS/OUTPUTS help is clear.Pipeline-accepted types and the Boolean output semantics are documented. Good.
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1 (4)
4-24: Integration setup block is correct and follows project templateBeforeDiscovery/Import logic is aligned with the repository’s integration test guidelines. Good job.
26-30: Module import pattern looks goodImporting SqlServerDsc in BeforeAll with -Force and -ErrorAction Stop is consistent with other integration tests.
57-112: ServerObject parameter set coverage is comprehensiveCovers enabled/disabled, Refresh, and pipeline input; asserts boolean return type. Looks solid.
114-162: LoginObject parameter set coverage is comprehensiveCovers enabled/disabled and pipeline input; asserts boolean return type. Looks solid.
tests/Unit/Public/Disable-SqlDscLogin.Tests.ps1 (3)
55-75: Parameter set discovery tests look goodValidates parameter set names and text forms. Nice parity with other cmdlets.
136-150: Not-found path is covered correctlyAsserting a terminating error and verifying Get-SqlDscLogin invocation is the right approach.
184-191: Good negative assertion for unintended lookupAsserting that Get-SqlDscLogin was not invoked when a LoginObject is provided protects against regressions.
source/Public/Disable-SqlDscLogin.ps1 (2)
100-105: Verified ShouldProcess localization in Disable-SqlDscLogin.ps1I confirmed that all three localization keys (
Login_Disable_ShouldProcessVerboseDescription,Login_Disable_ShouldProcessVerboseWarning, andLogin_Disable_ShouldProcessCaption) are defined insource/en-US/SqlServerDsc.strings.psd1(lines 84–87). TheShouldProcesscalls are properly wired to use these localized strings—no further changes needed.
82-85: Fix Force/Confirm handling
$Confirmis not a bound parameter; checking it is unreliable. Use$PSBoundParameters.ContainsKey('Confirm')or just honor-Forceunconditionally.Apply one of these diffs (preferred option A):
Option A (honor Force unless -Confirm explicitly provided):
- if ($Force.IsPresent -and -not $Confirm) + if ($Force.IsPresent -and -not $PSBoundParameters.ContainsKey('Confirm')) { $ConfirmPreference = 'None' }Option B (simpler; Force always suppresses confirm):
- if ($Force.IsPresent -and -not $Confirm) + if ($Force.IsPresent) { $ConfirmPreference = 'None' }⛔ Skipped due to learnings
Learnt from: johlju PR: dsccommunity/SqlServerDsc#2132 File: source/Public/New-SqlDscLogin.ps1:254-257 Timestamp: 2025-08-18T10:44:38.990Z Learning: When implementing Force and Confirm parameter interaction in PowerShell functions with SupportsShouldProcess, use the pattern `if ($Force.IsPresent -and -not $Confirm)` to suppress confirmations. This ensures that explicit `-Confirm:$true` is respected even when `-Force` is present, preserving user intent when they explicitly request confirmation alongside Force.Learnt from: johlju PR: dsccommunity/SqlServerDsc#2136 File: source/Public/Remove-SqlDscLogin.ps1:83-86 Timestamp: 2025-08-17T10:03:59.993Z Learning: In PowerShell functions with SupportsShouldProcess, the $Confirm automatic variable defaults to $false when no -Confirm parameter is passed, and only becomes $true when -Confirm or -Confirm:$true is explicitly passed. The logic `if ($Force.IsPresent -and -not $Confirm)` correctly handles both cases where no -Confirm is passed and where -Confirm:$false is explicitly passed.
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
Show resolved
Hide resolved
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…on.Tests.ps1 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
johlju
requested changes
Aug 23, 2025
…Test-SqlDscIsLoginEnabled unit tests Co-authored-by: johlju <[email protected]>
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
source/Public/Disable-SqlDscLogin.ps1 (1)
60-62: Fix OutputType/help mismatch: declare [System.Void] and update .OUTPUTSThe function returns no output but
[OutputType()]is empty and help says “None.” Align both with[System.Void]per guidelines.Apply:
- .OUTPUTS - None. + .OUTPUTS + [System.Void] + + This command does not produce output. @@ - [OutputType()] + [OutputType([System.Void])]Also applies to: 66-66
CHANGELOG.md (1)
36-36: Correct implementation reference: uses Get-SqlDscLogin, not Test-SqlDscIsLoginThe new command resolves the login via
Get-SqlDscLoginand evaluatesIsDisabled; it does not callTest-SqlDscIsLogin. Update to avoid misleading users.- - Uses `Test-SqlDscIsLogin` command for login validation following module patterns. + - Resolves the login object using `Get-SqlDscLogin` and evaluates its enabled state.source/Public/Test-SqlDscIsLoginEnabled.ps1 (1)
101-106: Remove misleading “array” naming/comment; assign directly
Get-SqlDscLogin -Namereturns a single object. The temporary “array” variable and comment are misleading.- # If this command does not find the login it will throw an exception. - $loginObjectArray = Get-SqlDscLogin @getSqlDscLoginParameters - - # Pick the only object in the array. - $LoginObject = $loginObjectArray + # If this command does not find the login it will throw an exception. + $LoginObject = Get-SqlDscLogin @getSqlDscLoginParameterstests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1 (2)
57-112: Reduce repetition by establishing state in Context-level BeforeEach (optional)Each It sets the login enabled/disabled. Factor into
BeforeEachper Context to DRY setup and guarantee state even if tests re-order.Example for the “ServerObject” context:
- Context 'When testing login state using ServerObject parameter set' { - It 'Should return True when login is enabled' { - Enable-SqlDscLogin -ServerObject $script:serverObject -Name $script:testLoginName -Force -ErrorAction 'Stop' + Context 'When testing login state using ServerObject parameter set' { + BeforeEach { + Enable-SqlDscLogin -ServerObject $script:serverObject -Name $script:testLoginName -Force -ErrorAction 'Stop' + } + It 'Should return True when login is enabled' { $result = Test-SqlDscIsLoginEnabled -ServerObject $script:serverObject -Name $script:testLoginName $result | Should -BeOfType [System.Boolean] $result | Should -BeTrue } @@ - It 'Should return False when login is disabled' { - Disable-SqlDscLogin -ServerObject $script:serverObject -Name $script:testLoginName -Force -ErrorAction 'Stop' + It 'Should return False when login is disabled' { + Disable-SqlDscLogin -ServerObject $script:serverObject -Name $script:testLoginName -Force -ErrorAction 'Stop' $result = Test-SqlDscIsLoginEnabled -ServerObject $script:serverObject -Name $script:testLoginName $result | Should -BeOfType [System.Boolean] $result | Should -BeFalse }Also applies to: 114-162
164-168: Avoid asserting exact error message in integration testsGuidelines recommend not using
-ExpectedMessage. Assert throw only.- { Test-SqlDscIsLoginEnabled -ServerObject $script:serverObject -Name 'NonExistentLogin' -ErrorAction 'Stop' } | - Should -Throw -ExpectedMessage 'There is no login with the name ''NonExistentLogin''.' + { Test-SqlDscIsLoginEnabled -ServerObject $script:serverObject -Name 'NonExistentLogin' -ErrorAction 'Stop' } | + Should -Throw
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (5)
CHANGELOG.md(1 hunks)source/Public/Disable-SqlDscLogin.ps1(1 hunks)source/Public/Test-SqlDscIsLoginEnabled.ps1(1 hunks)tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1(1 hunks)tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run project scripts in PowerShell from repository root
- Build after source changes:
.\build.ps1 -Tasks build- Test workflow: Build →
Invoke-Pester -Path @('<test paths>') -Output Detailed- New session required after class changes
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.ps1Requirements
- Always update CHANGELOG.md Unreleased section
- Localize all strings using 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/Public/Test-SqlDscIsLoginEnabled.Tests.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1source/Public/Disable-SqlDscLogin.ps1CHANGELOG.md
**/*.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: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:
@{}- Multi-line: 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: List each return type (one per line) with a 1‑line description. Must match both [OutputType()] and actual ...
Files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1source/Public/Disable-SqlDscLogin.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
- One
Describeblock per file matching the tested entity name- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test
Write-Verbose,Write-Debug, or parameter binding behavior- Pass all mandatory parameters to avoid prompts
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- 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,AfterEachItdescriptions start with 'Should'Contextdescriptions start with 'When'- Mock variables prefix: 'mock'
- Prefer
-BeTrue/-BeFalseover-Be $true/-Be $false- No
Should -Not -Throw- invoke commands directlyFile 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)-ForEachallowed onContextandItblocks- Keep scope close to usage context
Best Practices
- Assign unused return objects to
$null- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Cover all scenarios and code paths
- Use
BeforeEachandAfterEachsparingly- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cyc...
Files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.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/SqlServerDsc.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming
- Format:
FunctionName_Description(underscore separators)- Example:
Get_SqlDscDatabase_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/Public/Test-SqlDscIsLoginEnabled.ps1source/Public/Disable-SqlDscLogin.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).Required 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:dscModuleName = 'SqlServerDsc' Import-Module -Name $script:dscModuleName -Force -ErrorAction 'Stop' }
Files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- 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_number>
- No empty lines between list items in same section
- Do not add item if there are already an existing item for the same change
Files:
CHANGELOG.md
🧠 Learnings (29)
📚 Learning: 2025-08-17T10:13:30.079Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/Public/Remove-SqlDscLogin.ps1:104-108
Timestamp: 2025-08-17T10:13:30.079Z
Learning: In SqlServerDsc unit tests, SMO object stubs (like Login objects) should have properly mocked Parent properties with correct Server stub types and valid InstanceName values, rather than handling null Parent scenarios in production code.
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-18T13:50:53.789Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2132
File: tests/Unit/Public/New-SqlDscLogin.Tests.ps1:0-0
Timestamp: 2025-08-18T13:50:53.789Z
Learning: In SqlServerDsc unit tests, SMO stub objects can be used to verify method calls like Create() on Login objects by adding mock verifications with Should -Invoke, providing more robust testing than just checking for no exceptions.
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/Public/*.Tests.ps1 : Public command unit tests must include tests to validate parameter properties (e.g., Mandatory, ValueFromPipeline)
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/Public/*.Tests.ps1 : Public command unit tests must include parameter set validation using the provided template/pattern
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-16T13:22:15.230Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: tests/Unit/Public/Get-SqlDscLogin.Tests.ps1:78-93
Timestamp: 2025-08-16T13:22:15.230Z
Learning: In PowerShell unit tests for parameter validation in SqlServerDsc, accessing parameter attributes directly like `$cmd.Parameters['ParameterName'].Attributes.Mandatory` works correctly because PowerShell automatically iterates through the array and returns the property values. Additional filtering to ParameterAttribute is not necessary when the parameter structure is known and controlled.
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private}/*.ps1 : Every parameter in public commands and private functions must include a [Parameter()] attribute
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private}/*.ps1 : Only mandatory parameters should have [Parameter(Mandatory = $true)]; non-mandatory parameters must not set Mandatory
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T10:15:48.194Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: tests/Unit/Public/Remove-SqlDscLogin.Tests.ps1:36-39
Timestamp: 2025-08-17T10:15:48.194Z
Learning: Public command unit tests guideline: Never use InModuleScope unless accessing localized strings from $script:localizedData. PSDefaultParameterValues for InModuleScope should be kept in public command tests to support localized string retrieval when necessary.
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private}/*.ps1 : For parameters, place attributes, datatype, and name on separate lines; separate parameters by a single blank line
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Do not test parameter validation
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T10:59:03.955Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/Public/Remove-SqlDscLogin.ps1:59-77
Timestamp: 2025-08-17T10:59:03.955Z
Learning: In PowerShell, mandatory parameters with [Parameter(Mandatory = $true)] automatically guard against $null values and don't require additional [ValidateNotNull()] or [ValidateNotNullOrEmpty()] attributes, as these validations are redundant with the mandatory parameter behavior.
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/Public/*.Tests.ps1 : Unit tests for public commands must be placed in tests/Unit/Public and named <Command>.Tests.ps1
Applied to files:
tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1
📚 Learning: 2025-08-16T13:15:17.108Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:27-38
Timestamp: 2025-08-16T13:15:17.108Z
Learning: For PowerShell comment-based help .OUTPUTS sections: Each distinct return type should have its own .OUTPUTS entry with a description of when that type is returned, even if the underlying implementation uses array casting. This follows official PowerShell documentation standards where you list each return type on its own line under .OUTPUTS with clear descriptions of the conditions for each type.
Applied to files:
source/Public/Test-SqlDscIsLoginEnabled.ps1source/Public/Disable-SqlDscLogin.ps1
📚 Learning: 2025-08-16T13:15:17.108Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2134
File: source/Public/Get-SqlDscLogin.ps1:27-38
Timestamp: 2025-08-16T13:15:17.108Z
Learning: For PowerShell comment-based help in .OUTPUTS sections: Each distinct type that can be returned should have its own .OUTPUTS entry, even if the implementation technically always returns an array. This follows official PowerShell documentation standards and helps users understand what to expect in different usage scenarios.
Applied to files:
source/Public/Test-SqlDscIsLoginEnabled.ps1source/Public/Disable-SqlDscLogin.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private,Classes}/*.ps1 : Comment-based help .DESCRIPTION text must be descriptive and > 40 characters; .SYNOPSIS should be short; include at least one example
Applied to files:
source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/{Public,Private,Classes}/*.ps1 : Comment-based help must precede each function/class with a comment block using at least .SYNOPSIS, .DESCRIPTION, .PARAMETER, .EXAMPLE, .NOTES
Applied to files:
source/Public/Test-SqlDscIsLoginEnabled.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Include the standard integration test setup block (BeforeDiscovery/BeforeAll with module loading) before the first Describe block
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to azure-pipelines.yml : Add each public command integration test script to a group within the Integration_Test_Commands_SqlServer stage in ./azure-pipelines.yml, choosing the appropriate group
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Integration tests must exist for all public commands, not mock any commands, be placed at tests/Integration/Commands, and be named <Command>.Integration.Tests.ps1
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/**/*.Tests.ps1 : Do not use Should -Not -Throw; invoke the command directly
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T10:48:15.384Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2136
File: source/suffix.ps1:24-24
Timestamp: 2025-08-17T10:48:15.384Z
Learning: In source/suffix.ps1, the Write-Verbose message in the catch block for Import-SqlDscPreferredModule does not need localization because the exception message from Import-SqlDscPreferredModule is already localized by that command, making it an edge case exception to the localization guidelines.
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : In CI, integration tests must use Get-ComputerName when the computer name is needed
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/**/*.Tests.ps1 : Never test, mock, or use Should -Invoke for Write-Verbose or Write-Debug in unit tests
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Use BeforeAll/BeforeEach/AfterAll/AfterEach inside Context, placed near the It blocks using them; duplication across contexts is acceptable; AfterAll can clean up; use BeforeEach/AfterEach sparingly
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Each scenario/path should be in Context blocks starting with 'When'; It block descriptions should start with 'Should' and must call the subject; results and assertions stay within the same It; BeforeAll/BeforeEach must not call the subject
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/Unit/**/*.Tests.ps1 : Include the standard unit test setup block (BeforeDiscovery/BeforeAll/AfterAll with module loading and PSDefaultParameterValues) before the Describe block
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to tests/**/*.ps1 : Test code must not exist outside of a Describe block
Applied to files:
tests/Integration/Commands/Test-SqlDscIsLoginEnabled.Integration.Tests.ps1
📚 Learning: 2025-08-18T10:44:38.990Z
Learnt from: johlju
PR: dsccommunity/SqlServerDsc#2132
File: source/Public/New-SqlDscLogin.ps1:254-257
Timestamp: 2025-08-18T10:44:38.990Z
Learning: When implementing Force and Confirm parameter interaction in PowerShell functions with SupportsShouldProcess, use the pattern `if ($Force.IsPresent -and -not $Confirm)` to suppress confirmations. This ensures that explicit `-Confirm:$true` is respected even when `-Force` is present, preserving user intent when they explicitly request confirmation alongside Force.
Applied to files:
source/Public/Disable-SqlDscLogin.ps1
📚 Learning: 2025-08-17T09:48:55.756Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T09:48:55.756Z
Learning: Applies to source/Public/*-SqlDsc*.ps1 : All public command names must have the noun prefixed with 'SqlDsc' (Verb-SqlDscNoun)
Applied to files:
CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test HQRM)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Unit)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Determine DSC Resource Test Requirements)
- GitHub Check: PSScriptAnalyzer
- GitHub Check: dsccommunity.SqlServerDsc (Build Package Module)
🔇 Additional comments (5)
source/Public/Disable-SqlDscLogin.ps1 (2)
93-96: Force/Confirm logic is correct
-Forcebypass guarded by explicit-Confirm:$truematches the agreed pattern. No change needed.
111-118: Localized ShouldProcess strings and Parent.InstanceName verified
- Confirmed the three resource keys in source/en-US/SqlServerDsc.strings.psd1:
- Login_Disable_ShouldProcessVerboseDescription = “Disabling the login ‘{0}’ on the instance ‘{1}’” (2 placeholders)
- Login_Disable_ShouldProcessVerboseWarning = “Are you sure you want to disable the login ‘{0}’?” (1 placeholder)
- Login_Disable_ShouldProcessCaption = “Disable login on instance” (0 placeholders)
- In the ServerObject parameter set,
$LoginObjectis always retrieved viaGet-SqlDscLogin, which returns aMicrosoft.SqlServer.Management.Smo.LoginwhoseParent.InstanceNameis guaranteed to be populated.- In the LoginObject parameter set, the cmdlet accepts a pipeline-provided SMO Login (typically from
Get-SqlDscLogin), soParentwill likewise be non-null in normal use.No changes required.
CHANGELOG.md (1)
37-37: Out-of-scope additions: please confirm or remove unrelated command listThis bullet claims multiple commands were “added,” which appears unrelated to this PR’s scope. If not introduced here, remove to avoid confusion.
-- Added `Get-SqlDscLogin`, `Get-SqlDscRole`, `New-SqlDscLogin`, `New-SqlDscRole`, `Remove-SqlDscRole`, and `Remove-SqlDscLogin` commands for retrieving and managing SQL Server logins and roles with support for refresh, pipeline input, and ShouldProcess. +<!-- Removed: not introduced by this PR. -->tests/Unit/Public/Test-SqlDscIsLoginEnabled.Tests.ps1 (1)
52-72: Parameter set definition checks look goodValidates both parameter sets’ shapes exactly. Good guard against future regressions.
source/Public/Test-SqlDscIsLoginEnabled.ps1 (1)
1-39: Help quality meets guidelinesGood: clear DESCRIPTION (>40 chars), INPUTS documenting pipeline types, OUTPUTS with meaning, and examples covering both parameter sets and pipeline usage.
johlju
approved these changes
Aug 23, 2025
Member
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 2 of 7 files at r14, 1 of 6 files at r17, 2 of 4 files at r18, 4 of 4 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 41 unresolved discussions (waiting on @copilot)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR implements three new public commands for managing SQL Server login state, addressing the requirement to enable and disable SQL Server logins using the SMO Login class methods.
New Commands
Enable-SqlDscLoginEnables a SQL Server login in a SQL Server Database Engine instance. Supports two parameter sets:
Disable-SqlDscLoginDisables a SQL Server login with the same parameter set options as Enable-SqlDscLogin.
Test-SqlDscIsLoginEnabledTests whether a SQL Server login is enabled or disabled, returning a boolean value.
Implementation Details
Enable()andDisable()methods to the SMO Login stub for testingEnable-SqlDscAuditandDisable-SqlDscAudit-ForceparameterTesting
Added comprehensive testing coverage including:
Unit Tests (17 tests)
Integration Tests
All tests pass successfully, ensuring robust functionality and maintaining code quality standards. The integration between all login management commands is thoroughly tested with comprehensive login lifecycle management scenarios.
References
Implementation based on the Microsoft SMO Login class documentation: https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.login
Closes the issue requesting SQL Server login enable/disable functionality.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.
This change is