Skip to content

Commit

Permalink
🌱 Feature: Group commits into changesets (#2260)
Browse files Browse the repository at this point in the history 
* Group raw commits into changesets

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Add tests, fix golint

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Fix lint

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Address PR comments

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Fix test failures, remove unneeded fields from raw results

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Fix lint

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Fix tests

* Handle randomized order
* e2e

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Accept code reviews on any commit, not just HEAD

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

* Address PR comments

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

Signed-off-by: Raghav Kaul <raghavkaul@google.com>
Co-authored-by: Azeem Shaikh <azeemshaikh38@gmail.com>
  • Loading branch information
@raghavkaul @azeemshaikh38
raghavkaul and azeemshaikh38 authored Sep 20, 2022
1 parent 3629fd8 commit d75dea8
Show file tree
Hide file tree
Showing 11 changed files with 618 additions and 330 deletions.
17 changes: 16 additions & 1 deletion checker/raw_result.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,22 @@ type LicenseData struct {
// CodeReviewData contains the raw results
// for the Code-Review check.
type CodeReviewData struct {
DefaultBranchCommits []clients.Commit
DefaultBranchChangesets []Changeset
}
type ReviewPlatform = string

const (
ReviewPlatformGitHub ReviewPlatform = "GitHub"
ReviewPlatformProw ReviewPlatform = "Prow"
ReviewPlatformGerrit ReviewPlatform = "Gerrit"
ReviewPlatformPhabricator ReviewPlatform = "Phabricator"
ReviewPlatformPiper ReviewPlatform = "Piper"
)

type Changeset struct {
ReviewPlatform string
RevisionID string
Commits []clients.Commit
}

// ContributorsData represents contributor information.
Expand Down
2 changes: 1 addition & 1 deletion checks/code_review_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func TestCodereview(t *testing.T) {
},
},
expected: checker.CheckResult{
Score: 0,
Score: checker.MaxResultScore,
},
},
{
Expand Down
205 changes: 27 additions & 178 deletions checks/evaluation/code_review.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,210 +16,59 @@ package evaluation

import (
"fmt"
"strings"

"github.com/ossf/scorecard/v4/checker"
"github.com/ossf/scorecard/v4/clients"
sce "github.com/ossf/scorecard/v4/errors"
)

type reviewScore = int

// TODO(raghavkaul) More partial credit? E.g. approval from non-contributor, discussion liveness,
// number of resolved comments, number of approvers (more eyes on a project).
const (
reviewPlatformGitHub = "GitHub"
reviewPlatformProw = "Prow"
reviewPlatformGerrit = "Gerrit"
reviewPlatformPhabricator = "Phabricator"
reviewPlatformPiper = "Piper"
noReview reviewScore = 0 // No approving review before merge
changesReviewed reviewScore = 1 // Changes were reviewed
reviewedOutsideGithub reviewScore = 1 // Full marks until we can check review platforms outside of GitHub
)

// CodeReview applies the score policy for the Code-Review check.
func CodeReview(name string, dl checker.DetailLogger,
r *checker.CodeReviewData,
) checker.CheckResult {
func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData) checker.CheckResult {
if r == nil {
e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data")
return checker.CreateRuntimeErrorResult(name, e)
}

if len(r.DefaultBranchCommits) == 0 {
if len(r.DefaultBranchChangesets) == 0 {
return checker.CreateInconclusiveResult(name, "no commits found")
}

totalReviewed := map[string]int{
reviewPlatformGitHub: 0,
reviewPlatformProw: 0,
reviewPlatformGerrit: 0,
reviewPlatformPhabricator: 0,
reviewPlatformPiper: 0,
}

for i := range r.DefaultBranchCommits {
commit := r.DefaultBranchCommits[i]

rs := getApprovedReviewSystem(&commit, dl)
if rs == "" {
dl.Warn(&checker.LogMessage{
Text: fmt.Sprintf("no reviews found for commit: %s", commit.SHA),
})
continue
score := 0
numReviewed := 0
for i := range r.DefaultBranchChangesets {
score += reviewScoreForChangeset(&r.DefaultBranchChangesets[i])
if score >= changesReviewed {
numReviewed += 1
}

totalReviewed[rs]++
}
reason := fmt.Sprintf(
"%v out of last %v changesets reviewed before merge", numReviewed, len(r.DefaultBranchChangesets),
)

if totalReviewed[reviewPlatformGitHub] == 0 &&
totalReviewed[reviewPlatformGerrit] == 0 &&
totalReviewed[reviewPlatformProw] == 0 &&
totalReviewed[reviewPlatformPhabricator] == 0 && totalReviewed[reviewPlatformPiper] == 0 {
return checker.CreateMinScoreResult(name, "no reviews found")
}

totalCommits := len(r.DefaultBranchCommits)
// Consider a single review system.
nbReviews, reviewSystem := computeReviews(totalReviewed)
if nbReviews == totalCommits {
return checker.CreateMaxScoreResult(name,
fmt.Sprintf("all last %v commits are reviewed through %s", totalCommits, reviewSystem))
}

reason := fmt.Sprintf("%s code reviews found for %v commits out of the last %v", reviewSystem, nbReviews, totalCommits)
return checker.CreateProportionalScoreResult(name, reason, nbReviews, totalCommits)
return checker.CreateProportionalScoreResult(name, reason, score, len(r.DefaultBranchChangesets))
}

func computeReviews(m map[string]int) (int, string) {
n := 0
s := ""
for k, v := range m {
if v > n {
n = v
s = k
}
func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) {
if changeset.ReviewPlatform != "" && changeset.ReviewPlatform != checker.ReviewPlatformGitHub {
return reviewedOutsideGithub
}
return n, s
}

func isBot(name string) bool {
for _, substring := range []string{"bot", "gardener"} {
if strings.Contains(name, substring) {
return true
}
}
return false
}

func getApprovedReviewSystem(c *clients.Commit, dl checker.DetailLogger) string {
switch {
case isReviewedOnGitHub(c, dl):
return reviewPlatformGitHub
case isReviewedOnProw(c, dl):
return reviewPlatformProw
case isReviewedOnGerrit(c, dl):
return reviewPlatformGerrit
case isReviewedOnPhabricator(c, dl):
return reviewPlatformPhabricator
case isReviewedOnPiper(c, dl):
return reviewPlatformPiper
}
return ""
}

func isReviewedOnGitHub(c *clients.Commit, dl checker.DetailLogger) bool {
mr := c.AssociatedMergeRequest
if mr.MergedAt.IsZero() {
return false
}

for _, r := range mr.Reviews {
if r.State == "APPROVED" {
dl.Debug(&checker.LogMessage{
Text: fmt.Sprintf("commit %s was reviewed through %s #%d approved merge request",
c.SHA, reviewPlatformGitHub, mr.Number),
})
return true
}
}

// Check if the merge request 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 c.Committer.Login != "" &&
c.Committer.Login != mr.Author.Login {
dl.Debug(&checker.LogMessage{
Text: fmt.Sprintf("commit %s was reviewed through %s #%d merge request",
c.SHA, reviewPlatformGitHub, mr.Number),
})
return true
}

return false
}

func isReviewedOnProw(c *clients.Commit, 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
}

if !c.AssociatedMergeRequest.MergedAt.IsZero() {
for _, l := range c.AssociatedMergeRequest.Labels {
if l.Name == "lgtm" || l.Name == "approved" {
dl.Debug(&checker.LogMessage{
Text: fmt.Sprintf("commit %s review was through %s #%d approved merge request",
c.SHA, reviewPlatformProw, c.AssociatedMergeRequest.Number),
})
return true
for i := range changeset.Commits {
for _, review := range changeset.Commits[i].AssociatedMergeRequest.Reviews {
if review.State == "APPROVED" {
return changesReviewed
}
}
}
return false
}

func isReviewedOnGerrit(c *clients.Commit, 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.Message
if strings.Contains(m, "\nReviewed-on: ") &&
strings.Contains(m, "\nReviewed-by: ") {
dl.Debug(&checker.LogMessage{
Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformGerrit),
})
return true
}
return false
}

func isReviewedOnPhabricator(c *clients.Commit, 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.Message
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
}

func isReviewedOnPiper(c *clients.Commit, dl checker.DetailLogger) bool {
m := c.Message
if strings.Contains(m, "\nPiperOrigin-RevId: ") {
dl.Debug(&checker.LogMessage{
Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformPiper),
})
return true
}
return false
return noReview
}
9 changes: 4 additions & 5 deletions checks/evaluation/code_review_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,15 @@ func TestCodeReview(t *testing.T) {
{
name: "NoReviews",
expected: scut.TestReturn{
Score: checker.MinResultScore,
NumberOfWarn: 2,
Score: checker.MinResultScore,
},
rawData: &checker.CodeReviewData{
DefaultBranchCommits: []clients.Commit{
DefaultBranchChangesets: []checker.Changeset{
{
SHA: "1",
Commits: []clients.Commit{{SHA: "1"}},
},
{
SHA: "2",
Commits: []clients.Commit{{SHA: "1"}},
},
},
},
Expand Down
Loading

0 comments on commit d75dea8

Please sign in to comment.