✨ Raw data for code review check (#1505)

* separate code review's eval and check

* missing file

* add comments

* fix

* fix

* linter

* fixes

* fix

* linter

* linter

* linter

* draft

* fixes

* fixes

* simplify

* update date

* rem comments

* typo

* linter

* typo

* linter

Author: laurentsimon

checker/raw_result.go

@@ -22,6 +22,13 @@ type RawResults struct {
 	SecurityPolicyResults       SecurityPolicyData
 	DependencyUpdateToolResults DependencyUpdateToolData
 	BranchProtectionResults     BranchProtectionsData
+	CodeReviewResults           CodeReviewData
+}
+
+// CodeReviewData contains the raw results
+// for the Code-Review check.
+type CodeReviewData struct {
+	DefaultBranchCommits []DefaultBranchCommit
 }
 
 // VulnerabilitiesData contains the raw results
@@ -84,7 +91,7 @@ type Tool struct {
 	Runs []Run
 	// Issues created by the tool.
 	Issues []Issue
-	// Merges requests created by the tool.
+	// Merge requests created by the tool.
 	MergeRequests []MergeRequest
 	Name          string
 	URL           string
@@ -104,10 +111,36 @@ type Issue struct {
 	// TODO: add fields, e.g., state=[opened|closed]
 }
 
+// DefaultBranchCommit represents a commit
+// to the default branch.
+type DefaultBranchCommit struct {
+	// Fields below are taken directly from cloud
+	// version control systems, e.g. GitHub.
+	SHA           string
+	CommitMessage string
+	MergeRequest  *MergeRequest
+	Committer     User
+}
+
 // MergeRequest represents a merge request.
+//nolint:govet
 type MergeRequest struct {
-	URL string
-	// TODO: add fields, e.g., State=["merged"|"closed"]
+	Number  int
+	Labels  []string
+	Reviews []Review
+	Author  User
+}
+
+// Review represent a review using the built-in review system.
+type Review struct {
+	Reviewer User
+	State    string
+	// TODO(Review): add fields here if needed.
+}
+
+// User represent a user.
+type User struct {
+	Login string
 }
 
 // File represents a file.

checks/code_review.go

@@ -15,10 +15,9 @@
 package checks
 
 import (
-	"fmt"
-	"strings"
-
 	"github.com/ossf/scorecard/v4/checker"
+	"github.com/ossf/scorecard/v4/checks/evaluation"
+	"github.com/ossf/scorecard/v4/checks/raw"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
@@ -27,196 +26,26 @@ const CheckCodeReview = "Code-Review"
 
 //nolint:gochecknoinits
 func init() {
-	if err := registerCheck(CheckCodeReview, DoesCodeReview); err != nil {
+	if err := registerCheck(CheckCodeReview, CodeReview); err != nil {
 		// this should never happen
 		panic(err)
 	}
 }
 
-// DoesCodeReview attempts to determine whether a project requires review before code gets merged.
-// It uses a set of heuristics:
-// - Looking at the repo configuration to see if reviews are required.
-// - Checking if most of the recent merged PRs were "Approved".
-// - Looking for other well-known review labels.
-func DoesCodeReview(c *checker.CheckRequest) checker.CheckResult {
-	// GitHub reviews.
-	ghScore, ghReason, err := githubCodeReview(c)
-	if err != nil {
-		return checker.CreateRuntimeErrorResult(CheckCodeReview, err)
-	}
-
-	// Review messages.
-	hintScore, hintReason, err := commitMessageHints(c)
-	if err != nil {
-		return checker.CreateRuntimeErrorResult(CheckCodeReview, err)
-	}
-
-	score, reason := selectBestScoreAndReason(hintScore, ghScore, hintReason, ghReason, c.Dlogger)
-
-	// Prow CI/CD.
-	prowScore, prowReason, err := prowCodeReview(c)
-	if err != nil {
-		return checker.CreateRuntimeErrorResult(CheckCodeReview, err)
-	}
-
-	score, reason = selectBestScoreAndReason(prowScore, score, prowReason, reason, c.Dlogger)
-	if score == checker.MinResultScore {
-		c.Dlogger.Info3(&checker.LogMessage{
-			Text: reason,
-		})
-		return checker.CreateResultWithScore(CheckCodeReview, "no reviews detected", score)
-	}
-
-	if score == checker.InconclusiveResultScore {
-		return checker.CreateInconclusiveResult(CheckCodeReview, "no reviews detected")
-	}
-
-	return checker.CreateResultWithScore(CheckCodeReview, checker.NormalizeReason(reason, score), score)
-}
-
-//nolint
-func selectBestScoreAndReason(s1, s2 int, r1, r2 string,
-	dl checker.DetailLogger) (int, string) {
-	if s1 > s2 {
-		dl.Info3(&checker.LogMessage{
-			Text: r2,
-		})
-		return s1, r1
-	}
-
-	dl.Info3(&checker.LogMessage{
-		Text: r1,
-	})
-	return s2, r2
-}
-
-//nolint
-func githubCodeReview(c *checker.CheckRequest) (int, string, error) {
-	// Look at some merged PRs to see if they were reviewed.
-	totalMerged := 0
-	totalReviewed := 0
-	prs, err := c.RepoClient.ListMergedPRs()
+// CodeReview will check if the maintainers perform code review.
+func CodeReview(c *checker.CheckRequest) checker.CheckResult {
+	rawData, err := raw.CodeReview(c.RepoClient)
 	if err != nil {
-		return 0, "", sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
+		e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
+		return checker.CreateRuntimeErrorResult(CheckCodeReview, e)
 	}
-	for _, pr := range prs {
-		if pr.MergedAt.IsZero() {
-			continue
-		}
-		totalMerged++
-
-		// Check if the PR is approved by a reviewer.
-		foundApprovedReview := false
-		for _, r := range pr.Reviews {
-			if r.State == "APPROVED" {
-				c.Dlogger.Debug3(&checker.LogMessage{
-					Text: fmt.Sprintf("found review approved pr: %d", pr.Number),
-				})
-				totalReviewed++
-				foundApprovedReview = true
-				break
-			}
-		}
-
-		// Check if the PR is committed by someone other than author. this is kind
-		// of equivalent to a review and is done several times on small prs to save
-		// time on clicking the approve button.
-		if !foundApprovedReview &&
-			pr.MergeCommit.Committer.Login != "" &&
-			pr.MergeCommit.Committer.Login != pr.Author.Login {
-			c.Dlogger.Debug3(&checker.LogMessage{
-				Text: fmt.Sprintf("found PR#%d with committer (%s) different from author (%s)",
-					pr.Number, pr.Author.Login, pr.MergeCommit.Committer.Login),
-			})
-			totalReviewed++
-			foundApprovedReview = true
-		}
-
-		if !foundApprovedReview {
-			c.Dlogger.Debug3(&checker.LogMessage{
-				Text: fmt.Sprintf("merged PR without code review: %d", pr.Number),
-			})
-		}
-
-	}
-
-	return createReturn("GitHub", totalReviewed, totalMerged)
-}
-
-//nolint
-func prowCodeReview(c *checker.CheckRequest) (int, string, error) {
-	// Look at some merged PRs to see if they were reviewed
-	totalMerged := 0
-	totalReviewed := 0
-	prs, err := c.RepoClient.ListMergedPRs()
-	if err != nil {
-		sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
-	}
-	for _, pr := range prs {
-		if pr.MergedAt.IsZero() {
-			continue
-		}
-		totalMerged++
-		for _, l := range pr.Labels {
-			if l.Name == "lgtm" || l.Name == "approved" {
-				totalReviewed++
-				break
-			}
-		}
-	}
-
-	return createReturn("Prow", totalReviewed, totalMerged)
-}
-
-//nolint
-func commitMessageHints(c *checker.CheckRequest) (int, string, error) {
-	commits, err := c.RepoClient.ListCommits()
-	if err != nil {
-		return checker.InconclusiveResultScore, "",
-			sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("Client.Repositories.ListCommits: %v", err))
-	}
-
-	total := 0
-	totalReviewed := 0
-	for _, commit := range commits {
-		isBot := false
-		committer := commit.Committer.Login
-		for _, substring := range []string{"bot", "gardener"} {
-			if strings.Contains(committer, substring) {
-				isBot = true
-				break
-			}
-		}
-		if isBot {
-			c.Dlogger.Debug3(&checker.LogMessage{
-				Text: fmt.Sprintf("skip commit from bot account: %s", committer),
-			})
-			continue
-		}
-
-		total++
-
-		// check for gerrit use via Reviewed-on and Reviewed-by
-		commitMessage := commit.Message
-		if strings.Contains(commitMessage, "\nReviewed-on: ") &&
-			strings.Contains(commitMessage, "\nReviewed-by: ") {
-			c.Dlogger.Debug3(&checker.LogMessage{
-				Text: fmt.Sprintf("Gerrit review found for commit '%s'", commit.SHA),
-			})
-			totalReviewed++
-			continue
-		}
-	}
-
-	return createReturn("Gerrit", totalReviewed, total)
-}
 
-//nolint
-func createReturn(reviewName string, reviewed, total int) (int, string, error) {
-	if total > 0 {
-		reason := fmt.Sprintf("%s code reviews found for %v commits out of the last %v", reviewName, reviewed, total)
-		return checker.CreateProportionalScore(reviewed, total), reason, nil
+	// Return raw results.
+	if c.RawResults != nil {
+		c.RawResults.CodeReviewResults = rawData
+		return checker.CheckResult{}
 	}
 
-	return checker.InconclusiveResultScore, fmt.Sprintf("no %v commits found", reviewName), nil
+	// Return the score evaluation.
+	return evaluation.CodeReview(CheckCodeReview, c.Dlogger, &rawData)
 }