Install-DbaMaintenanceSolution: change Compress/Verify/CheckSum to ValidateSet string params#10247
Install-DbaMaintenanceSolution: change Compress/Verify/CheckSum to ValidateSet string params#10247andreasjordan wants to merge 3 commits intodevelopmentfrom
Conversation
…lidateSet string params Fixes #10183. The -Compress, -Verify and -CheckSum parameters were switches, which caused documentation/behaviour mismatches: - Switch default ($false) would set @compress = 'N' even when not specified - Switch default ($false) would override Ola's @verify = 'Y' default Changed all three to [string] with [ValidateSet('Default', 'ForceOn', 'ForceOff', 'Remove')]: - Default: leaves job text unchanged (Ola's defaults: Verify='Y', Checksum='Y', no Compress) - ForceOn: explicitly sets parameter to 'Y' - ForceOff: explicitly sets parameter to 'N' - Remove: strips the parameter from job text entirely Updated NUL+Verify validation to check for ForceOff/Remove instead of boolean false. Updated all integration tests to use the new string values. (do *MaintenanceSolution*) Co-authored-by: Andreas Jordan <andreasjordan@users.noreply.github.com>
|
Looks mostly good. My objections are fairly minor. My only major objection is the lack of test coverage. Specifically, only I would also like to import whatever we can from #10184, but converting that to use the new strings will be tedious. |
|
Ok, I will try and see if Claude can help here tomorrow. |
|
Claude finished @andreasjordan's task in 4m 8s —— View job Tasks
I added two new integration test contexts to cover the previously untested Note: I couldn't directly access PR #10184 (WebFetch/gh CLI not available), but based on the reviewer's feedback and the implementation, the missing test coverage was clearly New context: "Additional backup parameters with Default values" Installs with no Compress/Verify/CheckSum specified (all default to
New context: "Additional backup parameters all removed" Installs with
|
…meter values Add two new integration test contexts covering the previously untested 'Default' and 'Remove' values for the Compress, Verify, and CheckSum parameters. - Default: Compress absent, Verify='Y' and CheckSum='Y' (Ola's defaults) - Remove: all three parameters stripped from job text (do Install-DbaMaintenanceSolution) Co-authored-by: Andreas Jordan <andreasjordan@users.noreply.github.com>
|
@ReeceGoding Claude can not read your test PR. But if you like you can just get the code changes from this pull request and implment it in your fork and open a new pull requst. Or you can paste some of the tests here as a comment and Claude can read them. |
|
On review, there isn't much left to add. All that is left is to ask Claude to add a context for "Defaults for unspecified parameters are as documentation says" . This context should include this $splatInstall = @{
SqlInstance = $TestConfig.InstanceMulti2
# We could test that the default database is master,
# but that is currently enforced at the PowerShell level
# so it could just be a unit test.
Database = $testDbName
InstallJobs = $true
ReplaceExisting = $true
AutoScheduleJobs = "WeeklyFull"
}
$installResult = Install-DbaMaintenanceSolution @splatInstalland test for: Verify='Y', Checksum='Y', and no Compress. Don't forget It "Should have StartTime set to 011500 in backup schedule" {
if (-not $script:installationSucceeded) {
Set-ItResult -Skipped -Because "Installation failed"
return
}
$schedule = Get-DbaAgentSchedule -SqlInstance $TestConfig.InstanceMulti2
# We do not make any promises about job names,
# so checking for times is all we can do.
$schedule.ActiveStartTimeOfDay |
Where-Object { $_.Hours -eq 1 -and $_.Minutes -eq 15 } |
Should -Not -BeNullOrEmpty
}and It "Should have BackupLocation parameter set to instance default in backup jobs" {
if (-not $script:installationSucceeded) {
Set-ItResult -Skipped -Because "Installation failed"
return
}
$splatJobStep = @{
SqlInstance = $TestConfig.InstanceMulti2
Job = "DatabaseBackup - USER_DATABASES - FULL"
}
$jobStep = Get-DbaAgentJobStep @splatJobStep
$backupLocationSetting = (Get-DbaDefaultPath -SqlInstance $TestConfig.InstanceMulti2).Backup
$jobStep.Command | Should -BeLike "*@Directory = *$backupLocationSetting*"
} |
|
@claude please implement the tests that @ReeceGoding suggested. |
|
Claude finished @andreasjordan's task in 3m 2s —— View job Tasks
Added the
The install for this context uses |
…meter values Add 'Defaults for unspecified parameters are as documentation says' context with tests suggested by ReeceGoding covering: - No Compress parameter when not specified - Verify='Y' by default (Ola's default) - CheckSum='Y' by default (Ola's default) - StartTime set to 011500 when using AutoScheduleJobs='WeeklyFull' - BackupLocation defaults to instance default path Co-authored-by: ReeceGoding <ReeceGoding@users.noreply.github.com> Co-authored-by: Andreas Jordan <andreasjordan@users.noreply.github.com>
|
I think that's everything? |
Fixes #10183.
Changed
-Compress,-Verify, and-CheckSumfrom[switch]to[string]parameters with[ValidateSet('Default', 'ForceOn', 'ForceOff', 'Remove')].Default= leave Ola's job text unchanged (Verify='Y', Checksum='Y', no Compress)ForceOn= explicitly set parameter to 'Y'ForceOff= explicitly set parameter to 'N'Remove= strip the parameter from job text entirelyGenerated with Claude Code