✨ Add support for Phabricator as a code review system (#1884)

* ✨ Add support for Phabricator as a code review system Signed-off-by: Vihang Mehta <[email protected]> * Also look for Differential Revision: to ensure that this repo uses Phabricator Signed-off-by: Vihang Mehta <[email protected]> * Add some unit tests to cover Phabricator Review detection Signed-off-by: Vihang Mehta <[email protected]>
ossf · May 5, 2022 · 72086c9 · 72086c9
1 parent f779fb8
commit 72086c9
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 8 deletions.
diff --git a/checks/code_review_test.go b/checks/code_review_test.go
@@ -187,6 +187,51 @@ func TestCodereview(t *testing.T) {
 				Score: 5,
 			},
 		},
+		{
+			name: "Valid Phabricator commit",
+			commits: []clients.Commit{
+				{
+					SHA: "sha",
+					Committer: clients.User{
+						Login: "bob",
+					},
+					Message: "Title\nReviewed By: alice\nDifferential Revision: PHAB234",
+				},
+			},
+			expected: checker.CheckResult{
+				Score: 10,
+			},
+		},
+		{
+			name: "Phabricator like, missing differential",
+			commits: []clients.Commit{
+				{
+					SHA: "sha",
+					Committer: clients.User{
+						Login: "bob",
+					},
+					Message: "Title\nReviewed By: alice",
+				},
+			},
+			expected: checker.CheckResult{
+				Score: 0,
+			},
+		},
+		{
+			name: "Phabricator like, missing reviewed by",
+			commits: []clients.Commit{
+				{
+					SHA: "sha",
+					Committer: clients.User{
+						Login: "bob",
+					},
+					Message: "Title\nDifferential Revision: PHAB234",
+				},
+			},
+			expected: checker.CheckResult{
+				Score: 0,
+			},
+		},
 	}
 
 	for _, tt := range tests {

diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go
@@ -23,9 +23,10 @@ import (
 )
 
 var (
-	reviewPlatformGitHub = "GitHub"
-	reviewPlatformProw   = "Prow"
-	reviewPlatformGerrit = "Gerrit"
+	reviewPlatformGitHub      = "GitHub"
+	reviewPlatformProw        = "Prow"
+	reviewPlatformGerrit      = "Gerrit"
+	reviewPlatformPhabricator = "Phabricator"
 )
 
 // CodeReview applies the score policy for the Code-Review check.
@@ -42,10 +43,11 @@ func CodeReview(name string, dl checker.DetailLogger,
 	}
 
 	totalReviewed := map[string]int{
-		// The 3 platforms we support.
-		reviewPlatformGitHub: 0,
-		reviewPlatformProw:   0,
-		reviewPlatformGerrit: 0,
+		// The 4 platforms we support.
+		reviewPlatformGitHub:      0,
+		reviewPlatformProw:        0,
+		reviewPlatformGerrit:      0,
+		reviewPlatformPhabricator: 0,
 	}
 
 	for i := range r.DefaultBranchCommits {
@@ -64,7 +66,8 @@ func CodeReview(name string, dl checker.DetailLogger,
 
 	if totalReviewed[reviewPlatformGitHub] == 0 &&
 		totalReviewed[reviewPlatformGerrit] == 0 &&
-		totalReviewed[reviewPlatformProw] == 0 {
+		totalReviewed[reviewPlatformProw] == 0 &&
+		totalReviewed[reviewPlatformPhabricator] == 0 {
 		return checker.CreateMinScoreResult(name, "no reviews found")
 	}
 
@@ -111,6 +114,9 @@ func getApprovedReviewSystem(c *checker.DefaultBranchCommit, dl checker.DetailLo
 
 	case isReviewedOnGerrit(c, dl):
 		return reviewPlatformGerrit
+
+	case isReviewedOnPhabricator(c, dl):
+		return reviewPlatformPhabricator
 	}
 
 	return ""
@@ -187,3 +193,22 @@ func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 	}
 	return false
 }
+
+func isReviewedOnPhabricator(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
+	if isBot(c.Committer.Login) {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("skip commit %s from bot account: %s", c.SHA, c.Committer.Login),
+		})
+		return true
+	}
+
+	m := c.CommitMessage
+	if strings.Contains(m, "\nDifferential Revision: ") &&
+		strings.Contains(m, "\nReviewed By: ") {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformPhabricator),
+		})
+		return true
+	}
+	return false
+}