diff --git a/echo-core/src/main/java/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgent.java b/echo-core/src/main/java/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgent.java index 85126a6e3..c4c0e2ae6 100644 --- a/echo-core/src/main/java/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgent.java +++ b/echo-core/src/main/java/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgent.java @@ -108,6 +108,21 @@ public void processEvent(Event event) { return; } + if (STAGE.equals(configType)) { + Object when = event.getContent().get("when"); + if (when != null) { + if (when instanceof String) { + if (!isManualJudgmentMatchStringCase((String) when, status)) { + return; + } + } else if (when instanceof Collection) { + if (!isManualJudgmentMatchCollectionCase((Collection) when, status)) { + return; + } + } + } + } + // TODO(lpollo): why do we have a 'CANCELED' status and a canceled property, which are prime for // inconsistency? if (isExecution(configType) && isExecutionCanceled(event)) { @@ -241,11 +256,26 @@ String getName() { } private static boolean isManualJudgmentMatchStringCase(String when, String status) { + // For Manual Judgment stages, only send specialized notifications, not standard stage ones + // This prevents duplicate notifications between standard stage and manual judgment events + if (ManualJudgmentCondition.MANUAL_JUDGMENT.getName().equals(when) + || ManualJudgmentCondition.CONTINUE.getName().equals(when) + || ManualJudgmentCondition.STOP.getName().equals(when)) { + return false; + } return status.equals(MANUAL_JUDGMENT_CONDITIONS.get(when)); } private static boolean isManualJudgmentMatchCollectionCase( Collection when, String status) { + // For Manual Judgment stages, only send specialized notifications, not standard stage ones + // This prevents duplicate notifications between standard stage and manual judgment events + if (when.contains(ManualJudgmentCondition.MANUAL_JUDGMENT.getName()) + || when.contains(ManualJudgmentCondition.CONTINUE.getName()) + || when.contains(ManualJudgmentCondition.STOP.getName())) { + return false; + } + for (String condition : when) { if (status.equals(MANUAL_JUDGMENT_CONDITIONS.get(condition))) { return true; diff --git a/echo-core/src/test/groovy/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgentSpec.groovy b/echo-core/src/test/groovy/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgentSpec.groovy index 5bf4bfd63..230fa4d6b 100644 --- a/echo-core/src/test/groovy/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgentSpec.groovy +++ b/echo-core/src/test/groovy/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgentSpec.groovy @@ -92,6 +92,45 @@ class AbstractEventNotificationAgentSpec extends Specification { // notifications ON, stage is synthetic fakeStageEvent("orca:stage:complete", "stage.complete", false, true) || 0 } + + @Unroll + def "prevents stage notifications for Manual Judgment stages with specialized notifications"() { + given: + subclassMock.sendNotifications(*_) >> { notification, application, event_local, config, status_param -> } + + when: + // Create a stage event with specialized Manual Judgment notification type + def event = createManualJudgmentStageEvent("orca:stage:" + status, notificationType) + agent.processEvent(event) + + then: + // Verify no notifications are sent due to our fix + 0 * subclassMock.sendNotifications(*_) + + where: + status | notificationType + "starting" | "manualJudgment" + "complete" | "manualJudgmentContinue" + "failed" | "manualJudgmentStop" + } + + @Unroll + def "still sends stage notifications for non-Manual Judgment stages"() { + given: + subclassMock.sendNotifications(*_) >> { notification, application, event_local, config, status_param -> } + + when: + // Create a stage event with regular stage notification type + def event = createRegularStageEvent("orca:stage:" + status, "stage." + status) + agent.processEvent(event) + + then: + // Verify notifications are still sent for normal stages + 1 * subclassMock.sendNotifications(*_) + + where: + status << ["starting", "complete", "failed"] + } @Unroll def "sends notifications for ManualJudgment stage based on status and configuration"() { @@ -106,9 +145,9 @@ class AbstractEventNotificationAgentSpec extends Specification { where: event || expectedNotifications - fakeStageEvent("orca:stage:complete", "manualJudgmentContinue") || 1 - fakeStageEvent("orca:stage:starting", "manualJudgment") || 1 - fakeStageEvent("orca:stage:failed", "manualJudgmentStop") || 1 + fakeStageEvent("orca:stage:complete", "manualJudgmentContinue") || 0 + fakeStageEvent("orca:stage:starting", "manualJudgment") || 0 + fakeStageEvent("orca:stage:failed", "manualJudgmentStop") || 0 } private def fakePipelineEvent(String type, String status, String notifyWhen, Map extraExecutionProps = [:]) { @@ -173,4 +212,46 @@ class AbstractEventNotificationAgentSpec extends Specification { return new Event(eventProps) } + + private Event createManualJudgmentStageEvent(String eventType, String notificationType) { + return new Event( + details: [ + type: eventType, + application: "testapp" + ], + content: [ + context: [ + type: "manualJudgment", + sendNotifications: true, + notifications: [ + [ + type: "fake", + when: [notificationType] + ] + ] + ] + ] + ) + } + + private Event createRegularStageEvent(String eventType, String notificationType) { + return new Event( + details: [ + type: eventType, + application: "testapp" + ], + content: [ + context: [ + type: "regularStage", + sendNotifications: true, + notifications: [ + [ + type: "fake", + when: [notificationType] + ] + ] + ] + ] + ) + } }