From 8b36ea430f4599cb7ae8516475eaf83f7470edc3 Mon Sep 17 00:00:00 2001 From: Vihang Mehta Date: Tue, 3 May 2022 21:51:02 -0700 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9C=A8=20Add=20support=20for=20Phabricat?= =?UTF-8?q?or=20as=20a=20code=20review=20system?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Vihang Mehta --- checks/evaluation/code_review.go | 40 +++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go index cf46d603af1..8069f987b8b 100644 --- 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,21 @@ 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, "\nReviewed By: ") { + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformPhabricator), + }) + return true + } + return false +} From 609a4bc49c3af6bbadae77a29d0a3302479ed164 Mon Sep 17 00:00:00 2001 From: Vihang Mehta Date: Wed, 4 May 2022 09:53:05 -0700 Subject: [PATCH 2/3] Also look for Differential Revision: to ensure that this repo uses Phabricator Signed-off-by: Vihang Mehta --- checks/evaluation/code_review.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go index 8069f987b8b..7b72de44fb6 100644 --- a/checks/evaluation/code_review.go +++ b/checks/evaluation/code_review.go @@ -203,7 +203,8 @@ func isReviewedOnPhabricator(c *checker.DefaultBranchCommit, dl checker.DetailLo } m := c.CommitMessage - if strings.Contains(m, "\nReviewed By: ") { + 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), }) From aef4781c8fc57e87ceaaf514750ef90cf4fc74c6 Mon Sep 17 00:00:00 2001 From: Vihang Mehta Date: Wed, 4 May 2022 10:51:23 -0700 Subject: [PATCH 3/3] Add some unit tests to cover Phabricator Review detection Signed-off-by: Vihang Mehta --- checks/code_review_test.go | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/checks/code_review_test.go b/checks/code_review_test.go index 340b74aebe6..5080f7c24ab 100644 --- 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 {