Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 23, 2025

  • Add commands for SQL Server database management
  • Implement Get-SqlDscDatabase command
  • Implement New-SqlDscDatabase command
  • Implement Set-SqlDscDatabase command
  • Implement Remove-SqlDscDatabase command
  • Implement Test-SqlDscDatabase command
  • Add comprehensive unit tests (26 tests)
  • Add integration tests for all commands
  • Add Azure Pipelines integration for all database tests
  • Add localization strings (47 new strings)
  • Fix file endings and trailing whitespace
  • Add unit test coverage for CompatibilityLevel, Collation, and OwnerName parameters
  • Remove Write-Verbose mocks and fix -Not -Throw patterns in tests
  • Revert build.ps1 file mode change

💡 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] Based on this file @dsccommunity/SqlServerDsc/files/source/DSCResources/DSC_SqlDatabase/DSC_SqlDatabase.psm1 I would like to create new public commands that does the creation, testing, removing and setting of the object/task it handles. The commands sh... Add new public commands for SQL Server database management Aug 23, 2025
Copilot AI requested a review from johlju August 23, 2025 08:14
@codecov
Copy link

codecov bot commented Aug 23, 2025

Codecov Report

❌ Patch coverage is 76.70455% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 94%. Comparing base (6488425) to head (4982d91).

Files with missing lines Patch % Lines
source/Public/Set-SqlDscDatabase.ps1 51% 28 Missing ⚠️
source/Public/New-SqlDscDatabase.ps1 84% 7 Missing ⚠️
source/Public/Remove-SqlDscDatabase.ps1 81% 5 Missing ⚠️
source/Public/Test-SqlDscDatabase.ps1 96% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##           main   #2148    +/-   ##
=====================================
- Coverage    94%     94%    -1%     
=====================================
  Files       122     127     +5     
  Lines      8382    8558   +176     
=====================================
+ Hits       7939    8074   +135     
- Misses      443     484    +41     
Flag Coverage Δ
unit 94% <76%> (-1%) ⬇️
Files with missing lines Coverage Δ
source/Public/Get-SqlDscDatabase.ps1 100% <100%> (ø)
source/Public/Test-SqlDscDatabase.ps1 96% <96%> (ø)
source/Public/Remove-SqlDscDatabase.ps1 81% <81%> (ø)
source/Public/New-SqlDscDatabase.ps1 84% <84%> (ø)
source/Public/Set-SqlDscDatabase.ps1 51% <51%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Remove trailing whitespace from line 111 in Remove-SqlDscDatabase.ps1
- Add exactly one blank line at the end of all new database command files
- Resolves PSAvoidTrailingWhitespace linting rule violation

Co-authored-by: johlju <[email protected]>
Copilot AI requested a review from johlju August 24, 2025 16:58
@johlju
Copy link
Member

johlju commented Aug 24, 2025

@copilot add integration tests for Set-SqlDscDatabase and Remove-SqlDscDatabase, also add all integration tests to azure pipelines accoring to instructions.

Copy link
Contributor Author

Copilot AI commented Aug 24, 2025

@copilot add integration tests for Set-SqlDscDatabase and Remove-SqlDscDatabase, also add all integration tests to azure pipelines accoring to instructions.

I've added integration tests for Set-SqlDscDatabase and Remove-SqlDscDatabase and updated the Azure Pipelines configuration to include all database integration tests.

The new integration tests cover:

  • Set-SqlDscDatabase: Tests property modification using both ServerObject and DatabaseObject parameter sets, including recovery model, owner name, and pipeline scenarios
  • Remove-SqlDscDatabase: Tests database removal with system database protection, both parameter sets, and refresh functionality

All database integration tests have been added to Group 2 in the Integration_Test_Commands_SqlServer stage of azure-pipelines.yml to ensure they run in CI.

Completed in commit 07097b3.

Copilot AI changed the title Add new public commands for SQL Server database management Add new public commands for SQL Server database management with complete integration testing Aug 24, 2025
@coderabbitai
Copy link

coderabbitai bot commented Aug 24, 2025

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.

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.

Walkthrough

Adds five new public PowerShell cmdlets for SQL Server database lifecycle (Get, New, Set, Remove, Test), their localization strings, unit and integration Pester tests, a CHANGELOG entry, and inserts the new integration tests into the CI pipeline.

Changes

Cohort / File(s) Summary
Public cmdlets
source/Public/Get-SqlDscDatabase.ps1, source/Public/New-SqlDscDatabase.ps1, source/Public/Set-SqlDscDatabase.ps1, source/Public/Remove-SqlDscDatabase.ps1, source/Public/Test-SqlDscDatabase.ps1
Add five cmdlets to retrieve, create, update, remove, and test SMO Database objects. Implement parameter sets (ServerObject/DatabaseObject), validation (collation, compatibility level), Refresh semantics, ShouldProcess/Force where applicable, verbose/localized messaging, and output types.
Localization
source/en-US/SqlServerDsc.strings.psd1
Add en-US localization strings for new database cmdlets: verbose/status, ShouldProcess captions/descriptions, and error messages.
CHANGELOG
CHANGELOG.md
Document the five new public cmdlets under Unreleased > Added.
CI pipeline
azure-pipelines.yml
Insert new integration test scripts into the Commands integration test sequence; no other pipeline logic changes.
Integration tests
tests/Integration/Commands/...
tests/Integration/Commands/Get-SqlDscDatabase.Integration.Tests.ps1, .../New-SqlDscDatabase.Integration.Tests.ps1, .../Set-SqlDscDatabase.Integration.Tests.ps1, .../Test-SqlDscDatabase.Integration.Tests.ps1, .../Remove-SqlDscDatabase.Integration.Tests.ps1
Add end-to-end Pester suites per cmdlet with dependency resolution, setup/teardown that starts/stops a named test SQL instance, multi-version tags, and assertions covering happy/error/Refresh/parameter-set/pipeline behaviors.
Unit tests
tests/Unit/Public/...
tests/Unit/Public/Get-SqlDscDatabase.Tests.ps1, .../New-SqlDscDatabase.Tests.ps1, .../Set-SqlDscDatabase.Tests.ps1, .../Test-SqlDscDatabase.Tests.ps1, .../Remove-SqlDscDatabase.Tests.ps1
Add unit Pester tests using mocked/stubbed SMO types covering creation/update/removal/testing flows, validation of parameters (CompatibilityLevel/Collation), Refresh/PassThru behaviors, and parameter-set integrity.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant PS as PowerShell
  participant Cmd as New-SqlDscDatabase
  participant SMO as SMO Server/Database

  User->>PS: Invoke New-SqlDscDatabase -ServerObject -Name ...
  PS->>Cmd: Bind parameters
  alt Refresh specified
    Cmd->>SMO: Server.Databases.Refresh()
  end
  Cmd->>SMO: Check if database exists
  alt Not exists and ShouldProcess approved
    Cmd->>SMO: new Database(Server, Name) & set properties
    Cmd->>SMO: Create()
    opt OwnerName provided
      Cmd->>SMO: SetOwner(OwnerName)
    end
    Cmd-->>PS: Return Database
  else Exists or failure
    Cmd-->>PS: Error (already exists / create failed)
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant PS as PowerShell
  participant Cmd as Set-SqlDscDatabase
  participant SMO as SMO Server/Database

  User->>PS: Invoke Set-SqlDscDatabase (-ServerObject|-DatabaseObject) ...
  PS->>Cmd: Bind parameters
  alt ServerObject set
    opt Refresh
      Cmd->>SMO: Server.Databases.Refresh()
    end
    Cmd->>SMO: Resolve Database by Name
  else DatabaseObject set
    Cmd->>SMO: Use provided Database
  end
  Cmd->>SMO: Validate Collation/CompatibilityLevel
  alt ShouldProcess approved
    Cmd->>SMO: Update properties (Collation/Compat/Recovery/Owner)
    Cmd->>SMO: Alter()
    opt PassThru
      Cmd-->>PS: Return Database
    end
  else
    Cmd-->>PS: No changes / skip
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant PS as PowerShell
  participant Cmd as Remove-SqlDscDatabase
  participant SMO as SMO Server/Database

  User->>PS: Invoke Remove-SqlDscDatabase (-ServerObject|-DatabaseObject) [-Force]
  PS->>Cmd: Bind parameters
  alt ServerObject set
    opt Refresh
      Cmd->>SMO: Server.Databases.Refresh()
    end
    Cmd->>SMO: Resolve Database by Name
  else DatabaseObject set
    Cmd->>SMO: Use provided Database
  end
  Cmd->>Cmd: Guard against system DBs
  alt ShouldProcess approved
    Cmd->>SMO: Database.Drop()
    Cmd-->>PS: Complete (no output)
  else
    Cmd-->>PS: No action
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant PS as PowerShell
  participant Cmd as Get/Test-SqlDscDatabase
  participant SMO as SMO Server/Database

  User->>PS: Invoke Get-/Test-SqlDscDatabase -ServerObject [-Name] [-Refresh]
  PS->>Cmd: Bind parameters
  opt Refresh
    Cmd->>SMO: Server.Databases.Refresh()
  end
  alt Get
    Cmd->>SMO: Fetch by Name or all
    Cmd-->>PS: Database[] or (error + empty)
  else Test
    Cmd->>SMO: Evaluate existence and optional properties
    Cmd-->>PS: Boolean result
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Join our Discord community for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@johlju
Copy link
Member

johlju commented Aug 27, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Aug 27, 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: 17

🧹 Nitpick comments (31)
source/Public/Test-SqlDscDatabase.ps1 (1)

50-51: Update OutputType documentation format

The output type documentation should use brackets instead of backticks for consistency with PowerShell documentation standards.

-        `[System.Boolean]`
+        [System.Boolean]
tests/Unit/Public/Test-SqlDscDatabase.Tests.ps1 (1)

68-69: Consider adding explicit -Force on ScriptMethod

While the current implementation adds -Force to the pipeline result correctly, consider adding it directly to the Add-Member call for the ScriptMethod for clarity and consistency.

                } | Add-Member -MemberType 'ScriptMethod' -Name 'Refresh' -Value {
                    # Mock implementation
-                } -PassThru -Force
+                } -Force -PassThru
source/Public/New-SqlDscDatabase.ps1 (1)

52-54: Update OutputType documentation format

Similar to Test-SqlDscDatabase, the output type documentation should use brackets instead of backticks.

-        `[Microsoft.SqlServer.Management.Smo.Database]`
+        [Microsoft.SqlServer.Management.Smo.Database]
tests/Unit/Public/Get-SqlDscDatabase.Tests.ps1 (1)

80-80: Remove trailing whitespace

Line 80 has trailing whitespace that should be removed.

-            Mock -CommandName 'Write-Verbose'
-            
+            Mock -CommandName 'Write-Verbose'
+
tests/Unit/Public/Set-SqlDscDatabase.Tests.ps1 (4)

36-44: Do not set InModuleScope defaults for public-command unit tests

Per guidelines, public command unit tests should not use InModuleScope. You’re not using it anyway; drop the defaults and matching cleanup.

-    $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:dscModuleName
     $PSDefaultParameterValues['Mock:ModuleName'] = $script:dscModuleName
     $PSDefaultParameterValues['Should:ModuleName'] = $script:dscModuleName
-    $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')
     $PSDefaultParameterValues.Remove('Mock:ModuleName')
     $PSDefaultParameterValues.Remove('Should:ModuleName')

98-102: Strengthen assertion by verifying property change (RecoveryModel)

Validate that Set-SqlDscDatabase actually changed the property.

-            { Set-SqlDscDatabase -ServerObject $mockServerObject -Name 'TestDatabase' -RecoveryModel 'Simple' -Force } | Should -Not -Throw
+            { Set-SqlDscDatabase -ServerObject $mockServerObject -Name 'TestDatabase' -RecoveryModel 'Simple' -Force } | Should -Not -Throw
+            $mockServerObject.Databases['TestDatabase'].RecoveryModel | Should -Be 'Simple'

104-111: Assert returned object reflects updated state with -PassThru

Also assert the changed property on the object returned.

             $result = Set-SqlDscDatabase -ServerObject $mockServerObject -Name 'TestDatabase' -RecoveryModel 'Simple' -Force -PassThru

             $result | Should -Not -BeNullOrEmpty
             $result.Name | Should -Be 'TestDatabase'
+            $result.RecoveryModel | Should -Be 'Simple'

83-90: Optionally verify -Refresh triggers Databases.Refresh()

Track and assert a refresh was invoked when -Refresh is passed.

             $mockServerObject | Add-Member -MemberType 'ScriptProperty' -Name 'Databases' -Value {
-                return @{
+                return @{
                     'TestDatabase' = $mockDatabaseObject
                 } | Add-Member -MemberType 'ScriptMethod' -Name 'Refresh' -Value {
-                    # Mock implementation
+                    $script:refreshCalled = $true
                 } -PassThru -Force
             } -Force

Additional test to add (outside the shown range):

It 'Should call Databases.Refresh() when -Refresh is specified' {
    $script:refreshCalled = $false
    { Set-SqlDscDatabase -ServerObject $mockServerObject -Name 'TestDatabase' -RecoveryModel 'Simple' -Refresh -Force } | Should -Not -Throw
    $script:refreshCalled | Should -BeTrue
}
source/Public/Get-SqlDscDatabase.ps1 (1)

74-88: Return a typed empty array on not-found and use enum ErrorCategory

Improves consumer ergonomics and aligns with best practices.

             if (-not $databaseObject)
             {
                 Write-Verbose -Message ($script:localizedData.Database_NotFound -f $Name)

                 $missingDatabaseMessage = $script:localizedData.Database_NotFound -f $Name

                 $writeErrorParameters = @{
                     Message      = $missingDatabaseMessage
-                    Category     = 'ObjectNotFound'
+                    Category     = [System.Management.Automation.ErrorCategory]::ObjectNotFound
                     ErrorId      = 'GSDD0001' # cspell: disable-line
                     TargetObject = $Name
                 }
 
                 Write-Error @writeErrorParameters
+                $databaseObject = @()
             }
tests/Integration/Commands/New-SqlDscDatabase.Integration.Tests.ps1 (3)

29-30: Fail fast on module import in integration tests

Add -ErrorAction Stop to surface failures early.

-    Import-Module -Name $script:dscModuleName
+    Import-Module -Name $script:dscModuleName -ErrorAction 'Stop'

45-46: Ensure Connect errors fail the test immediately

Pass -ErrorAction Stop to Connect-SqlDscDatabaseEngine.

-        $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential
+        $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential -ErrorAction 'Stop'

101-118: Prefer AfterEach cleanup over try/finally and fail fast inside test

Simplify cleanup and ensure terminating errors are surfaced.

-            try {
-                $result = New-SqlDscDatabase -ServerObject $script:serverObject -Name $uniqueName -Refresh -Force
+            try {
+                $result = New-SqlDscDatabase -ServerObject $script:serverObject -Name $uniqueName -Refresh -Force -ErrorAction 'Stop'
@@
-                $dbToRemove = Get-SqlDscDatabase -ServerObject $script:serverObject -Name $uniqueName -ErrorAction 'SilentlyContinue'
+                $dbToRemove = Get-SqlDscDatabase -ServerObject $script:serverObject -Name $uniqueName -ErrorAction 'SilentlyContinue'
                 if ($dbToRemove) {
-                    Remove-SqlDscDatabase -DatabaseObject $dbToRemove -Force
+                    Remove-SqlDscDatabase -DatabaseObject $dbToRemove -Force -ErrorAction 'Stop'
                 }
             }

Optional alternative (cleaner):

Context 'When using the Refresh parameter' {
    AfterEach {
        $db = Get-SqlDscDatabase -ServerObject $script:serverObject -Name $uniqueName -ErrorAction 'SilentlyContinue'
        if ($db) { Remove-SqlDscDatabase -DatabaseObject $db -Force -ErrorAction 'Stop' }
    }
    It 'Should refresh the database collection before creating' {
        $uniqueName = 'SqlDscTestRefresh_' + (Get-Random)
        $result = New-SqlDscDatabase -ServerObject $script:serverObject -Name $uniqueName -Refresh -Force -ErrorAction 'Stop'
        $result | Should -Not -BeNullOrEmpty
        $result.Name | Should -Be $uniqueName
    }
}
tests/Integration/Commands/Get-SqlDscDatabase.Integration.Tests.ps1 (3)

29-30: Fail fast on module import in integration tests

Add -ErrorAction Stop.

-    Import-Module -Name $script:dscModuleName
+    Import-Module -Name $script:dscModuleName -ErrorAction 'Stop'

45-46: Ensure Connect errors fail the test immediately

Add -ErrorAction Stop.

-        $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential
+        $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential -ErrorAction 'Stop'

55-74: Optionally add -ErrorAction Stop to Get calls that must succeed

Keeps failures obvious; style consistency with guidelines.

-            $result = Get-SqlDscDatabase -ServerObject $script:serverObject
+            $result = Get-SqlDscDatabase -ServerObject $script:serverObject -ErrorAction 'Stop'

Apply similarly where success is expected (Lines 68, 79, 100).

tests/Integration/Commands/Remove-SqlDscDatabase.Integration.Tests.ps1 (4)

29-30: Fail fast on module import

Add -ErrorAction Stop.

-    Import-Module -Name $script:dscModuleName
+    Import-Module -Name $script:dscModuleName -ErrorAction 'Stop'

45-46: Ensure Connect errors fail the test immediately

Add -ErrorAction Stop.

-        $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential
+        $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential -ErrorAction 'Stop'

82-111: Add -ErrorAction Stop for reliable signal on failures

Apply to New/Get/Remove and add AfterEach cleanup for the context.

-            New-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameForObject -Force
+            New-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameForObject -Force -ErrorAction 'Stop'
@@
-            $databaseObject = Get-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameForObject
+            $databaseObject = Get-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameForObject -ErrorAction 'Stop'
@@
-            Remove-SqlDscDatabase -DatabaseObject $databaseObject -Force
+            Remove-SqlDscDatabase -DatabaseObject $databaseObject -Force -ErrorAction 'Stop'
@@
-            $databaseObject | Remove-SqlDscDatabase -Force
+            $databaseObject | Remove-SqlDscDatabase -Force -ErrorAction 'Stop'

Recommended AfterEach (add outside shown range):

AfterEach {
    $db = Get-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameForObject -ErrorAction 'SilentlyContinue'
    if ($db) { Remove-SqlDscDatabase -DatabaseObject $db -Force -ErrorAction 'Stop' }
}

135-149: Use -ErrorAction Stop in refresh scenario and ensure cleanup

Adds reliability to the refresh test.

-            New-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameRefresh -Force
+            New-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameRefresh -Force -ErrorAction 'Stop'
@@
-            Remove-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameRefresh -Refresh -Force
+            Remove-SqlDscDatabase -ServerObject $script:serverObject -Name $script:testDatabaseNameRefresh -Refresh -Force -ErrorAction 'Stop'
source/Public/Remove-SqlDscDatabase.ps1 (3)

101-105: Use ObjectNotFound for missing database and prefer $null-left comparison

More accurate error category and style per guidelines.

-            if (-not $DatabaseObject)
+            if ($null -eq $DatabaseObject)
             {
                 $errorMessage = $script:localizedData.Database_NotFound -f $Name
-                New-InvalidOperationException -Message $errorMessage
+                New-ObjectNotFoundException -Message $errorMessage
             }

121-126: Align ShouldProcess pattern and honor -Force/-Confirm semantics

Adopt the required variable names and Force pattern so -Force truly bypasses confirmation prompts.

-        $verboseDescriptionMessage = $script:localizedData.Database_Remove_ShouldProcessVerboseDescription -f $Name, $DatabaseObject.Parent.InstanceName
-        $verboseWarningMessage = $script:localizedData.Database_Remove_ShouldProcessVerboseWarning -f $Name
-        $captionMessage = $script:localizedData.Database_Remove_ShouldProcessCaption
+        $descriptionMessage = $script:localizedData.Database_Remove_ShouldProcessVerboseDescription -f $Name, $DatabaseObject.Parent.InstanceName
+        $confirmationMessage = $script:localizedData.Database_Remove_ShouldProcessVerboseWarning -f $Name
+        $captionMessage = $script:localizedData.Database_Remove_ShouldProcessCaption
+
+        if ($Force.IsPresent -and -not $Confirm)
+        {
+            $ConfirmPreference = 'None'
+        }
 
-        if ($Force.IsPresent -or $PSCmdlet.ShouldProcess($verboseDescriptionMessage, $verboseWarningMessage, $captionMessage))
+        if ($Force.IsPresent -or $PSCmdlet.ShouldProcess($descriptionMessage, $confirmationMessage, $captionMessage))
         {

91-97: De-duplicate system database guard

Define the system list once to avoid drift between branches.

-            $systemDatabases = @('master', 'model', 'msdb', 'tempdb')
+            $systemDatabases = @('master', 'model', 'msdb', 'tempdb')
...
-            $systemDatabases = @('master', 'model', 'msdb', 'tempdb')
+            # Uses the same $systemDatabases as above

Also applies to: 112-118

source/Public/Set-SqlDscDatabase.ps1 (3)

126-131: Use ObjectNotFound for missing database and prefer $null-left comparison

-            if (-not $DatabaseObject)
+            if ($null -eq $DatabaseObject)
             {
                 $errorMessage = $script:localizedData.Database_NotFound -f $Name
-                New-InvalidOperationException -Message $errorMessage
+                New-ObjectNotFoundException -Message $errorMessage
             }

171-176: Align ShouldProcess pattern and honor -Force/-Confirm semantics

-        $verboseDescriptionMessage = $script:localizedData.Database_Set_ShouldProcessVerboseDescription -f $Name, $ServerObject.InstanceName
-        $verboseWarningMessage = $script:localizedData.Database_Set_ShouldProcessVerboseWarning -f $Name
-        $captionMessage = $script:localizedData.Database_Set_ShouldProcessCaption
+        $descriptionMessage = $script:localizedData.Database_Set_ShouldProcessVerboseDescription -f $Name, $ServerObject.InstanceName
+        $confirmationMessage = $script:localizedData.Database_Set_ShouldProcessVerboseWarning -f $Name
+        $captionMessage = $script:localizedData.Database_Set_ShouldProcessCaption
+
+        if ($Force.IsPresent -and -not $Confirm)
+        {
+            $ConfirmPreference = 'None'
+        }
 
-        if ($Force.IsPresent -or $PSCmdlet.ShouldProcess($verboseDescriptionMessage, $verboseWarningMessage, $captionMessage))
+        if ($Force.IsPresent -or $PSCmdlet.ShouldProcess($descriptionMessage, $confirmationMessage, $captionMessage))
         {

142-156: Guard for unknown future SQL versions in compatibility mapping

If VersionMajor isn’t in the map, validation will always fail. Fallback to the highest known level.

-            if ($CompatibilityLevel -notin $supportedCompatibilityLevels.$($ServerObject.VersionMajor))
+            $levelsForVersion = $supportedCompatibilityLevels[$ServerObject.VersionMajor]
+            if (-not $levelsForVersion)
+            {
+                $levelsForVersion = $supportedCompatibilityLevels[
+                    (($supportedCompatibilityLevels.Keys | Measure-Object -Maximum).Maximum)
+                ]
+            }
+
+            if ($CompatibilityLevel -notin $levelsForVersion)
             {
                 $errorMessage = $script:localizedData.Database_InvalidCompatibilityLevel -f $CompatibilityLevel, $ServerObject.InstanceName
                 New-ObjectNotFoundException -Message $errorMessage
             }
tests/Unit/Public/New-SqlDscDatabase.Tests.ps1 (3)

36-39: Avoid InModuleScope defaults for public command tests

Guidelines: public command tests should not use InModuleScope. The default value isn’t used here—safe to remove.

-    $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:dscModuleName
     $PSDefaultParameterValues['Mock:ModuleName'] = $script:dscModuleName
     $PSDefaultParameterValues['Should:ModuleName'] = $script:dscModuleName

54-69: Assert SMO actions were invoked (Create/SetOwner) for stronger behavior checks

Use flags in the stub to verify calls, aligning with prior project practice.

 BeforeAll {
+    $script:createCalled = $false
+    $script:setOwnerCalled = $false
@@
-    Mock -CommandName 'New-Object' -ParameterFilter { $TypeName -eq 'Microsoft.SqlServer.Management.Smo.Database' } -MockWith {
+    Mock -CommandName 'New-Object' -ParameterFilter { $TypeName -eq 'Microsoft.SqlServer.Management.Smo.Database' } -MockWith {
         $mockDatabaseObject = New-Object -TypeName 'Microsoft.SqlServer.Management.Smo.Database'
@@
-        $mockDatabaseObject | Add-Member -MemberType 'ScriptMethod' -Name 'Create' -Value {
-            # Mock implementation
-        } -Force
+        $mockDatabaseObject | Add-Member -MemberType 'ScriptMethod' -Name 'Create' -Value {
+            $script:createCalled = $true
+        } -Force
         $mockDatabaseObject | Add-Member -MemberType 'ScriptMethod' -Name 'SetOwner' -Value {
             param($OwnerName)
-            # Mock implementation
+            $script:setOwnerCalled = $true
         } -Force
         return $mockDatabaseObject
     }
@@
 It 'Should create a database successfully with minimal parameters' {
@@
     $result.Name | Should -Be 'TestDatabase'
+    $script:createCalled | Should -BeTrue
@@
 It 'Should create a database with specified properties' {
@@
     $result.CompatibilityLevel | Should -Be 'Version150'
+    $script:createCalled | Should -BeTrue
+    $script:setOwnerCalled | Should -BeTrue
 }

Also applies to: 70-85, 87-95, 97-107


124-139: Brittle __AllParameterSets string comparison

To reduce brittleness, prefer asserting presence/mandatory flags on individual parameters rather than full ToString().

tests/Unit/Public/Remove-SqlDscDatabase.Tests.ps1 (3)

36-39: Avoid InModuleScope defaults for public command tests

Not used—remove per guidelines.

-    $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:dscModuleName
     $PSDefaultParameterValues['Mock:ModuleName'] = $script:dscModuleName
     $PSDefaultParameterValues['Should:ModuleName'] = $script:dscModuleName

52-76: Verify Drop() was called when removal succeeds

Assert the destructive action occurred.

 BeforeAll {
+    $script:dropCalled = $false
@@
-    $mockDatabaseObject | Add-Member -MemberType 'ScriptMethod' -Name 'Drop' -Value {
-        # Mock implementation
-    } -Force
+    $mockDatabaseObject | Add-Member -MemberType 'ScriptMethod' -Name 'Drop' -Value {
+        $script:dropCalled = $true
+    } -Force
@@
 It 'Should remove database successfully' {
@@
-    { Remove-SqlDscDatabase -ServerObject $mockServerObject -Name 'TestDatabase' -Force } | Should -Not -Throw
+    { Remove-SqlDscDatabase -ServerObject $mockServerObject -Name 'TestDatabase' -Force } | Should -Not -Throw
+    $script:dropCalled | Should -BeTrue
 }

Also applies to: 78-83


99-111: Add a system-database guard test for DatabaseObject parameter set

Covers the second branch of the protection logic.

 BeforeAll {
     $mockDatabaseObject = New-Object -TypeName 'Microsoft.SqlServer.Management.Smo.Database'
-    $mockDatabaseObject | Add-Member -MemberType 'NoteProperty' -Name 'Name' -Value 'TestDatabase' -Force
+    $mockDatabaseObject | Add-Member -MemberType 'NoteProperty' -Name 'Name' -Value 'TestDatabase' -Force
@@
 }
 
 It 'Should remove database successfully using database object' {
@@
 }
+
+It 'Should throw when database object is a system database' {
+    $sysDb = New-Object -TypeName 'Microsoft.SqlServer.Management.Smo.Database'
+    $sysDb | Add-Member -MemberType 'NoteProperty' -Name 'Name' -Value 'master' -Force
+    $sysDb | Add-Member -MemberType 'ScriptProperty' -Name 'Parent' -Value {
+        $s = New-Object -TypeName 'Microsoft.SqlServer.Management.Smo.Server'
+        $s | Add-Member -MemberType 'NoteProperty' -Name 'InstanceName' -Value 'TestInstance' -Force
+        return $s
+    } -Force
+    { Remove-SqlDscDatabase -DatabaseObject $sysDb -Force } | Should -Throw -ExpectedMessage '*Cannot remove system database*'
+}

Also applies to: 113-118

@johlju
Copy link
Member

johlju commented Aug 28, 2025

@CodeRabbit review

Copy link
Contributor Author

Copilot AI commented Aug 29, 2025

@copilot add integration tests for Set-SqlDscDatabase and Remove-SqlDscDatabase, also add all integration tests to azure pipelines accoring to instructions.

Integration tests for Set-SqlDscDatabase and Remove-SqlDscDatabase have already been implemented and added to Azure Pipelines in commit 07097b3. The tests are located in:

  • tests/Integration/Commands/Set-SqlDscDatabase.Integration.Tests.ps1
  • tests/Integration/Commands/Remove-SqlDscDatabase.Integration.Tests.ps1

All database integration tests are included in Group 2 of the Integration_Test_Commands_SqlServer stage in azure-pipelines.yml (lines 304-308) and will run across SQL Server 2017, 2019, and 2022 on Windows 2022 and 2025 environments.

@johlju johlju marked this pull request as ready for review August 29, 2025 18:36
@johlju johlju requested a review from a team as a code owner August 29, 2025 18:36
@johlju johlju changed the title Add new public commands for SQL Server database management with complete integration testing Add new public commands for SQL Server database management Aug 29, 2025
@johlju johlju changed the title Add new public commands for SQL Server database management Add commands for SQL Server database management Aug 29, 2025
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 4 of 5 files at r2, 1 of 4 files at r3, 2 of 4 files at r5, 2 of 6 files at r6, 4 of 4 files at r7, 1 of 1 files at r8, 5 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @copilot)

Copy link
Member

Choose a reason for hiding this comment

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

@copilot Revert the change to this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the build.ps1 file mode change in commit d262457.

Copilot AI requested a review from johlju August 29, 2025 19:17
@johlju johlju merged commit 53a4a65 into main Aug 29, 2025
0 of 5 checks passed
@johlju johlju deleted the copilot/fix-1fc19ae7-6072-474f-b527-f0328b3aaa64 branch August 29, 2025 19:17
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