Merge pull request #496 from danilo-gemoli/feat/tide/improve-blockers-err-handling

Tide: Improve error handling when getting PR blockers

Author: k8s-ci-robot

pkg/tide/blockers/blockers.go

@@ -54,6 +54,27 @@ type OrgRepoBranch struct {
 	Org, Repo, Branch string
 }
 
+type OrgError struct {
+	Orgs  sets.Set[string]
+	inner error
+}
+
+func (e *OrgError) Error() string {
+	return e.inner.Error()
+}
+
+func (e *OrgError) Unwrap() error {
+	return e.inner
+}
+
+func (e *OrgError) Is(other error) bool {
+	if other == nil {
+		return false
+	}
+	_, ok := other.(*OrgError)
+	return ok
+}
+
 // Blockers holds maps of issues that are blocking various repos/branches.
 type Blockers struct {
 	Repo   map[OrgRepo][]Blocker       `json:"repo,omitempty"`
@@ -77,7 +98,7 @@ func FindAll(ghc githubClient, log *logrus.Entry, label string, orgRepoTokensByO
 	queries := map[string]sets.Set[string]{}
 	for org, query := range orgRepoTokensByOrg {
 		if splitQueryByOrg {
-			queries[org] = sets.New[string](blockerQuery(label, query)...)
+			queries[org] = sets.New(blockerQuery(label, query)...)
 		} else {
 			if queries[""] == nil {
 				queries[""] = sets.Set[string]{}
@@ -90,6 +111,7 @@ func FindAll(ghc githubClient, log *logrus.Entry, label string, orgRepoTokensByO
 	var errs []error
 	var lock sync.Mutex
 	var wg sync.WaitGroup
+	var orgErrs = sets.New[string]()
 
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
 	defer cancel()
@@ -110,6 +132,7 @@ func FindAll(ghc githubClient, log *logrus.Entry, label string, orgRepoTokensByO
 			defer lock.Unlock()
 			if err != nil {
 				errs = append(errs, err)
+				orgErrs.Insert(org)
 				return
 			}
 			issues = append(issues, result...)
@@ -119,11 +142,16 @@ func FindAll(ghc githubClient, log *logrus.Entry, label string, orgRepoTokensByO
 	}
 	wg.Wait()
 
+	var orgErr error
 	if err := utilerrors.NewAggregate(errs); err != nil {
-		return Blockers{}, fmt.Errorf("error searching for blocker issues: %w", err)
+		if splitQueryByOrg {
+			orgErr = &OrgError{Orgs: orgErrs, inner: fmt.Errorf("error searching for blocker issues: %w", err)}
+		} else {
+			return Blockers{}, fmt.Errorf("error searching for blocker issues: %w", err)
+		}
 	}
 
-	return fromIssues(issues, log), nil
+	return fromIssues(issues, log), orgErr
 }
 
 func fromIssues(issues []Issue, log *logrus.Entry) Blockers {

pkg/tide/blockers/blockers_test.go

@@ -18,6 +18,7 @@ package blockers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 	"strconv"
@@ -435,11 +436,18 @@ func TestBlockers(t *testing.T) {
 }
 
 type fakeGitHubClient struct {
-	lock    sync.Mutex
-	queries map[string][]string
+	lock      sync.Mutex
+	queries   map[string][]string
+	injectErr func(ctx context.Context, q interface{}, vars map[string]interface{}, org string) error
 }
 
 func (fghc *fakeGitHubClient) QueryWithGitHubAppsSupport(ctx context.Context, q interface{}, vars map[string]interface{}, org string) error {
+	if fghc.injectErr != nil {
+		if err := fghc.injectErr(ctx, q, vars, org); err != nil {
+			return err
+		}
+	}
+
 	if query := vars["query"]; query == nil || string(query.(githubql.String)) == "" {
 		return fmt.Errorf("query variable was unset, variables: %+v", vars)
 	}
@@ -461,37 +469,61 @@ func TestBlockersFindAll(t *testing.T) {
 	orgRepoTokensByOrg := map[string]string{
 		"org-a": `org:"org-a" -repo:"org-a/repo-b"`,
 		"org-b": `org:"org-b" -repo:"org-b/repo-b"`,
+		"org-c": `org:"org-c" -repo:"org-c/repo-c"`,
 	}
 	const blockerLabel = "tide/merge-blocker"
 	testCases := []struct {
-		name         string
-		usesAppsAuth bool
-
+		name            string
+		usesAppsAuth    bool
+		injectQueryErr  func(ctx context.Context, q interface{}, vars map[string]interface{}, org string) error
 		expectedQueries map[string][]string
+		expectedErr     error
 	}{
 		{
 			name:         "Apps auth, query is split by org",
 			usesAppsAuth: true,
 			expectedQueries: map[string][]string{
 				"org-a": {`-repo:"org-a/repo-b" is:issue label:"tide/merge-blocker" org:"org-a" state:open`},
 				"org-b": {`-repo:"org-b/repo-b" is:issue label:"tide/merge-blocker" org:"org-b" state:open`},
+				"org-c": {`-repo:"org-c/repo-c" is:issue label:"tide/merge-blocker" org:"org-c" state:open`},
 			},
 		},
 		{
 			name:         "No apps auth, one query",
 			usesAppsAuth: false,
 			expectedQueries: map[string][]string{
-				"": {`-repo:"org-a/repo-b" -repo:"org-b/repo-b" is:issue label:"tide/merge-blocker" org:"org-a" org:"org-b" state:open`},
+				"": {`-repo:"org-a/repo-b" -repo:"org-b/repo-b" -repo:"org-c/repo-c" is:issue label:"tide/merge-blocker" org:"org-a" org:"org-b" org:"org-c" state:open`},
+			},
+		},
+		{
+			name:         "One org query fails, the other succeed",
+			usesAppsAuth: true,
+			injectQueryErr: func(ctx context.Context, q interface{}, vars map[string]interface{}, org string) error {
+				if org == "org-c" {
+					return errors.New("fault")
+				}
+				return nil
+			},
+			expectedQueries: map[string][]string{
+				"org-a": {`-repo:"org-a/repo-b" is:issue label:"tide/merge-blocker" org:"org-a" state:open`},
+				"org-b": {`-repo:"org-b/repo-b" is:issue label:"tide/merge-blocker" org:"org-b" state:open`},
 			},
+			expectedErr: &OrgError{},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			ghc := &fakeGitHubClient{}
+			ghc := &fakeGitHubClient{injectErr: tc.injectQueryErr}
 
 			if _, err := FindAll(ghc, logrus.WithField("tc", tc.name), blockerLabel, orgRepoTokensByOrg, tc.usesAppsAuth); err != nil {
-				t.Fatalf("FindAll: %v", err)
+				if tc.expectedErr != nil {
+					if !errors.Is(err, tc.expectedErr) {
+						t.Errorf("want error %T but got %T", tc.expectedErr, err)
+					}
+				} else {
+					t.Fatalf("FindAll: %v", err)
+				}
 			}
 
 			if diff := cmp.Diff(ghc.queries, tc.expectedQueries, cmpopts.SortSlices(func(a, b string) bool { return a < b })); diff != "" {

pkg/tide/tide.go

@@ -24,6 +24,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"maps"
 	"net/http"
 	"sort"
 	"strconv"
@@ -500,7 +501,18 @@ func (c *syncController) Sync() error {
 	var blocks blockers.Blockers
 	if len(prs) > 0 {
 		blocks, err = c.provider.blockers()
-		if err != nil {
+
+		// When the GitHubApp authentication is enabled, the blocker issues query is split by organization.
+		// Each query might fail individually, and if this happens, we continue with the rest of the work
+		// but we remove all PRs that belong to any of the affected organizations. This is why we don't
+		// want to take into account PRs for which we don't have all the necessary information to process them.
+		if orgBlockersErr := (&blockers.OrgError{}); errors.As(err, &orgBlockersErr) {
+			failedBlockerOrgs := orgBlockersErr.Orgs.UnsortedList()
+			c.logger.Warnf("failed getting blockers for organizations %q - won't merge any PR coming from those", strings.Join(failedBlockerOrgs, ","))
+			maps.DeleteFunc(prs, func(_ string, pr CodeReviewCommon) bool {
+				return orgBlockersErr.Orgs.Has(pr.Org)
+			})
+		} else if err != nil {
 			return fmt.Errorf("failed getting blockers: %v", err)
 		}
 	}