fix: Tars no longer checks mergeable (#383)

Tars failed again, with a large number of unknown states on the tidb repository. Then I took a closer look at the logs and found that the conflicting PRs actually report errors when they are updated. The reason we couldn't update other PRs was that tars also scanned for PRs without 'status/can-merge', and those PRs returned nil because they didn't have the label, and then the loop was terminated. So this time we don't check for mergeable and update the PRs directly. Even if we encounter conflicting PRs, the update will fail and we will end up with a PR that meets the requirements or all PRs do not meet the requirements.
ti-community-infra · Mar 3, 2021 · 472d2dc · 472d2dc
1 parent a452d00
commit 472d2dc
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 73 deletions.
diff --git a/internal/pkg/externalplugins/tars/tars.go b/internal/pkg/externalplugins/tars/tars.go
@@ -72,7 +72,6 @@ type pullRequest struct {
 			Name githubql.String
 		}
 	} `graphql:"labels(first:100)"`
-	Mergeable githubql.MergeableState
 }
 
 type searchQuery struct {
@@ -154,14 +153,10 @@ func HandleIssueCommentEvent(log *logrus.Entry, ghc githubClient, ice *github.Is
 
 func handlePullRequest(log *logrus.Entry, ghc githubClient,
 	pr *github.PullRequest, cfg *externalplugins.Configuration) error {
-	if pr.Merged {
-		return nil
-	}
-
 	org := pr.Base.Repo.Owner.Login
 	repo := pr.Base.Repo.Name
 	number := pr.Number
-	mergeable := false
+	updated := false
 	tars := cfg.TarsFor(org, repo)
 
 	// If the OnlyWhenLabel configuration is set, the pr will only be updated if it has this label.
@@ -194,12 +189,12 @@ func handlePullRequest(log *logrus.Entry, ghc githubClient,
 	for _, prCommit := range prCommits {
 		for _, parentCommit := range prCommit.Parents {
 			if parentCommit.SHA == currentBaseCommit.SHA {
-				mergeable = true
+				updated = true
 			}
 		}
 	}
 
-	if mergeable {
+	if updated {
 		return nil
 	}
 
@@ -215,8 +210,6 @@ func HandlePushEvent(log *logrus.Entry, ghc githubClient, pe *github.PushEvent,
 		return nil
 	}
 
-	// Before checking mergeability wait 30 seconds to give github a chance to calculate it.
-	sleep(time.Second * 30)
 	org := pe.Repo.Owner.Login
 	repo := pe.Repo.Name
 	branch := getRefBranch(pe.Ref)
@@ -242,11 +235,6 @@ func HandlePushEvent(log *logrus.Entry, ghc githubClient, pe *github.PushEvent,
 			"pr":   num,
 		})
 
-		// Skips PRs with conflicting or unknown status.
-		if pr.Mergeable != githubql.MergeableStateMergeable {
-			l.Infof("Skipped because have conflicting or unknown status: %s.", pr.Mergeable)
-			continue
-		}
 		takenAction, err := handle(l, ghc, &pr, cfg)
 		if err != nil {
 			l.WithError(err).Error("Error handling PR.")
@@ -300,11 +288,6 @@ func HandleAll(log *logrus.Entry, ghc githubClient, config *plugins.Configuratio
 			"repo": repo,
 			"pr":   num,
 		})
-		// Skips PRs with conflicting or unknown status.
-		if pr.Mergeable != githubql.MergeableStateMergeable {
-			l.Infof("Skipped because have conflicting or unknown status: %s.", pr.Mergeable)
-			continue
-		}
 		_, err = handle(l, ghc, &pr, externalConfig)
 		if err != nil {
 			l.WithError(err).Error("Error handling PR.")

diff --git a/internal/pkg/externalplugins/tars/tars_test.go b/internal/pkg/externalplugins/tars/tars_test.go
@@ -1,3 +1,4 @@
+// nolint:dupl
 package tars
 
 import (
@@ -179,14 +180,13 @@ func TestHandleIssueCommentEvent(t *testing.T) {
 	defer func() { sleep = oldSleep }()
 
 	testcases := []struct {
-		name        string
-		pr          *github.PullRequest
-		labels      []github.Label
-		conflicting bool
-		baseCommit  github.RepositoryCommit
-		prCommits   []github.RepositoryCommit
-		outOfDate   bool
-		message     string
+		name       string
+		pr         *github.PullRequest
+		labels     []github.Label
+		baseCommit github.RepositoryCommit
+		prCommits  []github.RepositoryCommit
+		outOfDate  bool
+		message    string
 
 		expectComment  bool
 		expectDeletion bool
@@ -271,11 +271,6 @@ func TestHandleIssueCommentEvent(t *testing.T) {
 			expectComment:  true,
 			expectUpdate:   true,
 		},
-		{
-			name:        "conflicting pr is ignored",
-			pr:          getPullRequest("org", "repo", 5),
-			conflicting: true,
-		},
 	}
 
 	for _, testcase := range testcases {
@@ -496,15 +491,14 @@ func TestHandlePushEvent(t *testing.T) {
 	}
 
 	testcases := []struct {
-		name        string
-		pe          *github.PushEvent
-		pr          *github.PullRequest
-		conflicting bool
-		labels      []github.Label
-		baseCommit  github.RepositoryCommit
-		prCommits   []github.RepositoryCommit
-		outOfDate   bool
-		message     string
+		name       string
+		pe         *github.PushEvent
+		pr         *github.PullRequest
+		labels     []github.Label
+		baseCommit github.RepositoryCommit
+		prCommits  []github.RepositoryCommit
+		outOfDate  bool
+		message    string
 
 		expectComment  bool
 		expectDeletion bool
@@ -607,15 +601,6 @@ func TestHandlePushEvent(t *testing.T) {
 			expectComment:  true,
 			expectUpdate:   true,
 		},
-		{
-			name: "conflicting pr is ignored",
-			pe: &github.PushEvent{
-				Ref: "refs/heads/main",
-			},
-			pr:          getPullRequest("org1", "repo1", 6),
-			conflicting: true,
-			prCommits:   updatedPrCommits(),
-		},
 	}
 
 	oldSleep := sleep
@@ -628,7 +613,7 @@ func TestHandlePushEvent(t *testing.T) {
 			// For now we only add one pr.
 			var prs []pullRequest
 			if tc.pr != nil {
-				prs = generatePullRequests("org1", "repo1", tc.pr, tc.prCommits, tc.labels, tc.conflicting)
+				prs = generatePullRequests("org1", "repo1", tc.pr, tc.prCommits, tc.labels)
 			}
 			fc := newFakeGithubClient(prs, tc.pr, tc.baseCommit, tc.prCommits, tc.outOfDate)
 			externalConfig := &externalplugins.Configuration{}
@@ -683,14 +668,13 @@ func TestHandleAll(t *testing.T) {
 	}
 
 	testcases := []struct {
-		name        string
-		pr          *github.PullRequest
-		conflicting bool
-		labels      []github.Label
-		baseCommit  github.RepositoryCommit
-		prCommits   []github.RepositoryCommit
-		outOfDate   bool
-		message     string
+		name       string
+		pr         *github.PullRequest
+		labels     []github.Label
+		baseCommit github.RepositoryCommit
+		prCommits  []github.RepositoryCommit
+		outOfDate  bool
+		message    string
 
 		expectComment  bool
 		expectDeletion bool
@@ -775,12 +759,6 @@ func TestHandleAll(t *testing.T) {
 			expectComment:  true,
 			expectUpdate:   true,
 		},
-		{
-			name:        "conflicting pr is ignored",
-			pr:          getPullRequest("org", "repo", 5),
-			conflicting: true,
-			prCommits:   updatedPrCommits(),
-		},
 	}
 
 	oldSleep := sleep
@@ -793,7 +771,7 @@ func TestHandleAll(t *testing.T) {
 			// For now we only add one pr.
 			var prs []pullRequest
 			if tc.pr != nil {
-				prs = generatePullRequests("org", "repo", tc.pr, tc.prCommits, tc.labels, tc.conflicting)
+				prs = generatePullRequests("org", "repo", tc.pr, tc.prCommits, tc.labels)
 			}
 			fc := newFakeGithubClient(prs, tc.pr, tc.baseCommit, tc.prCommits, tc.outOfDate)
 			cfg := &plugins.Configuration{
@@ -816,7 +794,7 @@ func TestHandleAll(t *testing.T) {
 }
 
 func generatePullRequests(org string, repo string, pr *github.PullRequest,
-	prCommits []github.RepositoryCommit, labels []github.Label, conflicting bool) []pullRequest {
+	prCommits []github.RepositoryCommit, labels []github.Label) []pullRequest {
 	var prs []pullRequest
 
 	graphPr := pullRequest{}
@@ -826,11 +804,6 @@ func generatePullRequests(org string, repo string, pr *github.PullRequest,
 	graphPr.Repository.Owner.Login = githubql.String(org)
 	graphPr.Author.Login = githubql.String(pr.User.Login)
 	graphPr.BaseRef.Name = githubql.String(pr.Base.Ref)
-	if !conflicting {
-		graphPr.Mergeable = githubql.MergeableStateMergeable
-	} else {
-		graphPr.Mergeable = githubql.MergeableStateConflicting
-	}
 
 	// Convert the commit.
 	lastCommit := prCommits[len(prCommits)-1]