Remove ListMergedPRs API (#1566)

Co-authored-by: Azeem Shaikh <[email protected]>

checker/raw_result.go

@@ -14,6 +14,8 @@
 
 package checker
 
+import "time"
+
 // RawResults contains results before a policy
 // is applied.
 type RawResults struct {
@@ -123,12 +125,13 @@ type DefaultBranchCommit struct {
 }
 
 // MergeRequest represents a merge request.
-//nolint:govet
+// nolint:govet
 type MergeRequest struct {
-	Number  int
-	Labels  []string
-	Reviews []Review
-	Author  User
+	Number   int
+	Labels   []string
+	Reviews  []Review
+	Author   User
+	MergedAt time.Time
 }
 
 // Review represent a review using the built-in review system.

checks/ci_tests.go

@@ -39,16 +39,18 @@ func init() {
 
 // CITests runs CI-Tests check.
 func CITests(c *checker.CheckRequest) checker.CheckResult {
-	prs, err := c.RepoClient.ListMergedPRs()
+	commits, err := c.RepoClient.ListCommits()
 	if err != nil {
-		e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
+		e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
 		return checker.CreateRuntimeErrorResult(CheckCITests, e)
 	}
 
 	totalMerged := 0
 	totalTested := 0
-	for index := range prs {
-		pr := &prs[index]
+	for i := range commits {
+		pr := &commits[i].AssociatedMergeRequest
+		// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
+		// but the PR was created in the original repo.
 		if pr.MergedAt.IsZero() {
 			continue
 		}

checks/code_review_test.go

@@ -37,7 +37,6 @@ func TestCodereview(t *testing.T) {
 		name      string
 		commiterr error
 		commits   []clients.Commit
-		prs       []clients.PullRequest
 		expected  checker.CheckResult
 	}{
 		{
@@ -70,150 +69,115 @@ func TestCodereview(t *testing.T) {
 		},
 		{
 			name: "Valid GitHub PR",
-			prs: []clients.PullRequest{
-				{
-					Number:   1,
-					MergedAt: time.Now(),
-					Reviews: []clients.Review{
-						{
-							State: "APPROVED",
-						},
-					},
-					MergeCommit: clients.Commit{
-						SHA: "sha",
-					},
-				},
-			},
 			commits: []clients.Commit{
 				{
 					SHA: "sha",
 					Committer: clients.User{
 						Login: "user",
 					},
+					AssociatedMergeRequest: clients.PullRequest{
+						Number:   1,
+						MergedAt: time.Now(),
+						Reviews: []clients.Review{
+							{
+								State: "APPROVED",
+							},
+						},
+					},
 				},
 			},
-
 			expected: checker.CheckResult{
 				Score: 10,
 				Pass:  true,
 			},
 		},
 		{
 			name: "Valid Prow PR as not a bot",
-			prs: []clients.PullRequest{
-				{
-					Number:   1,
-					MergedAt: time.Now(),
-					Labels: []clients.Label{
-						{
-							Name: "lgtm",
-						},
-					},
-					MergeCommit: clients.Commit{
-						SHA: "sha",
-					},
-				},
-			},
 			commits: []clients.Commit{
 				{
 					SHA: "sha",
 					Committer: clients.User{
 						Login: "user",
 					},
+					AssociatedMergeRequest: clients.PullRequest{
+						Number:   1,
+						MergedAt: time.Now(),
+						Labels: []clients.Label{
+							{
+								Name: "lgtm",
+							},
+						},
+					},
 				},
 			},
-
 			expected: checker.CheckResult{
 				Score: 10,
 				Pass:  true,
 			},
 		},
 		{
 			name: "Valid Prow PR and commits as bot",
-			prs: []clients.PullRequest{
-				{
-					Number:   1,
-					MergedAt: time.Now(),
-					Labels: []clients.Label{
-						{
-							Name: "lgtm",
-						},
-					},
-					MergeCommit: clients.Commit{
-						SHA: "sha",
-					},
-				},
-			},
 			commits: []clients.Commit{
 				{
 					SHA: "sha",
 					Committer: clients.User{
 						Login: "bot",
 					},
+					AssociatedMergeRequest: clients.PullRequest{
+						Number:   1,
+						MergedAt: time.Now(),
+						Labels: []clients.Label{
+							{
+								Name: "lgtm",
+							},
+						},
+					},
 				},
 			},
-
 			expected: checker.CheckResult{
 				Score: 10,
 				Pass:  true,
 			},
 		},
 		{
 			name: "Valid PR's and commits with merged by someone else",
-			prs: []clients.PullRequest{
-				{
-					Number: 1,
-					Labels: []clients.Label{
-						{
-							Name: "lgtm",
-						},
-					},
-					MergeCommit: clients.Commit{
-						SHA: "sha",
-						Committer: clients.User{
-							Login: "bot",
-						},
-					},
-				},
-			},
 			commits: []clients.Commit{
 				{
 					SHA: "sha",
 					Committer: clients.User{
 						Login: "bob",
 					},
+					AssociatedMergeRequest: clients.PullRequest{
+						Number: 1,
+						Labels: []clients.Label{
+							{
+								Name: "lgtm",
+							},
+						},
+					},
 				},
 			},
-
 			expected: checker.CheckResult{
 				Score: 0,
 			},
 		},
 		{
 			name: "2 PRs 2 review on GitHub",
-			prs: []clients.PullRequest{
-				{
-					Number:   1,
-					MergedAt: time.Now(),
-					Reviews: []clients.Review{
-						{
-							State: "APPROVED",
-						},
-					},
-					MergeCommit: clients.Commit{
-						SHA: "sha",
-						Committer: clients.User{
-							Login: "bot",
-						},
-					},
-				},
-			},
 			commits: []clients.Commit{
 				{
 					SHA: "sha",
 					Committer: clients.User{
 						Login: "bob",
 					},
+					AssociatedMergeRequest: clients.PullRequest{
+						Number:   1,
+						MergedAt: time.Now(),
+						Reviews: []clients.Review{
+							{
+								State: "APPROVED",
+							},
+						},
+					},
 				},
 				{
 					SHA: "sha2",
@@ -222,7 +186,6 @@ func TestCodereview(t *testing.T) {
 					},
 				},
 			},
-
 			expected: checker.CheckResult{
 				Score: 5,
 			},
@@ -235,18 +198,7 @@ func TestCodereview(t *testing.T) {
 			t.Parallel()
 			ctrl := gomock.NewController(t)
 			mockRepo := mockrepo.NewMockRepoClient(ctrl)
-			mockRepo.EXPECT().ListMergedPRs().DoAndReturn(func() ([]clients.PullRequest, error) {
-				if tt.err != nil {
-					return tt.prs, tt.err
-				}
-				return tt.prs, tt.err
-			}).AnyTimes()
-			mockRepo.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) {
-				if tt.commiterr != nil {
-					return tt.commits, tt.commiterr
-				}
-				return tt.commits, tt.err
-			}).AnyTimes()
+			mockRepo.EXPECT().ListCommits().Return(tt.commits, tt.err).AnyTimes()
 
 			req := checker.CheckRequest{
 				RepoClient: mockRepo,

checks/evaluation/code_review.go

@@ -126,7 +126,7 @@ func getApprovedReviewSystem(c *checker.DefaultBranchCommit, dl checker.DetailLo
 
 func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
 	mr := c.MergeRequest
-	if mr == nil {
+	if mr == nil || mr.MergedAt.IsZero() {
 		return false
 	}
 
@@ -163,7 +163,7 @@ func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) b
 		return true
 	}
 
-	if c.MergeRequest != nil {
+	if c.MergeRequest != nil && !c.MergeRequest.MergedAt.IsZero() {
 		for _, l := range c.MergeRequest.Labels {
 			if l == "lgtm" || l == "approved" {
 				dl.Debug3(&checker.LogMessage{

checks/maintained.go

@@ -59,8 +59,8 @@ func IsMaintained(c *checker.CheckRequest) checker.CheckResult {
 		return checker.CreateRuntimeErrorResult(CheckMaintained, e)
 	}
 	commitsWithinThreshold := 0
-	for _, commit := range commits {
-		if commit.CommittedDate.After(threshold) {
+	for i := range commits {
+		if commits[i].CommittedDate.After(threshold) {
 			commitsWithinThreshold++
 		}
 	}

checks/raw/code_review.go

@@ -31,63 +31,21 @@ func CodeReview(c clients.RepoClient) (checker.CodeReviewData, error) {
 		return checker.CodeReviewData{}, fmt.Errorf("%w", err)
 	}
 
-	oc := make(map[string]checker.DefaultBranchCommit)
-	for _, commit := range commits {
-		com := commitRequest(commit)
-		// Keep an index of commits by SHA.
-		oc[commit.SHA] = com
-	}
-
-	// Look at merge requests.
-	mrs, err := c.ListMergedPRs()
-	if err != nil {
-		return checker.CodeReviewData{}, fmt.Errorf("%w", err)
-	}
-
-	for i := range mrs {
-		mr := mrs[i]
-
-		if mr.MergedAt.IsZero() {
-			continue
-		}
-
-		// If the merge request is not a recent commit, skip.
-		com, exists := oc[mr.MergeCommit.SHA]
-
-		if exists {
-			// Sanity checks the logins are the same.
-			// TODO(#1543): re-enable this code.
-			//nolint:gocritic
-			/*if com.Committer.Login != mr.MergeCommit.Committer.Login {
-				return checker.CodeReviewData{}, sce.WithMessage(sce.ErrScorecardInternal,
-					fmt.Sprintf("commit login (%s) different from merge request commit login (%s)",
-						com.Committer.Login, mr.MergeCommit.Committer.Login))
-			}*/
-
-			// We have a recent merge request, update it.
-			com.MergeRequest = mergeRequest(&mr)
-			oc[com.SHA] = com
-		}
-	}
-
-	for _, v := range oc {
-		results = append(results, v)
-	}
-
-	if len(results) > 0 {
-		return checker.CodeReviewData{DefaultBranchCommits: results}, nil
+	for i := range commits {
+		results = append(results, getRawDataFrom(&commits[i]))
 	}
 
 	return checker.CodeReviewData{DefaultBranchCommits: results}, nil
 }
 
-func commitRequest(c clients.Commit) checker.DefaultBranchCommit {
+func getRawDataFrom(c *clients.Commit) checker.DefaultBranchCommit {
 	r := checker.DefaultBranchCommit{
 		Committer: checker.User{
 			Login: c.Committer.Login,
 		},
 		SHA:           c.SHA,
 		CommitMessage: c.Message,
+		MergeRequest:  mergeRequest(&c.AssociatedMergeRequest),
 	}
 
 	return r
@@ -99,9 +57,9 @@ func mergeRequest(mr *clients.PullRequest) *checker.MergeRequest {
 		Author: checker.User{
 			Login: mr.Author.Login,
 		},
-
-		Labels:  labels(mr),
-		Reviews: reviews(mr),
+		MergedAt: mr.MergedAt,
+		Labels:   labels(mr),
+		Reviews:  reviews(mr),
 	}
 	return &r
 }