Add Assert-SqlDscLogin command to validate SQL Server login existence#2123
Add Assert-SqlDscLogin command to validate SQL Server login existence#2123
Conversation
Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
|
We need to add the integration tests files to the stage in azure-pipelines.yml: SqlServerDsc/azure-pipelines.yml Line 196 in 79dcc9f The integration tests should be added after the integration test that installs Database Engine, preferably in a "Group 2". Also, when the testing the command Assert-SqlLogin, we should also test if \SqlAdmin is a login too. The user is added as a login when the integration tests 'tests/Integration/Commands/Install-SqlDscServer.Integration.Tests.ps1' is run. To dynamically get the computer name the CI has, use the command |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…min login test Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Added the Assert-SqlLogin integration test to azure-pipelines.yml in Group 2 as requested. Also included a test for the SqlAdmin login using Get-ComputerName to dynamically get the computer name in the CI environment. The test validates that the SqlAdmin login created during Database Engine installation exists. (3e5d2b5) |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Can you update the .github/copilot-instructions.md with:
- In the copilot instructions 'integration tests' section, add a paragraph that says something like: "any integration tests script files for commands should be added to a group in the stage 'Integration_Test_Commands_SqlServer' in ./azure-pipelines.yml. The group number should be determined by what other command's integration test it is dependent on."
- Mentioned in copilot instructions 'integration tests' section that to dynamically get the computer name the CI has, always use the command Get-ComputerName (a command that is available in the build pipeline).
- In the section 'PowerShell files' add that all files must end with an new line.
|
Update Unreleased section in the CHANGELOG.md using keepachangelog-format, make the notes as concrete releases notes with the main changes you made. Also update the .github/copilot-instructions so it says the the unreleased section should always be updated. |
…ructions Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Updated the copilot instructions with:
Changes made in commit aab9a74. |
…log instructions to copilot-instructions.md Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Updated CHANGELOG.md with concrete release notes for the Assert-SqlLogin command using keepachangelog format. Also added changelog update requirements to copilot-instructions.md to ensure the Unreleased section is always updated for future changes. (f12f442) |
| Context 'When connecting to SQL Server instance' { | ||
| BeforeAll { | ||
| $script:sqlAdminCredential = New-Object -TypeName 'System.Management.Automation.PSCredential' -ArgumentList @( | ||
| 'SqlAdmin', |
There was a problem hiding this comment.
The user name is a local user account so need to use format \SqlAdmin
| 'SqlAdmin', | |
| ('{0}\SqlAdmin' -f (Get-ComputerName)), |
There was a problem hiding this comment.
Fixed the SqlAdmin credential format to use computer name prefix for local user account as suggested. (c0e5b4a)
…user account Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
…cle and improve credential handling
…SQL Server service
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Reviewed 3 of 7 files at r8, 3 of 4 files at r9, 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @copilot)
tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 62 at r11 (raw file):
It 'Should not throw an error when the login exists' { { Assert-SqlDscLogin -ServerObject $mockServerObject -Name 'TestLogin' } | Should -Not -Throw
We should no longer use Should -Not -Throw to prepare for Pester v6 where it has been removed, by default the it-block will handle any unexpected exception. Change all occurrences in this test script, also update the copilot-instructions.md in the 'unit test'-section to mention it for AI agents.
Suggestion:
Assert-SqlDscLogin -ServerObject $mockServerObject -Name 'TestLogin'tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 117 at r11 (raw file):
catch { # Expected error }
Do not use try-catch block here in the test, use { <code> } | Should -Throw. We should still be able to validate that Test-SqlDscIsLogin is called with this change.
Suggestion:
{ Assert-SqlDscLogin -ServerObject $mockServerObject -Name 'NonExistentLogin' } | Should -Throwtests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 125 at r11 (raw file):
} }
Add a test 'Should have the correct parameters in parameter set <MockParameterSetName>' that validate parameters and parameter sets, use the template here:
SqlServerDsc/tests/Unit/Public/Enable-SqlDscAudit.Tests.ps1
Lines 53 to 80 in 79dcc9f
Also update the copilot-instructions.md to include that one test for public commands should always have a test to validate parameter set, and also add a template to copilot-instructions.md for AI agents to know how to make it.
…arameter set validation Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Updated unit tests to prepare for Pester v6 compatibility:
All unit tests now pass with the corrected parameter format. (7d6f7ba) |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @copilot)
.github/copilot-instructions.md line 243 at r12 (raw file):
$result.ParameterSetName | Should -Be $MockParameterSetName $result.ParameterListAsString | Should -Be $MockExpectedParameters }
This is a template when command does not have any parameter sets, use this as a base for a template for a command that does have parameter sets:
SqlServerDsc/tests/Unit/Public/New-SqlDscAudit.Tests.ps1
Lines 53 to 99 in 79dcc9f
Code quote:
It 'Should have the correct parameters in parameter set <MockParameterSetName>' -ForEach @(
@{
MockParameterSetName = '__AllParameterSets'
MockExpectedParameters = '[-Parameter1] <Type> [-Parameter2] <Type> [<CommonParameters>]'
}
) {
$result = (Get-Command -Name 'CommandName').ParameterSets |
Where-Object -FilterScript {
$_.Name -eq $mockParameterSetName
} |
Select-Object -Property @(
@{
Name = 'ParameterSetName'
Expression = { $_.Name }
},
@{
Name = 'ParameterListAsString'
Expression = { $_.ToString() }
}
)
$result.ParameterSetName | Should -Be $MockParameterSetName
$result.ParameterListAsString | Should -Be $MockExpectedParameters
}tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 67 at r12 (raw file):
It 'Should call Test-SqlDscIsLogin with correct parameters' { Assert-SqlDscLogin -ServerObject $mockServerObject -Name 'TestLogin'
Remove extra blank spaces on this line
tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 75 at r12 (raw file):
It 'Should accept ServerObject from pipeline' { { $mockServerObject | Assert-SqlDscLogin -Name 'TestLogin' } | Should -Not -Throw
We should no longer use Should -Not -Throw to prepare for Pester v6 where it has been removed, by default the it-block will handle any unexpected exception. Change all occurrences in this test script,
Suggestion:
$mockServerObject | Assert-SqlDscLogin -Name 'TestLogin'tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 113 at r12 (raw file):
It 'Should call Test-SqlDscIsLogin with correct parameters' { { Assert-SqlDscLogin -ServerObject $mockServerObject -Name 'NonExistentLogin' } | Should -Throw
Remove extra blank spaces on this line
tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1 line 139 at r12 (raw file):
$parameterInfo = (Get-Command -Name 'Assert-SqlDscLogin').Parameters['ServerObject'] $parameterInfo.Attributes.ValueFromPipeline | Should -Contain $true }
This would be good to add as a template to copilot-instructions.md, to show how to validate public command parameters in each public command's unit test.
Code quote:
It 'Should have ServerObject as a mandatory parameter' {
$parameterInfo = (Get-Command -Name 'Assert-SqlDscLogin').Parameters['ServerObject']
$parameterInfo.Attributes.Mandatory | Should -Contain $true
}
It 'Should have Name as a mandatory parameter' {
$parameterInfo = (Get-Command -Name 'Assert-SqlDscLogin').Parameters['Name']
$parameterInfo.Attributes.Mandatory | Should -Contain $true
}
It 'Should accept ServerObject from pipeline' {
$parameterInfo = (Get-Command -Name 'Assert-SqlDscLogin').Parameters['ServerObject']
$parameterInfo.Attributes.ValueFromPipeline | Should -Contain $true
}…ctions Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Fixed all the unit test issues for Pester v6 compatibility:
All unit tests now pass successfully. (f3a9647) |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @copilot)
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @copilot)
This PR implements the
Assert-SqlDscLogincommand as requested in issue #2111. The command provides a way to assert that a specified SQL Server principal exists as a login, throwing a terminating error if it doesn't.Changes Made
New Command:
Assert-SqlDscLoginsource/Public/Assert-SqlDscLogin.ps1ServerObject(mandatory, pipeline input supported): SQL Server connection objectName(mandatory): Name of the principal to validateObjectNotFoundif the login doesn't existImplementation Details
Test-SqlDscIsLogincommand for login validation following existing module patternsError Handling
ObjectNotFoundASDL0001Localization
Added localized strings to
source/en-US/SqlServerDsc.strings.psd1:AssertDscLogin_CheckingLogin: Verbose message when checking login existenceAssertDscLogin_LoginMissing: Error message when login doesn't existAssertDscLogin_LoginExists: Debug message when login existsTesting
Unit Tests:
tests/Unit/Public/Assert-SqlDscLogin.Tests.ps1Integration Tests:
tests/Integration/Commands/Assert-SqlDscLogin.Integration.Tests.ps1sa,NT AUTHORITY\SYSTEM, andSqlAdminGet-ComputerNameto dynamically test theSqlAdminlogin created during installationDocumentation
.github/copilot-instructions.mdwith SqlDsc naming requirements and additional guidelines for integration tests, CI pipeline integration, and changelog maintenanceUsage Examples
Validation
Fixes #2111
💡 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