fix: Ignore 422 response while creating commit statuses

Signed-off-by: Ragnar Paide <[email protected]>

Author: ragnarpa

docs/services/github.md

@@ -95,3 +95,7 @@ template.app-deployed: |
   For more information see the [GitHub Deployment API Docs](https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#create-a-deployment).
 - If `github.pullRequestComment.content` is set to 65536 characters or more, it will be truncated.
 - Reference is optional. When set, it will be used as the ref to deploy. If not set, the revision will be used as the ref to deploy.
+
+## Commit Statuses
+
+The [method for generating commit statuses](https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status) only allows a maximum of 1000 attempts using the same commit SHA and context. Once this limit is hit, the API begins to produce validation errors (HTTP 422). The notification engine disregards these errors and labels these notification attempts as completed.

pkg/controller/controller.go

@@ -3,6 +3,7 @@ package controller
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"runtime/debug"
@@ -232,9 +233,16 @@ func (c *notificationController) processResourceWithAPI(api api.API, resource v1
 					if err := api.Send(un.Object, cr.Templates, to); err != nil {
 						logEntry.Errorf("Failed to notify recipient %s defined in resource %s/%s: %v using the configuration in namespace %s",
 							to, resource.GetNamespace(), resource.GetName(), err, apiNamespace)
-						notificationsState.SetAlreadyNotified(c.isSelfServiceConfigureApi(api), apiNamespace, trigger, cr, to, false)
-						c.metricsRegistry.IncDeliveriesCounter(trigger, to.Service, false)
-						eventSequence.addError(fmt.Errorf("failed to deliver notification %s to %s: %v using the configuration in namespace %s", trigger, to, err, apiNamespace))
+
+						var ghErr *services.TooManyCommitStatusesError
+						if ok := errors.As(err, &ghErr); ok {
+							logEntry.Warnf("We seem to have created too many commit statuses for sha %s and context %s. Abandoning the notification.", ghErr.Sha, ghErr.Context)
+						} else {
+							notificationsState.SetAlreadyNotified(c.isSelfServiceConfigureApi(api), apiNamespace, trigger, cr, to, false)
+							c.metricsRegistry.IncDeliveriesCounter(trigger, to.Service, false)
+							eventSequence.addError(fmt.Errorf("failed to deliver notification %s to %s: %v using the configuration in namespace %s", trigger, to, err, apiNamespace))
+						}
+
 					} else {
 						logEntry.Debugf("Notification %s was sent using the configuration in namespace %s", to.Recipient, apiNamespace)
 						c.metricsRegistry.IncDeliveriesCounter(trigger, to.Service, true)

pkg/controller/controller_test.go

@@ -188,6 +188,79 @@ func TestDoesNotSendNotificationIfAnnotationPresent(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestDoesNotSendNotificationIfTooManyCommitStatusesReceived(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	setAnnotations := func(notifiedAnnoationKeyValue string) func(obj *unstructured.Unstructured) {
+		return withAnnotations(map[string]string{
+			subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient",
+			notifiedAnnotationKey: notifiedAnnoationKeyValue,
+		})
+	}
+
+	state := NotificationsState{}
+	_ = state.SetAlreadyNotified(false, "", "my-trigger", triggers.ConditionResult{}, services.Destination{Service: "mock", Recipient: "recipient"}, false)
+	app := newResource("test", setAnnotations(mustToJson(state)))
+	ctrl, api, err := newController(t, ctx, newFakeClient(app))
+	assert.NoError(t, err)
+
+	api.EXPECT().GetConfig().Return(notificationApi.Config{}).AnyTimes()
+	api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: true, Templates: []string{"test"}}}, nil).Times(2)
+	api.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(&services.TooManyCommitStatusesError{Sha: "sha", Context: "context"}).Times(1)
+
+	// First attempt should hit the TooManyCommitStatusesError.
+	// Returned annotations1 should contain the information about processed notification
+	// as a result of hitting the ToomanyCommitStatusesError error.
+	annotations1, err := ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{})
+
+	assert.NoError(t, err)
+
+	// Persist the notification state in the annotations.
+	setAnnotations(annotations1[notifiedAnnotationKey])(app)
+
+	// The second attempt should see that the notification has already been processed
+	// and the value of the notification annotation should not change. In the second attempt api.Send is not called.
+	annotations2, err := ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{})
+
+	assert.NoError(t, err)
+	assert.Equal(t, annotations1[notifiedAnnotationKey], annotations2[notifiedAnnotationKey])
+}
+
+func TestRetriesNotificationIfSendThrows(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	setAnnotations := func(notifiedAnnoationKeyValue string) func(obj *unstructured.Unstructured) {
+		return withAnnotations(map[string]string{
+			subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient",
+			notifiedAnnotationKey: notifiedAnnoationKeyValue,
+		})
+	}
+
+	state := NotificationsState{}
+	_ = state.SetAlreadyNotified(false, "", "my-trigger", triggers.ConditionResult{}, services.Destination{Service: "mock", Recipient: "recipient"}, false)
+	app := newResource("test", setAnnotations(mustToJson(state)))
+	ctrl, api, err := newController(t, ctx, newFakeClient(app))
+	assert.NoError(t, err)
+
+	api.EXPECT().GetConfig().Return(notificationApi.Config{}).AnyTimes()
+	api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: true, Templates: []string{"test"}}}, nil).Times(2)
+	api.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("boom")).Times(2)
+
+	// First attempt. The returned annotations should not contain the notification state due to the error.
+	annotations, err := ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{})
+
+	assert.NoError(t, err)
+	assert.Empty(t, annotations[notifiedAnnotationKey])
+
+	// Second attempt. The returned annotations should not contain the notification state due to the error.
+	annotations, err = ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{})
+
+	assert.NoError(t, err)
+	assert.Empty(t, annotations[notifiedAnnotationKey])
+}
+
 func TestRemovesAnnotationIfNoTrigger(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.TODO())
 	defer cancel()

pkg/services/github.go

@@ -3,6 +3,7 @@ package services
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -62,6 +63,15 @@ type GitHubPullRequestComment struct {
 	Content string `json:"content,omitempty"`
 }
 
+type TooManyCommitStatusesError struct {
+	Sha     string
+	Context string
+}
+
+func (e *TooManyCommitStatusesError) Error() string {
+	return fmt.Sprintf("too many commit statuses for sha %s and context %s", e.Sha, e.Context)
+}
+
 const (
 	repoURLtemplate  = "{{.app.spec.source.repoURL}}"
 	revisionTemplate = "{{.app.status.operationState.syncResult.revision}}"
@@ -341,7 +351,11 @@ func (g gitHubService) Send(notification Notification, _ Destination) error {
 				TargetURL:   &notification.GitHub.Status.TargetURL,
 			},
 		)
-		if err != nil {
+
+		var ghErr *github.ErrorResponse
+		if ok := errors.As(err, &ghErr); ok && ghErr.Response.StatusCode == 422 {
+			return &TooManyCommitStatusesError{Sha: notification.GitHub.revision, Context: notification.GitHub.Status.Label}
+		} else if err != nil {
 			return err
 		}
 	}