fix: enable notifications without when condition (argoproj#2231)

* fix: enable notifications without when condition Signed-off-by: Ravi Hari <[email protected]> * fix: use trigger action item from the list Signed-off-by: Ravi Hari <[email protected]> * fix: add emptycondition logic to make notifications work with/without conditions Signed-off-by: Ravi Hari <[email protected]> * fix: linting Signed-off-by: Ravi Hari <[email protected]> Signed-off-by: Ravi Hari <[email protected]> Signed-off-by: Xijun Dai <[email protected]>
daixijun · Sep 27, 2022 · c184a81 · c184a81
1 parent 62c7b79
commit c184a81
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 20 deletions.
diff --git a/utils/record/record.go b/utils/record/record.go
@@ -2,6 +2,8 @@ package record
 
 import (
 	"context"
+	"crypto/sha1"
+	"encoding/base64"
 	"encoding/json"
 	"regexp"
 	"strings"
@@ -266,39 +268,52 @@ func (e *EventRecorderAdapter) sendNotifications(object runtime.Object, opts Eve
 		return nil
 	}
 
-	// Creates config for notifications for built-in triggers
-	triggerActions, ok := cfg.Triggers[trigger]
-	if !ok {
-		logCtx.Debugf("No configured template for trigger: %s", trigger)
-		return nil
-	}
-
 	objMap, err := toObjectMap(object)
 	if err != nil {
 		return err
 	}
 
-	res, err := notificationsAPI.RunTrigger(trigger, objMap)
-	if err != nil {
-		log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err)
-		return err
-	}
-	log.Infof("Trigger %s result: %v", trigger, res)
+	emptyCondition := hash("")
+
+	for _, destination := range destinations {
+		res, err := notificationsAPI.RunTrigger(trigger, objMap)
+		if err != nil {
+			log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err)
+			return err
+		}
+		log.Infof("Trigger %s result: %v", trigger, res)
 
-	for _, dest := range destinations {
 		for _, c := range res {
-			if c.Triggered == true {
-				err = notificationsAPI.Send(objMap, triggerActions[0].Send, dest)
+			log.Infof("Res When Condition hash: %s, Templates: %s", c.Key, c.Templates)
+			s := strings.Split(c.Key, ".")[1]
+			if s != emptyCondition && c.Triggered == true {
+				err = notificationsAPI.Send(objMap, c.Templates, destination)
+				if err != nil {
+					log.Errorf("notification error: %s", err.Error())
+					return err
+				}
+			} else if s == emptyCondition {
+				err = notificationsAPI.Send(objMap, c.Templates, destination)
 				if err != nil {
 					log.Errorf("notification error: %s", err.Error())
 					return err
 				}
 			}
 		}
 	}
+
 	return nil
 }
 
+// This function is copied over from notification engine to make sure we honour emptyCondition
+// emptyConditions today are not handled well in notification engine.
+// TODO: update notification engine to handle emptyConditions and remove this function and its usage
+func hash(input string) string {
+	h := sha1.New()
+	_, _ = h.Write([]byte(input))
+	return base64.RawURLEncoding.EncodeToString(h.Sum(nil))
+}
+
 // toObjectMap converts an object to a map for the purposes of sending to the notification engine
 func toObjectMap(object interface{}) (map[string]interface{}, error) {
 	objBytes, err := json.Marshal(object)

diff --git a/utils/record/record_test.go b/utils/record/record_test.go
@@ -96,7 +96,7 @@ func TestSendNotifications(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	mockAPI := mocks.NewMockAPI(mockCtrl)
 	cr := []triggers.ConditionResult{{
-		Key:       "",
+		Key:       "1." + hash(""),
 		Triggered: true,
 		Templates: []string{"my-template"},
 	}}
@@ -112,6 +112,33 @@ func TestSendNotifications(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestSendNotificationsWhenCondition(t *testing.T) {
+	r := v1alpha1.Rollout{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "guestbook",
+			Namespace:   "default",
+			Annotations: map[string]string{"notifications.argoproj.io/subscribe.on-foo-reason.console": "console"},
+		},
+	}
+	mockCtrl := gomock.NewController(t)
+	mockAPI := mocks.NewMockAPI(mockCtrl)
+	cr := []triggers.ConditionResult{{
+		Key:       "1." + hash(""),
+		Triggered: true,
+		Templates: []string{"my-template"},
+	}}
+	mockAPI.EXPECT().RunTrigger(gomock.Any(), gomock.Any()).Return(cr, nil).AnyTimes()
+	mockAPI.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	mockAPI.EXPECT().GetConfig().Return(api.Config{
+		Triggers: map[string][]triggers.Condition{"on-foo-reason": {triggers.Condition{When: "rollout.spec.template.spec.containers[0].image == test:blue", Send: []string{"my-template"}}}}}).AnyTimes()
+	apiFactory := &mocks.FakeFactory{Api: mockAPI}
+	rec := NewFakeEventRecorder()
+	rec.EventRecorderAdapter.apiFactory = apiFactory
+	//ch := make(chan prometheus.HistogramVec, 1)
+	err := rec.sendNotifications(&r, EventOptions{EventReason: "FooReason"})
+	assert.NoError(t, err)
+}
+
 func TestNotificationFailedCounter(t *testing.T) {
 	r := v1alpha1.Rollout{
 		ObjectMeta: metav1.ObjectMeta{
@@ -201,7 +228,7 @@ func TestSendNotificationsFails(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		mockAPI := mocks.NewMockAPI(mockCtrl)
 		cr := []triggers.ConditionResult{{
-			Key:       "",
+			Key:       "1." + hash(""),
 			Triggered: true,
 			Templates: []string{"my-template"},
 		}}
@@ -241,7 +268,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		mockAPI := mocks.NewMockAPI(mockCtrl)
 		cr := []triggers.ConditionResult{{
-			Key:       "",
+			Key:       "1." + hash(""),
 			Triggered: true,
 			Templates: []string{"my-template"},
 		}}
@@ -279,6 +306,12 @@ func TestSendNotificationsNoTrigger(t *testing.T) {
 
 	mockCtrl := gomock.NewController(t)
 	mockAPI := mocks.NewMockAPI(mockCtrl)
+	cr := []triggers.ConditionResult{{
+		Key:       "1." + hash(""),
+		Triggered: false,
+		Templates: []string{"my-template"},
+	}}
+	mockAPI.EXPECT().RunTrigger(gomock.Any(), gomock.Any()).Return(cr, errors.New("trigger 'on-missing-reason' is not configured")).AnyTimes()
 	mockAPI.EXPECT().GetConfig().Return(api.Config{
 		Triggers: map[string][]triggers.Condition{"on-foo-reason": {triggers.Condition{Send: []string{"my-template"}}}}}).AnyTimes()
 	mockAPI.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to send")).Times(0)
@@ -287,7 +320,7 @@ func TestSendNotificationsNoTrigger(t *testing.T) {
 	rec.EventRecorderAdapter.apiFactory = apiFactory
 
 	err := rec.sendNotifications(&r, EventOptions{EventReason: "MissingReason"})
-	assert.NoError(t, err)
+	assert.Error(t, err)
 }
 
 func TestNewAPIFactorySettings(t *testing.T) {