mirror of
https://github.com/ossf/scorecard.git
synced 2024-11-04 03:52:31 +03:00
✨ 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
This commit is contained in:
laurentsimon
GitHub
parent
7032b1910e
commit
9037444513
@ -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.
|
||||
|
||||
@ -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)
|
||||
// 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 checker.CreateRuntimeErrorResult(CheckCodeReview, err)
|
||||
e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
|
||||
return checker.CreateRuntimeErrorResult(CheckCodeReview, e)
|
||||
}
|
||||
|
||||
// Review messages.
|
||||
hintScore, hintReason, err := commitMessageHints(c)
|
||||
if err != nil {
|
||||
return checker.CreateRuntimeErrorResult(CheckCodeReview, err)
|
||||
// Return raw results.
|
||||
if c.RawResults != nil {
|
||||
c.RawResults.CodeReviewResults = rawData
|
||||
return checker.CheckResult{}
|
||||
}
|
||||
|
||||
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()
|
||||
if err != nil {
|
||||
return 0, "", sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
|
||||
}
|
||||
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 checker.InconclusiveResultScore, fmt.Sprintf("no %v commits found", reviewName), nil
|
||||
// Return the score evaluation.
|
||||
return evaluation.CodeReview(CheckCodeReview, c.Dlogger, &rawData)
|
||||
}
|
||||
|
||||
@ -69,7 +69,7 @@ func TestCodereview(t *testing.T) {
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "Valid PR's and commits as not a bot",
|
||||
name: "Valid GitHub PR",
|
||||
prs: []clients.PullRequest{
|
||||
{
|
||||
Number: 1,
|
||||
@ -79,10 +79,8 @@ func TestCodereview(t *testing.T) {
|
||||
State: "APPROVED",
|
||||
},
|
||||
},
|
||||
Labels: []clients.Label{
|
||||
{
|
||||
Name: "lgtm",
|
||||
},
|
||||
MergeCommit: clients.Commit{
|
||||
SHA: "sha",
|
||||
},
|
||||
},
|
||||
},
|
||||
@ -100,23 +98,50 @@ func TestCodereview(t *testing.T) {
|
||||
Pass: true,
|
||||
},
|
||||
},
|
||||
|
||||
{
|
||||
name: "Valid PR's and commits as bot",
|
||||
name: "Valid Prow PR as not a bot",
|
||||
prs: []clients.PullRequest{
|
||||
{
|
||||
Number: 1,
|
||||
MergedAt: time.Now(),
|
||||
Reviews: []clients.Review{
|
||||
{
|
||||
State: "APPROVED",
|
||||
},
|
||||
},
|
||||
Labels: []clients.Label{
|
||||
{
|
||||
Name: "lgtm",
|
||||
},
|
||||
},
|
||||
MergeCommit: clients.Commit{
|
||||
SHA: "sha",
|
||||
},
|
||||
},
|
||||
},
|
||||
commits: []clients.Commit{
|
||||
{
|
||||
SHA: "sha",
|
||||
Committer: clients.User{
|
||||
Login: "user",
|
||||
},
|
||||
},
|
||||
},
|
||||
|
||||
expected: checker.CheckResult{
|
||||
Score: 10,
|
||||
Pass: true,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "Valid Prow PR and commits as bot",
|
||||
prs: []clients.PullRequest{
|
||||
{
|
||||
Number: 1,
|
||||
MergedAt: time.Now(),
|
||||
Labels: []clients.Label{
|
||||
{
|
||||
Name: "lgtm",
|
||||
},
|
||||
},
|
||||
MergeCommit: clients.Commit{
|
||||
SHA: "sha",
|
||||
},
|
||||
},
|
||||
},
|
||||
commits: []clients.Commit{
|
||||
@ -133,47 +158,11 @@ func TestCodereview(t *testing.T) {
|
||||
Pass: true,
|
||||
},
|
||||
},
|
||||
|
||||
{
|
||||
name: "Valid PR's and commits not yet merged",
|
||||
prs: []clients.PullRequest{
|
||||
{
|
||||
Number: 1,
|
||||
Reviews: []clients.Review{
|
||||
{
|
||||
State: "APPROVED",
|
||||
},
|
||||
},
|
||||
Labels: []clients.Label{
|
||||
{
|
||||
Name: "lgtm",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
commits: []clients.Commit{
|
||||
{
|
||||
SHA: "sha",
|
||||
Committer: clients.User{
|
||||
Login: "bot",
|
||||
},
|
||||
},
|
||||
},
|
||||
|
||||
expected: checker.CheckResult{
|
||||
Score: -1,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "Valid PR's and commits with merged by someone else",
|
||||
prs: []clients.PullRequest{
|
||||
{
|
||||
Number: 1,
|
||||
Reviews: []clients.Review{
|
||||
{
|
||||
State: "APPROVED",
|
||||
},
|
||||
},
|
||||
Labels: []clients.Label{
|
||||
{
|
||||
Name: "lgtm",
|
||||
@ -182,7 +171,7 @@ func TestCodereview(t *testing.T) {
|
||||
MergeCommit: clients.Commit{
|
||||
SHA: "sha",
|
||||
Committer: clients.User{
|
||||
Login: "bob",
|
||||
Login: "bot",
|
||||
},
|
||||
},
|
||||
},
|
||||
@ -191,13 +180,51 @@ func TestCodereview(t *testing.T) {
|
||||
{
|
||||
SHA: "sha",
|
||||
Committer: clients.User{
|
||||
Login: "bot",
|
||||
Login: "bob",
|
||||
},
|
||||
},
|
||||
},
|
||||
|
||||
expected: checker.CheckResult{
|
||||
Score: -1,
|
||||
Score: 0,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "2 PRs 2 review on GitHub",
|
||||
prs: []clients.PullRequest{
|
||||
{
|
||||
Number: 1,
|
||||
MergedAt: time.Now(),
|
||||
Reviews: []clients.Review{
|
||||
{
|
||||
State: "APPROVED",
|
||||
},
|
||||
},
|
||||
MergeCommit: clients.Commit{
|
||||
SHA: "sha",
|
||||
Committer: clients.User{
|
||||
Login: "bot",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
commits: []clients.Commit{
|
||||
{
|
||||
SHA: "sha",
|
||||
Committer: clients.User{
|
||||
Login: "bob",
|
||||
},
|
||||
},
|
||||
{
|
||||
SHA: "sha2",
|
||||
Committer: clients.User{
|
||||
Login: "bob",
|
||||
},
|
||||
},
|
||||
},
|
||||
|
||||
expected: checker.CheckResult{
|
||||
Score: 5,
|
||||
},
|
||||
},
|
||||
}
|
||||
@ -225,7 +252,7 @@ func TestCodereview(t *testing.T) {
|
||||
RepoClient: mockRepo,
|
||||
}
|
||||
req.Dlogger = &scut.TestDetailLogger{}
|
||||
res := DoesCodeReview(&req)
|
||||
res := CodeReview(&req)
|
||||
|
||||
if tt.err != nil {
|
||||
if res.Error2 == nil {
|
||||
|
||||
197
checks/evaluation/code_review.go
Normal file
197
checks/evaluation/code_review.go
Normal file
@ -0,0 +1,197 @@
|
||||
// Copyright 2021 Security Scorecard Authors
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package evaluation
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"strings"
|
||||
|
||||
"github.com/ossf/scorecard/v4/checker"
|
||||
sce "github.com/ossf/scorecard/v4/errors"
|
||||
)
|
||||
|
||||
var (
|
||||
reviewPlatformGitHub = "GitHub"
|
||||
reviewPlatformProw = "Prow"
|
||||
reviewPlatformGerrit = "Gerrit"
|
||||
)
|
||||
|
||||
// CodeReview applies the score policy for the Code-Review check.
|
||||
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)
|
||||
}
|
||||
|
||||
totalCommits := 0
|
||||
totalReviewed := map[string]int{
|
||||
// The 3 platforms we support.
|
||||
reviewPlatformGitHub: 0,
|
||||
reviewPlatformProw: 0,
|
||||
reviewPlatformGerrit: 0,
|
||||
}
|
||||
|
||||
for i := range r.DefaultBranchCommits {
|
||||
commit := r.DefaultBranchCommits[i]
|
||||
|
||||
// New commit to consider.
|
||||
totalCommits++
|
||||
|
||||
rs := getApprovedReviewSystem(&commit, dl)
|
||||
// No commits.
|
||||
if rs == "" {
|
||||
continue
|
||||
}
|
||||
|
||||
totalReviewed[rs]++
|
||||
}
|
||||
|
||||
if totalCommits == 0 {
|
||||
return checker.CreateInconclusiveResult(name, "no commits found")
|
||||
}
|
||||
|
||||
if totalReviewed[reviewPlatformGitHub] == 0 &&
|
||||
totalReviewed[reviewPlatformGerrit] == 0 &&
|
||||
totalReviewed[reviewPlatformProw] == 0 {
|
||||
// Only show all warnings if all fail.
|
||||
// We should not show warning if at least one succeeds, as this is confusing.
|
||||
for k := range totalReviewed {
|
||||
dl.Warn3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("no %s reviews found", k),
|
||||
})
|
||||
}
|
||||
|
||||
return checker.CreateMinScoreResult(name, "no reviews found")
|
||||
}
|
||||
|
||||
// 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)
|
||||
}
|
||||
|
||||
func computeReviews(m map[string]int) (int, string) {
|
||||
n := 0
|
||||
s := ""
|
||||
for k, v := range m {
|
||||
if v > n {
|
||||
n = v
|
||||
s = k
|
||||
}
|
||||
}
|
||||
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 *checker.DefaultBranchCommit, dl checker.DetailLogger) string {
|
||||
switch {
|
||||
case isReviewedOnGitHub(c, dl):
|
||||
return reviewPlatformGitHub
|
||||
|
||||
case isReviewedOnProw(c, dl):
|
||||
return reviewPlatformProw
|
||||
|
||||
case isReviewedOnGerrit(c, dl):
|
||||
return reviewPlatformGerrit
|
||||
}
|
||||
|
||||
return ""
|
||||
}
|
||||
|
||||
func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
|
||||
mr := c.MergeRequest
|
||||
if mr == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
for _, r := range mr.Reviews {
|
||||
if r.State == "APPROVED" {
|
||||
dl.Debug3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("%s #%d merge request approved",
|
||||
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.Debug3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("%s #%d merge request approved",
|
||||
reviewPlatformGitHub, mr.Number),
|
||||
})
|
||||
return true
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
|
||||
if isBot(c.Committer.Login) {
|
||||
dl.Debug3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
|
||||
})
|
||||
return true
|
||||
}
|
||||
|
||||
if c.MergeRequest != nil {
|
||||
for _, l := range c.MergeRequest.Labels {
|
||||
if l == "lgtm" || l == "approved" {
|
||||
dl.Debug3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("%s #%d merge request approved",
|
||||
reviewPlatformProw, c.MergeRequest.Number),
|
||||
})
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
|
||||
if isBot(c.Committer.Login) {
|
||||
dl.Debug3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
|
||||
})
|
||||
return true
|
||||
}
|
||||
|
||||
m := c.CommitMessage
|
||||
if strings.Contains(m, "\nReviewed-on: ") &&
|
||||
strings.Contains(m, "\nReviewed-by: ") {
|
||||
dl.Debug3(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("%s commit approved", reviewPlatformGerrit),
|
||||
})
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
133
checks/raw/code_review.go
Normal file
133
checks/raw/code_review.go
Normal file
@ -0,0 +1,133 @@
|
||||
// Copyright 2020 Security Scorecard Authors
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package raw
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
|
||||
"github.com/ossf/scorecard/v4/checker"
|
||||
"github.com/ossf/scorecard/v4/clients"
|
||||
)
|
||||
|
||||
// CodeReview retrieves the raw data for the Code-Review check.
|
||||
func CodeReview(c clients.RepoClient) (checker.CodeReviewData, error) {
|
||||
results := []checker.DefaultBranchCommit{}
|
||||
|
||||
// Look at the latest commits.
|
||||
commits, err := c.ListCommits()
|
||||
if err != nil {
|
||||
return checker.CodeReviewData{}, fmt.Errorf("%w", err)
|
||||
}
|
||||
|
||||
oc := make(map[string]checker.DefaultBranchCommit)
|
||||
for _, commit := range commits {
|
||||
com := commitRequest(commit)
|
||||
// Keep an index of commits by SHA.
|
||||
oc[commit.SHA] = com
|
||||
}
|
||||
|
||||
// Look at merge requests.
|
||||
mrs, err := c.ListMergedPRs()
|
||||
if err != nil {
|
||||
return checker.CodeReviewData{}, fmt.Errorf("%w", err)
|
||||
}
|
||||
|
||||
for i := range mrs {
|
||||
mr := mrs[i]
|
||||
|
||||
if mr.MergedAt.IsZero() {
|
||||
continue
|
||||
}
|
||||
|
||||
// If the merge request is not a recent commit, skip.
|
||||
com, exists := oc[mr.MergeCommit.SHA]
|
||||
|
||||
if exists {
|
||||
// Sanity checks the logins are the same.
|
||||
// TODO(#1543): re-enable this code.
|
||||
//nolint:gocritic
|
||||
/*if com.Committer.Login != mr.MergeCommit.Committer.Login {
|
||||
return checker.CodeReviewData{}, sce.WithMessage(sce.ErrScorecardInternal,
|
||||
fmt.Sprintf("commit login (%s) different from merge request commit login (%s)",
|
||||
com.Committer.Login, mr.MergeCommit.Committer.Login))
|
||||
}*/
|
||||
|
||||
// We have a recent merge request, update it.
|
||||
com.MergeRequest = mergeRequest(&mr)
|
||||
oc[com.SHA] = com
|
||||
}
|
||||
}
|
||||
|
||||
for _, v := range oc {
|
||||
results = append(results, v)
|
||||
}
|
||||
|
||||
if len(results) > 0 {
|
||||
return checker.CodeReviewData{DefaultBranchCommits: results}, nil
|
||||
}
|
||||
|
||||
return checker.CodeReviewData{DefaultBranchCommits: results}, nil
|
||||
}
|
||||
|
||||
func commitRequest(c clients.Commit) checker.DefaultBranchCommit {
|
||||
r := checker.DefaultBranchCommit{
|
||||
Committer: checker.User{
|
||||
Login: c.Committer.Login,
|
||||
},
|
||||
SHA: c.SHA,
|
||||
CommitMessage: c.Message,
|
||||
}
|
||||
|
||||
return r
|
||||
}
|
||||
|
||||
func mergeRequest(mr *clients.PullRequest) *checker.MergeRequest {
|
||||
r := checker.MergeRequest{
|
||||
Number: mr.Number,
|
||||
Author: checker.User{
|
||||
Login: mr.Author.Login,
|
||||
},
|
||||
|
||||
Labels: labels(mr),
|
||||
Reviews: reviews(mr),
|
||||
}
|
||||
return &r
|
||||
}
|
||||
|
||||
func labels(mr *clients.PullRequest) []string {
|
||||
labels := []string{}
|
||||
for _, l := range mr.Labels {
|
||||
labels = append(labels, l.Name)
|
||||
}
|
||||
return labels
|
||||
}
|
||||
|
||||
func reviews(mr *clients.PullRequest) []checker.Review {
|
||||
reviews := []checker.Review{}
|
||||
for _, m := range mr.Reviews {
|
||||
r := checker.Review{
|
||||
State: m.State,
|
||||
}
|
||||
|
||||
if m.Author != nil &&
|
||||
m.Author.Login != "" {
|
||||
r.Reviewer = checker.User{
|
||||
Login: m.Author.Login,
|
||||
}
|
||||
}
|
||||
reviews = append(reviews, r)
|
||||
}
|
||||
return reviews
|
||||
}
|
||||
@ -84,7 +84,10 @@ type graphqlData struct {
|
||||
} `graphql:"labels(last: $labelsToAnalyze)"`
|
||||
Reviews struct {
|
||||
Nodes []struct {
|
||||
State githubv4.String
|
||||
State githubv4.String
|
||||
Author struct {
|
||||
Login githubv4.String
|
||||
}
|
||||
}
|
||||
} `graphql:"reviews(last: $reviewsToAnalyze)"`
|
||||
}
|
||||
@ -211,18 +214,29 @@ func pullRequestsFrom(data *graphqlData, repoOwner, repoName string) []clients.P
|
||||
Committer: clients.User{
|
||||
Login: string(commit.Author.User.Login),
|
||||
},
|
||||
SHA: string(pr.MergeCommit.Oid),
|
||||
},
|
||||
}
|
||||
|
||||
for _, label := range pr.Labels.Nodes {
|
||||
toAppend.Labels = append(toAppend.Labels, clients.Label{
|
||||
Name: string(label.Name),
|
||||
})
|
||||
}
|
||||
for _, review := range pr.Reviews.Nodes {
|
||||
toAppend.Reviews = append(toAppend.Reviews, clients.Review{
|
||||
r := clients.Review{
|
||||
State: string(review.State),
|
||||
})
|
||||
}
|
||||
|
||||
a := clients.User{
|
||||
Login: string(review.Author.Login),
|
||||
}
|
||||
if a.Login != "" {
|
||||
r.Author = &a
|
||||
}
|
||||
toAppend.Reviews = append(toAppend.Reviews, r)
|
||||
}
|
||||
|
||||
ret = append(ret, toAppend)
|
||||
break
|
||||
}
|
||||
|
||||
@ -37,5 +37,6 @@ type Label struct {
|
||||
|
||||
// Review represents a PR review.
|
||||
type Review struct {
|
||||
State string
|
||||
Author *User
|
||||
State string
|
||||
}
|
||||
|
||||
@ -47,11 +47,11 @@ var _ = Describe("E2E TEST:CodeReview", func() {
|
||||
expected := scut.TestReturn{
|
||||
Error: nil,
|
||||
Score: checker.MinResultScore,
|
||||
NumberOfWarn: 0,
|
||||
NumberOfInfo: 3,
|
||||
NumberOfWarn: 3,
|
||||
NumberOfInfo: 0,
|
||||
NumberOfDebug: 0,
|
||||
}
|
||||
result := checks.DoesCodeReview(&req)
|
||||
result := checks.CodeReview(&req)
|
||||
Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
|
||||
Expect(repoClient.Close()).Should(BeNil())
|
||||
})
|
||||
|
||||
@ -64,6 +64,33 @@ type jsonBranchProtection struct {
|
||||
Name string `json:"name"`
|
||||
}
|
||||
|
||||
type jsonReview struct {
|
||||
Reviewer jsonUser `json:"reviewer"`
|
||||
State string `json:"state"`
|
||||
}
|
||||
|
||||
type jsonUser struct {
|
||||
Login string `json:"login"`
|
||||
}
|
||||
|
||||
//nolint:govet
|
||||
type jsonMergeRequest struct {
|
||||
Number int `json:"number"`
|
||||
Labels []string `json:"labels"`
|
||||
Reviews []jsonReview `json:"reviews"`
|
||||
Author jsonUser `json:"author"`
|
||||
}
|
||||
|
||||
type jsonDefaultBranchCommit struct {
|
||||
// ApprovedReviews *jsonApprovedReviews `json:"approved-reviews"`
|
||||
Committer jsonUser `json:"committer"`
|
||||
MergeRequest *jsonMergeRequest `json:"merge-request"`
|
||||
CommitMessage string `json:"commit-message"`
|
||||
SHA string `json:"sha"`
|
||||
|
||||
// TODO: check runs, etc.
|
||||
}
|
||||
|
||||
type jsonRawResults struct {
|
||||
DatabaseVulnerabilities []jsonDatabaseVulnerability `json:"database-vulnerabilities"`
|
||||
// List of binaries found in the repo.
|
||||
@ -76,6 +103,52 @@ type jsonRawResults struct {
|
||||
DependencyUpdateTools []jsonTool `json:"dependency-update-tools"`
|
||||
// Branch protection settings for development and release branches.
|
||||
BranchProtections []jsonBranchProtection `json:"branch-protections"`
|
||||
// Commits.
|
||||
DefaultBranchCommits []jsonDefaultBranchCommit `json:"default-branch-commits"`
|
||||
}
|
||||
|
||||
//nolint:unparam
|
||||
func (r *jsonScorecardRawResult) addCodeReviewRawResults(cr *checker.CodeReviewData) error {
|
||||
r.Results.DefaultBranchCommits = []jsonDefaultBranchCommit{}
|
||||
for _, commit := range cr.DefaultBranchCommits {
|
||||
com := jsonDefaultBranchCommit{
|
||||
Committer: jsonUser{
|
||||
Login: commit.Committer.Login,
|
||||
},
|
||||
CommitMessage: commit.CommitMessage,
|
||||
SHA: commit.SHA,
|
||||
}
|
||||
|
||||
// Merge request field.
|
||||
if commit.MergeRequest != nil {
|
||||
mr := jsonMergeRequest{
|
||||
Number: commit.MergeRequest.Number,
|
||||
Author: jsonUser{
|
||||
Login: commit.MergeRequest.Author.Login,
|
||||
},
|
||||
}
|
||||
|
||||
if len(commit.MergeRequest.Labels) > 0 {
|
||||
mr.Labels = commit.MergeRequest.Labels
|
||||
}
|
||||
|
||||
for _, r := range commit.MergeRequest.Reviews {
|
||||
mr.Reviews = append(mr.Reviews, jsonReview{
|
||||
State: r.State,
|
||||
Reviewer: jsonUser{
|
||||
Login: r.Reviewer.Login,
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
com.MergeRequest = &mr
|
||||
}
|
||||
|
||||
com.CommitMessage = commit.CommitMessage
|
||||
|
||||
r.Results.DefaultBranchCommits = append(r.Results.DefaultBranchCommits, com)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
type jsonDatabaseVulnerability struct {
|
||||
@ -195,6 +268,11 @@ func (r *jsonScorecardRawResult) fillJSONRawResults(raw *checker.RawResults) err
|
||||
return sce.WithMessage(sce.ErrScorecardInternal, err.Error())
|
||||
}
|
||||
|
||||
// Code-Review.
|
||||
if err := r.addCodeReviewRawResults(&raw.CodeReviewResults); err != nil {
|
||||
return sce.WithMessage(sce.ErrScorecardInternal, err.Error())
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user