[Set-DbaAgentAlert] - Added ShouldProcess to the Rename to fix #8137 #8138
Conversation
functions/Set-DbaAgentAlert.ps1
Outdated
| Write-Message -Message "Setting alert name to $NewName" -Level Verbose | ||
| $currentAlert.Rename($NewName) | ||
| if ($Pscmdlet.ShouldProcess($server, "Setting alert name to $NewName for $a")) { | ||
| $JobSchedule.Rename($NewName) |
functions/Set-DbaAgentAlert.ps1
Outdated
|
|
||
| # Change the alert | ||
| $currentAlert.Alter() | ||
| } catch { | ||
| Stop-Function -Message "Something went wrong changing the alert" -ErrorRecord $_ -Target $instance -Continue | ||
| } | ||
| Get-DbaAgentAlert -SqlInstance $server | Where-Object Name -eq $currentAlert.name | ||
| Get-DbaAgentAlert -SqlInstance $server | Where-Object Name -EQ $currentAlert.name |
There was a problem hiding this comment.
Even if not the correct case, we use -eq with Where-Object for better readability.
There was a problem hiding this comment.
Sorry, don't follow - do you want me to change here something?
There was a problem hiding this comment.
Gotcha. I caught it in the other command, but this was changed by the formatted and I missed it to change it back to -eq
functions/Set-DbaAgentAlert.ps1
Outdated
| @@ -132,7 +132,7 @@ function Set-DbaAgentAlert { | |||
| #region alert options | |||
| # Settings the options for the alert | |||
| if ($NewName) { | |||
| if ($Pscmdlet.ShouldProcess($server, "Setting alert name to $NewName for $Alert")) { | |||
| if ($Pscmdlet.ShouldProcess($server, "Setting alert name to $NewName for $a")) { | |||
There was a problem hiding this comment.
Why have you changed that? $a is only the last alert from the foreach loop. Shouldn't it be $currentAlert?
| @@ -149,9 +150,9 @@ function Set-DbaAgentAlert { | |||
| #endregion alert options | |||
|
|
|||
| # Execute | |||
| if ($PSCmdlet.ShouldProcess($SqlInstance, "Changing the alert $a")) { | |||
| if ($PSCmdlet.ShouldProcess($SqlInstance, "Committing changes for alert $a")) { | |||
There was a problem hiding this comment.
Like in line 135, this should not be $a but $currentAlert.
| try { | ||
| Write-Message -Message "Changing the alert" -Level Verbose | ||
| Write-Message -Message "Committing changes for alert $a" -Level Verbose |
There was a problem hiding this comment.
You don't need this line, as ShouldProcess also has verbose output. (But if you still like to have the line: Like in line 135, this should not be $a but $currentAlert.)
|
I will hold off until this is approved ✅ |
|
I take that back, we can fix those small issues in another PR, this is an important fix! thank you both 🙇🏼 |
Type of Change
.\tests\manual.pester.ps1)Purpose
Approach
Commands to test
Screenshots
Learning