Update Copy-DbaAgentAlert.ps1 #5121
Conversation
Added check to make sure the operator exists at the destination before creating the alert. If not, skip the alert and update the output object.
Removed trailing spaces per failed test: "176Describe : dbatools style 177Context : indentation 178Name : It Copy-DbaAgentAlert.ps1 has no trailing spaces (line(s) 154,156) 179Result : Failed 180Message : Expected 0, but got 2."
|
Committed to fix failed test: 176Describe : dbatools style |
functions/Copy-DbaAgentAlert.ps1
Outdated
| @@ -150,6 +150,28 @@ function Copy-DbaAgentAlert { | |||
| continue | |||
| } | |||
|
|
|||
| $operatorDoesntExist = $false | |||
| $sourceAlertNotifications = Get-DbaAgentAlert -SqlInstance $sourceServer.Name | Where-Object Name -eq $serverAlert.name | Select-Object Notifications | |||
There was a problem hiding this comment.
This needs to pass in just the $sourceServer object and not the property value.
There was a problem hiding this comment.
Thanks, @wsmelton . Fixed and committed.
functions/Copy-DbaAgentAlert.ps1
Outdated
| } | ||
| } | ||
| } | ||
| if ($operatorDoesntExist) |
There was a problem hiding this comment.
This should be within your ShouldProcess block at line 161 so when user's user -WhatIf they do not get eronous warning messages in the output...we keep the output only to the WhatIf messages.
There was a problem hiding this comment.
So something like
if ($operatorNotExists) {
if ($PSCmdlet.ShouldProcess....) {
$copyAgentAlertSTatus....
...
Write-Message -Message "Operator []..." -Level Warning
continue
}
}
Stop-Function should be used in try/catch blocks and not with if statements. You can simply just output a warning that the alert is being skipped because the operator does not exist, and then continue on.
There was a problem hiding this comment.
Small note I'd rather use operatorNotExists than use contractions in variable names (just reads better).
There was a problem hiding this comment.
Thanks, @wsmelton . Fixed and committed along with incorporating the approach by @alevyinroc to get the list of operators (with a small correction found while testing). Thanks, @alevyinroc!
Incorporated approach by @alevyinroc at alevyinroc@8a8a8da . Fixed issues mentioned in review by @wsmelton - thank you!
Checked for $null $sourceAlertOperators so Compare-Object wouldn't fail with a $null -ReferenceObject.
Removed trailing spaces.
functions/Copy-DbaAgentAlert.ps1
Outdated
| @@ -155,7 +155,7 @@ function Copy-DbaAgentAlert { | |||
| $sourceAlertNotifications = Get-DbaAgentAlert -SqlInstance $sourceServer | Where-Object Name -eq $alertName | Select-Object Notifications | |||
| $sourceAlertOperators = $sourceAlertNotifications.Notifications.OperatorName | |||
| if ($null -ne $sourceAlertOperators) { | |||
| $missingOperators = Compare-Object -ReferenceObject $sourceAlertOperators -DifferenceObject $destServerOperators | Where-Object {$_.sideIndicator -eq "<="} | Select-Object -expandproperty InputObject | |||
| $missingOperators = Compare-Object -ReferenceObject $sourceAlertOperators -DifferenceObject $destServerOperators | Where-Object {$_.sideIndicator -eq "<="} | Select-Object -expandproperty InputObject | |||
There was a problem hiding this comment.
Use of external commands (even with built-in) require special attention because they will mess with our messaging system. (cause sea of red).
This alone is going to fail because you can't compare a populated object with an empty object. If there are no operators to start with on the destination server, your Compare command fails automatically.
There was a problem hiding this comment.
I'll work up an example of an alternate method that should include less overhead (improve timing, that compare command has overhead associated with it).
|
I've pushed an update on the test specific to this PR. So when the solution is added the test should pass with all green. On use of Compare and an alternative, I would use the pattern like below...which I think we have used in other commands but can't remember right off. Something like this should work: if ($serverAlert.HasNotification) {
$alertOperators = $serverAlert.EnumNotifications()
if ($destServerOperators.Name -notin $alertOperators.OperatorName) {
$missingOperators = ($alertOperators | Where-Object OperatorName -NotIn $destServerOperators.Name).OperatorName
if ($missingOperators.Count -gt 0 -or $missingOperators.Length -gt 0) {
$operatorList = $missingOperators -join ','
if ($PSCmdlet.ShouldProcess($destinstance, "Missing operator(s) at destination.")) {
$copyAgentAlertStatus.Status = "Skipped"
$copyAgentAlertStatus.Notes = "Operator(s) [$operatorList] do not exist on destination"
$copyAgentAlertStatus | Select-DefaultView -Property DateTime, SourceServer, DestinationServer, Name, Type, Status, Notes -TypeName MigrationObject
Write-Message -Message "One or more operators alerted by [$alertName] is not present at the destination. Alert will not be copied. Use Copy-DbaAgentOperator to copy the operator(s) to the destination. Missing operator(s): $operatorList" -Level Warning
continue
}
}
}
} |
|
Thanks, @wsmelton! I committed your suggestion. |
|
Yesssss! thanks everyone for your hard work. I'll be merging now and will publish asap. |
Added check to make sure the operator exists at the destination before creating the alert. If not, skip the alert and update the output object.
Type of Change
Purpose
Approach
Commands to test
Screenshots
Learning