Skip to content

Commit

Permalink
Merge branch 'main' into bug/wp
Browse files Browse the repository at this point in the history
  • Loading branch information
@laurentsimon
laurentsimon authored Feb 2, 2022
2 parents 211e625 + 9037444 commit 2906a3e
Show file tree
Hide file tree
Showing 10 changed files with 536 additions and 224 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/main.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,5 @@ jobs:
run: |
go env -w GOFLAGS=-mod=mod
go install github.com/google/addlicense@2fe3ee94479d08be985a84861de4e6b06a1c7208
addlicense -ignore "**/script-empty.sh" -ignore "pkg/testdata/*" -ignore "checks/testdata/*" -l apache -c 'Security Scorecard Authors' -v *
addlicense -ignore "**/script-empty.sh" -ignore "pkg/testdata/**" -ignore "checks/testdata/**" -l apache -c 'Security Scorecard Authors' -v *
git diff --exit-code
39 changes: 36 additions & 3 deletions checker/raw_result.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
199 changes: 14 additions & 185 deletions checks/code_review.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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)
}
Loading

0 comments on commit 2906a3e

Please sign in to comment.