feat: do not resend successful notification fix #79 (#116)

argoproj-labs · Sep 23, 2020 · 1d3da1c · 1d3da1c
1 parent 582ae88
commit 1d3da1c
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 46 deletions.
diff --git a/controller/controller.go b/controller/controller.go
@@ -182,10 +182,19 @@ func (c *notificationController) processApp(app *unstructured.Unstructured, logE
 			logEntry.Debugf("Failed to execute condition of trigger %s: %v", triggerKey, err)
 		}
 		recipients := c.getRecipients(app, triggerKey)
-		triggerAnnotation := fmt.Sprintf("%s.%s", triggerKey, sharedrecipients.AnnotationPostfix)
 		logEntry.Infof("Trigger %s result: %v", triggerKey, triggered)
 		c.metricsRegistry.IncTriggerEvaluationsCounter(triggerKey, triggered)
-		if triggered {
+		if !triggered {
+			for recipient := range recipients {
+				triggerAnnotation := sharedrecipients.FormatTriggerRecipientAnnotation(triggerKey, recipient)
+				delete(annotations, triggerAnnotation)
+			}
+			app.SetAnnotations(annotations)
+			continue
+		}
+
+		for recipient := range recipients {
+			triggerAnnotation := sharedrecipients.FormatTriggerRecipientAnnotation(triggerKey, recipient)
 			_, alreadyNotified := annotations[triggerAnnotation]
 			// informer might have stale data, so we cannot trust it and should reload app state to avoid sending notification twice
 			if !alreadyNotified && !refreshed {
@@ -202,45 +211,44 @@ func (c *notificationController) processApp(app *unstructured.Unstructured, logE
 
 				refreshed = true
 			}
+			if alreadyNotified {
+				logEntry.Infof("%s notification already sent", triggerKey)
+				continue // move to the next recipient
+			}
+			successful := true
 
-			if !alreadyNotified {
-				successful := true
-				for recipient := range recipients {
-					parts := strings.Split(recipient, ":")
-					if len(parts) < 2 {
-						return fmt.Errorf("%s is not valid recipient. Expected recipient format is <type>:<name>", recipient)
-					}
-					notifierType := parts[0]
-					notifier, ok := c.notifiers[notifierType]
-					if !ok {
-						return fmt.Errorf("%s is not valid recipient type.", notifierType)
-					}
+			parts := strings.Split(recipient, ":")
+			if len(parts) < 2 {
+				return fmt.Errorf("%s is not valid recipient. Expected recipient format is <type>:<name>", recipient)
+			}
+			notifierType := parts[0]
+			notifier, ok := c.notifiers[notifierType]
+			if !ok {
+				return fmt.Errorf("%s is not valid recipient type.", notifierType)
+			}
 
-					logEntry.Infof("Sending %s notification", triggerKey)
-					ctx := sharedrecipients.CopyStringMap(c.context)
-					ctx[notificationType] = notifierType
-					notification, err := t.FormatNotification(app, ctx)
-					if err != nil {
-						return err
-					}
-					if err = notifier.Send(*notification, parts[1]); err != nil {
-						logEntry.Errorf("Failed to notify recipient %s defined in app %s/%s: %v",
-							recipient, app.GetNamespace(), app.GetName(), err)
-						successful = false
-						c.metricsRegistry.IncDeliveriesCounter(t.GetTemplateName(), notifierType, false)
-					} else {
-						c.metricsRegistry.IncDeliveriesCounter(t.GetTemplateName(), notifierType, true)
-					}
-				}
-				if successful {
-					annotations[triggerAnnotation] = time.Now().Format(time.RFC3339)
-				}
+			logEntry.Infof("Sending %s notification", triggerKey)
+			ctx := sharedrecipients.CopyStringMap(c.context)
+			ctx[notificationType] = notifierType
+			notification, err := t.FormatNotification(app, ctx)
+			if err != nil {
+				return err
+			}
+			if err = notifier.Send(*notification, parts[1]); err != nil {
+				logEntry.Errorf("Failed to notify recipient %s defined in app %s/%s: %v",
+					recipient, app.GetNamespace(), app.GetName(), err)
+				successful = false
+				c.metricsRegistry.IncDeliveriesCounter(t.GetTemplateName(), notifierType, false)
 			} else {
-				logEntry.Infof("%s notification already sent", triggerKey)
+				c.metricsRegistry.IncDeliveriesCounter(t.GetTemplateName(), notifierType, true)
+			}
+
+			if successful {
+				logEntry.Debugf("Notification %s was sent", recipient)
+				annotations[triggerAnnotation] = time.Now().Format(time.RFC3339)
 			}
-		} else {
-			delete(annotations, triggerAnnotation)
 		}
+
 	}
 	app.SetAnnotations(annotations)
 	return nil

diff --git a/controller/controller_test.go b/controller/controller_test.go
@@ -75,15 +75,15 @@ func TestSendsNotificationIfTriggered(t *testing.T) {
 	err = ctrl.processApp(app, logEntry)
 
 	assert.NoError(t, err)
-	assert.NotEmpty(t, app.GetAnnotations()[fmt.Sprintf("mock.%s", recipients.AnnotationPostfix)])
+	assert.NotEmpty(t, app.GetAnnotations()[fmt.Sprintf("mock.mock.recipient.%s", recipients.AnnotationPostfix)])
 }
 
 func TestDoesNotSendNotificationIfAnnotationPresent(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.TODO())
 	defer cancel()
 	app := NewApp("test", WithAnnotations(map[string]string{
-		recipients.RecipientsAnnotation:                      "mock:recipient",
-		fmt.Sprintf("mock.%s", recipients.AnnotationPostfix): time.Now().Format(time.RFC3339),
+		recipients.RecipientsAnnotation:                                     "mock:recipient",
+		fmt.Sprintf("mock.mock.recipient.%s", recipients.AnnotationPostfix): time.Now().Format(time.RFC3339),
 	}))
 	ctrl, trigger, _, err := newController(t, ctx, fake.NewSimpleDynamicClient(runtime.NewScheme(), app))
 	assert.NoError(t, err)
@@ -99,8 +99,8 @@ func TestRemovesAnnotationIfNoTrigger(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.TODO())
 	defer cancel()
 	app := NewApp("test", WithAnnotations(map[string]string{
-		recipients.RecipientsAnnotation:                      "mock:recipient",
-		fmt.Sprintf("mock.%s", recipients.AnnotationPostfix): time.Now().Format(time.RFC3339),
+		recipients.RecipientsAnnotation:                                     "mock:recipient",
+		fmt.Sprintf("mock.mock.recipient.%s", recipients.AnnotationPostfix): time.Now().Format(time.RFC3339),
 	}))
 	ctrl, trigger, _, err := newController(t, ctx, fake.NewSimpleDynamicClient(runtime.NewScheme(), app))
 	assert.NoError(t, err)
@@ -110,7 +110,7 @@ func TestRemovesAnnotationIfNoTrigger(t *testing.T) {
 	err = ctrl.processApp(app, logEntry)
 
 	assert.NoError(t, err)
-	assert.Empty(t, app.GetAnnotations()[fmt.Sprintf("mock.%s", recipients.AnnotationPostfix)])
+	assert.Empty(t, app.GetAnnotations()[fmt.Sprintf("mock.mock.recipient.%s", recipients.AnnotationPostfix)])
 }
 
 func TestGetRecipients(t *testing.T) {
@@ -148,8 +148,8 @@ func TestUpdatedAnnotationsSavedAsPatch(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.TODO())
 	defer cancel()
 	app := NewApp("test", WithAnnotations(map[string]string{
-		recipients.RecipientsAnnotation:                      "mock:recipient",
-		fmt.Sprintf("mock.%s", recipients.AnnotationPostfix): time.Now().Format(time.RFC3339),
+		recipients.RecipientsAnnotation:                                     "mock:recipient",
+		fmt.Sprintf("mock.mock.recipient.%s", recipients.AnnotationPostfix): time.Now().Format(time.RFC3339),
 	}))
 
 	patchCh := make(chan []byte)
@@ -167,13 +167,13 @@ func TestUpdatedAnnotationsSavedAsPatch(t *testing.T) {
 	go ctrl.Run(ctx, 1)
 
 	select {
-	case <-time.After(time.Second * 10000):
+	case <-time.After(time.Second * 60):
 		t.Error("application was not patched")
 	case patchData := <-patchCh:
 		patch := map[string]interface{}{}
 		err = json.Unmarshal(patchData, &patch)
 		assert.NoError(t, err)
-		val, ok, err := unstructured.NestedFieldNoCopy(patch, "metadata", "annotations", fmt.Sprintf("mock.%s", recipients.AnnotationPostfix))
+		val, ok, err := unstructured.NestedFieldNoCopy(patch, "metadata", "annotations", fmt.Sprintf("mock.mock.recipient.%s", recipients.AnnotationPostfix))
 		assert.NoError(t, err)
 		assert.True(t, ok)
 		assert.Nil(t, val)

diff --git a/shared/recipients/annotations.go b/shared/recipients/annotations.go
@@ -1,6 +1,7 @@
 package recipients
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/argoproj-labs/argocd-notifications/shared/text"
@@ -71,3 +72,11 @@ func AnnotationsPatch(old map[string]string, new map[string]string) map[string]*
 	}
 	return patch
 }
+
+// FormatTriggerRecipientAnnotation builds from a trigger and recipient
+// valid kuberetes Annotation
+func FormatTriggerRecipientAnnotation(trigger string, recipient string) string {
+	recipient = strings.ReplaceAll(recipient, ":", ".")
+	recipient = strings.ReplaceAll(recipient, "@", ".")
+	return fmt.Sprintf("%s.%s.%s", trigger, recipient, AnnotationPostfix)
+}
diff --git a/shared/recipients/annotations_test.go b/shared/recipients/annotations_test.go
@@ -40,3 +40,26 @@ func TestAnnotationsPatch(t *testing.T) {
 		"key4": pointer.StringPtr("val4"),
 	}, patch)
 }
+
+func TestFormatTriggerRecipientAnnotation(t *testing.T) {
+	tests := []struct {
+		trigger   string
+		recipient string
+		want      string
+	}{
+		{
+			recipient: "slack:channel",
+			trigger:   "on-sync",
+			want:      "on-sync.slack.channel.argocd-notifications.argoproj.io",
+		},
+		{
+			recipient: "email:my@email.com",
+			trigger:   "on-sync",
+			want:      "on-sync.email.my.email.com.argocd-notifications.argoproj.io",
+		},
+	}
+	for _, tt := range tests {
+		got := FormatTriggerRecipientAnnotation(tt.trigger, tt.recipient)
+		assert.Equal(t, tt.want, got)
+	}
+}