From d75dea8a5895aab420b7c668ae24ebd43448b3f5 Mon Sep 17 00:00:00 2001 From: raghavkaul <8695110+raghavkaul@users.noreply.github.com> Date: Tue, 20 Sep 2022 13:53:11 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8C=B1=20Feature:=20Group=20commits=20int?= =?UTF-8?q?o=20changesets=20(#2260)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Group raw commits into changesets Signed-off-by: Raghav Kaul * Add tests, fix golint Signed-off-by: Raghav Kaul * Fix lint Signed-off-by: Raghav Kaul * Address PR comments Signed-off-by: Raghav Kaul * Fix test failures, remove unneeded fields from raw results Signed-off-by: Raghav Kaul * Fix lint Signed-off-by: Raghav Kaul * Fix tests * Handle randomized order * e2e Signed-off-by: Raghav Kaul * Accept code reviews on any commit, not just HEAD Signed-off-by: Raghav Kaul * Address PR comments Signed-off-by: Raghav Kaul Signed-off-by: Raghav Kaul Co-authored-by: Azeem Shaikh --- checker/raw_result.go | 17 +- checks/code_review_test.go | 2 +- checks/evaluation/code_review.go | 205 ++---------- checks/evaluation/code_review_test.go | 9 +- checks/raw/code_review.go | 146 ++++++++- checks/raw/code_review_test.go | 386 ++++++++++++++++++++--- cron/internal/format/json_raw_results.go | 77 ++--- e2e/code_review_test.go | 4 +- go.mod | 1 + go.sum | 2 + pkg/json_raw_results.go | 99 +++--- 11 files changed, 618 insertions(+), 330 deletions(-) diff --git a/checker/raw_result.go b/checker/raw_result.go index a7e87f3d6f8..2965646cfa6 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -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. diff --git a/checks/code_review_test.go b/checks/code_review_test.go index 26c8de7a03e..6c0c601fa3f 100644 --- a/checks/code_review_test.go +++ b/checks/code_review_test.go @@ -229,7 +229,7 @@ func TestCodereview(t *testing.T) { }, }, expected: checker.CheckResult{ - Score: 0, + Score: checker.MaxResultScore, }, }, { diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go index be42d3e1c9a..e631bd26580 100644 --- a/checks/evaluation/code_review.go +++ b/checks/evaluation/code_review.go @@ -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 } diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go index 44f0b11cbbe..106faf942c6 100644 --- a/checks/evaluation/code_review_test.go +++ b/checks/evaluation/code_review_test.go @@ -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"}}, }, }, }, diff --git a/checks/raw/code_review.go b/checks/raw/code_review.go index 60501bee691..68b0fea1cd8 100644 --- a/checks/raw/code_review.go +++ b/checks/raw/code_review.go @@ -16,6 +16,9 @@ package raw import ( "fmt" + "regexp" + "strconv" + "strings" "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/clients" @@ -29,7 +32,148 @@ func CodeReview(c clients.RepoClient) (checker.CodeReviewData, error) { return checker.CodeReviewData{}, fmt.Errorf("%w", err) } + changesets := getChangesets(commits) + + if err != nil { + return checker.CodeReviewData{}, fmt.Errorf("%w", err) + } + return checker.CodeReviewData{ - DefaultBranchCommits: commits, + DefaultBranchChangesets: changesets, }, nil } + +func getGithubRevisionID(c *clients.Commit) string { + mr := c.AssociatedMergeRequest + if !c.AssociatedMergeRequest.MergedAt.IsZero() && mr.Number != 0 { + return strconv.Itoa(mr.Number) + } + return "" +} + +func getProwRevisionID(c *clients.Commit) string { + mr := c.AssociatedMergeRequest + if !c.AssociatedMergeRequest.MergedAt.IsZero() { + for _, l := range c.AssociatedMergeRequest.Labels { + if l.Name == "lgtm" || l.Name == "approved" && mr.Number != 0 { + return strconv.Itoa(mr.Number) + } + } + } + + return "" +} + +func getGerritRevisionID(c *clients.Commit) string { + m := c.Message + if strings.Contains(m, "Reviewed-on:") && + strings.Contains(m, "Reviewed-by:") { + return c.SHA + } + return "" +} + +// Given m, a commit message, find the Phabricator revision ID in it. +func getPhabricatorRevisionID(c *clients.Commit) string { + m := c.Message + p, err := regexp.Compile(`Differential Revision:\s*(\w+)`) + if err != nil { + return "" + } + + match := p.FindStringSubmatch(m) + if match == nil || len(match) < 2 { + return "" + } + + return match[1] +} + +// Given m, a commit message, find the piper revision ID in it. +func getPiperRevisionID(c *clients.Commit) string { + m := c.Message + matchPiperRevID, err := regexp.Compile(`PiperOrigin-RevId:\s*(\d{3,})`) + if err != nil { + return "" + } + + match := matchPiperRevID.FindStringSubmatch(m) + if match == nil || len(match) < 2 { + return "" + } + + return match[1] +} + +type revisionInfo struct { + Platform checker.ReviewPlatform + ID string +} + +func detectCommitRevisionInfo(c *clients.Commit) revisionInfo { + if revisionID := getProwRevisionID(c); revisionID != "" { + return revisionInfo{checker.ReviewPlatformProw, revisionID} + } + if revisionID := getGithubRevisionID(c); revisionID != "" { + return revisionInfo{checker.ReviewPlatformGitHub, revisionID} + } + if revisionID := getPhabricatorRevisionID(c); revisionID != "" { + return revisionInfo{checker.ReviewPlatformPhabricator, revisionID} + } + if revisionID := getGerritRevisionID(c); revisionID != "" { + return revisionInfo{checker.ReviewPlatformGerrit, revisionID} + } + if revisionID := getPiperRevisionID(c); revisionID != "" { + return revisionInfo{checker.ReviewPlatformPiper, revisionID} + } + + return revisionInfo{} +} + +// Group commits by the changeset they belong to +// Commits must be in-order. +func getChangesets(commits []clients.Commit) []checker.Changeset { + changesets := []checker.Changeset{} + + if len(commits) == 0 { + return changesets + } + + changesetsByRevInfo := make(map[revisionInfo]checker.Changeset) + + for i := range commits { + rev := detectCommitRevisionInfo(&commits[i]) + + if changeset, ok := changesetsByRevInfo[rev]; !ok { + newChangeset := checker.Changeset{ + ReviewPlatform: rev.Platform, + RevisionID: rev.ID, + Commits: []clients.Commit{commits[i]}, + } + + changesetsByRevInfo[rev] = newChangeset + } else { + // Part of a previously found changeset. + changeset.Commits = append(changeset.Commits, commits[i]) + changesetsByRevInfo[rev] = changeset + } + } + + // Changesets are returned in map order (i.e. randomized) + for ri, cs := range changesetsByRevInfo { + // Ungroup all commits that don't have revision info + missing := revisionInfo{} + if ri == missing { + for i := range cs.Commits { + c := cs.Commits[i] + changesets = append(changesets, checker.Changeset{ + Commits: []clients.Commit{c}, + }) + } + } else { + changesets = append(changesets, cs) + } + } + + return changesets +} diff --git a/checks/raw/code_review_test.go b/checks/raw/code_review_test.go index 458860adecd..d41c735d588 100644 --- a/checks/raw/code_review_test.go +++ b/checks/raw/code_review_test.go @@ -15,58 +15,372 @@ package raw import ( - "errors" + "strings" "testing" + "time" - "github.com/golang/mock/gomock" - "github.com/google/go-cmp/cmp" + "golang.org/x/exp/slices" "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/clients" - mockrepo "github.com/ossf/scorecard/v4/clients/mockclients" ) +//nolint:gocritic +func assertCommitEq(t *testing.T, actual clients.Commit, expected clients.Commit) { + t.Helper() + + if actual.SHA != expected.SHA { + t.Fatalf("commit shas mismatched\na.sha: %s\nb.sha %s", actual.SHA, expected.SHA) + } +} + +func assertChangesetEq(t *testing.T, actual, expected checker.Changeset) { + t.Helper() + + if actual.ReviewPlatform != expected.ReviewPlatform { + t.Fatalf( + "changeset review platform mismatched\na.platform: %s\nb.platform: %s", + actual.ReviewPlatform, + expected.ReviewPlatform, + ) + } + + if actual.RevisionID != expected.RevisionID { + t.Fatalf( + "changeset revisionID mismatched\na.revid: %s\nb.revid %s", + actual.RevisionID, + expected.RevisionID, + ) + } + + if len(actual.Commits) != len(expected.Commits) { + t.Fatalf( + "changesets contain different numbers of commits\na:%d\nb:%d", + len(actual.Commits), + len(expected.Commits), + ) + } + + for i := range actual.Commits { + assertCommitEq(t, actual.Commits[i], expected.Commits[i]) + } +} + +func csless(a, b checker.Changeset) bool { + if cmp := strings.Compare(a.RevisionID, b.RevisionID); cmp != 0 { + return cmp < 0 + } + + return a.ReviewPlatform < b.ReviewPlatform +} + +func assertChangesetArrEq(t *testing.T, actual, expected []checker.Changeset) { + t.Helper() + + if len(actual) != len(expected) { + t.Fatalf("different number of changesets\na:%d\nb:%d", len(actual), len(expected)) + } + + slices.SortFunc(actual, csless) + slices.SortFunc(expected, csless) + + for i := range actual { + assertChangesetEq(t, actual[i], expected[i]) + } +} + // TestCodeReviews tests the CodeReviews function. -func TestCodeReview(t *testing.T) { +func Test_getChangesets(t *testing.T) { t.Parallel() + tests := []struct { - name string - want checker.CodeReviewData - wantErr bool + name string + commits []clients.Commit + expected []checker.Changeset }{ { - name: "Test_CodeReview", - wantErr: false, + name: "commit history squashed during merge", + commits: []clients.Commit{ + { + SHA: "a", + AssociatedMergeRequest: clients.PullRequest{Number: 3, MergedAt: time.Now()}, + }, + { + SHA: "b", + AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()}, + }, + { + SHA: "c", + AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()}, + }, + }, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "3", + Commits: []clients.Commit{{SHA: "a"}}, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "2", + Commits: []clients.Commit{{SHA: "b"}}, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "1", + Commits: []clients.Commit{{SHA: "c"}}, + }, + }, + }, + { + name: "commits in reverse chronological order", + commits: []clients.Commit{ + { + SHA: "a", + AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()}, + }, + { + SHA: "b", + AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()}, + }, + { + SHA: "c", + AssociatedMergeRequest: clients.PullRequest{Number: 3, MergedAt: time.Now()}, + }, + }, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "1", + Commits: []clients.Commit{ + { + SHA: "a", + AssociatedMergeRequest: clients.PullRequest{Number: 1}, + }, + }, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "2", + Commits: []clients.Commit{ + { + SHA: "b", + AssociatedMergeRequest: clients.PullRequest{Number: 2}, + }, + }, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "3", + Commits: []clients.Commit{ + { + SHA: "c", + AssociatedMergeRequest: clients.PullRequest{Number: 3}, + }, + }, + }, + }, }, { - name: "Want error", - wantErr: true, + name: "without commit squashing", + commits: []clients.Commit{ + { + SHA: "foo", + AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()}, + }, + { + SHA: "bar", + AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()}, + }, + { + SHA: "baz", + AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()}, + }, + }, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "1", + Commits: []clients.Commit{ + { + SHA: "foo", + AssociatedMergeRequest: clients.PullRequest{Number: 1}, + }, + }, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "2", + Commits: []clients.Commit{ + { + SHA: "bar", + AssociatedMergeRequest: clients.PullRequest{Number: 2}, + }, + { + SHA: "baz", + AssociatedMergeRequest: clients.PullRequest{Number: 2}, + }, + }, + }, + }, + }, + { + name: "some commits from external scm", + commits: []clients.Commit{ + { + Message: "\nDifferential Revision: 123\nReviewed By: user-123", + SHA: "abc", + }, + { + Message: "\nDifferential Revision: 158\nReviewed By: user-456", + SHA: "def", + }, + { + Message: "this one from github, but unrelated to prev", + AssociatedMergeRequest: clients.PullRequest{Number: 158, MergedAt: time.Now()}, + SHA: "fab", + }, + { + Message: "another from gh", + AssociatedMergeRequest: clients.PullRequest{Number: 158, MergedAt: time.Now()}, + SHA: "dab", + }, + }, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformPhabricator, + RevisionID: "123", + Commits: []clients.Commit{{SHA: "abc"}}, + }, + { + ReviewPlatform: checker.ReviewPlatformPhabricator, + RevisionID: "158", + Commits: []clients.Commit{{SHA: "def"}}, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "158", + Commits: []clients.Commit{ + {SHA: "fab"}, + {SHA: "dab"}, + }, + }, + }, + }, + { + name: "some commits from external scm with no revision id", + commits: []clients.Commit{ + { + Message: "first change\nReviewed-on: server.url \nReviewed-by:user-123", + SHA: "abc", + }, + { + Message: "followup\nReviewed-on: server.url \nReviewed-by:user-123", + SHA: "def", + }, + { + Message: "commit thru gh", + AssociatedMergeRequest: clients.PullRequest{Number: 3, MergedAt: time.Now()}, + SHA: "fab", + }, + }, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformGerrit, + RevisionID: "abc", + Commits: []clients.Commit{{SHA: "abc"}}, + }, + { + ReviewPlatform: checker.ReviewPlatformGerrit, + RevisionID: "def", + Commits: []clients.Commit{{SHA: "def"}}, + }, + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "3", + Commits: []clients.Commit{{SHA: "fab"}}, + }, + }, + }, + { + name: "external scm mirrored to github", + commits: []clients.Commit{ + { + Message: "\nDifferential Revision: 123\nReviewed By: user-123", + SHA: "abc", + }, + { + Message: "\nDifferential Revision: 158\nReviewed By: user-123", + SHA: "def", + }, + { + Message: "\nDifferential Revision: 2000\nReviewed By: user-456", + SHA: "fab", + }, + }, + expected: []checker.Changeset{ + { + RevisionID: "123", + ReviewPlatform: checker.ReviewPlatformPhabricator, + Commits: []clients.Commit{{SHA: "abc"}}, + }, + { + RevisionID: "158", + ReviewPlatform: checker.ReviewPlatformPhabricator, + Commits: []clients.Commit{{SHA: "def"}}, + }, + { + RevisionID: "2000", + ReviewPlatform: checker.ReviewPlatformPhabricator, + Commits: []clients.Commit{{SHA: "fab"}}, + }, + }, + }, + { + name: "external scm no squash", + commits: []clients.Commit{ + { + Message: "\nDifferential Revision: 123\nReviewed By: user-123", + SHA: "abc", + }, + { + Message: "\nDifferential Revision: 123\nReviewed By: user-123", + SHA: "def", + }, + { + Message: "\nDifferential Revision: 123\nReviewed By: user-456", + SHA: "fab", + }, + }, + expected: []checker.Changeset{ + { + RevisionID: "123", + ReviewPlatform: checker.ReviewPlatformPhabricator, + Commits: []clients.Commit{{SHA: "abc"}, {SHA: "def"}, {SHA: "fab"}}, + }, + }, + }, + { + name: "single changeset", + commits: []clients.Commit{ + { + SHA: "abc", + AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()}, + }, + }, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "1", + Commits: []clients.Commit{{SHA: "abc"}}, + }, + }, }, } + for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - mr := mockrepo.NewMockRepoClient(ctrl) - mr.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) { - if tt.wantErr { - //nolint - return nil, errors.New("error") - } - return []clients.Commit{ - { - SHA: "sha", - }, - }, nil - }) - result, err := CodeReview(mr) - if (err != nil) != tt.wantErr { - t.Errorf("CodeReview() error = %v, wantErr %v", err, tt.wantErr) - return - } else if !tt.wantErr && cmp.Equal(result, tt.want) { - t.Errorf(cmp.Diff(result, tt.want)) - } - }) + t.Logf("test: %s", tt.name) + changesets := getChangesets(tt.commits) + assertChangesetArrEq(t, changesets, tt.expected) } } diff --git a/cron/internal/format/json_raw_results.go b/cron/internal/format/json_raw_results.go index f6221445c52..f5b30a82b2d 100644 --- a/cron/internal/format/json_raw_results.go +++ b/cron/internal/format/json_raw_results.go @@ -74,20 +74,19 @@ 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 jsonDefaultBranchChangeset struct { + // ApprovedReviews *jsonApprovedReviews `json:"approved-reviews"` + RevisionID string `json:"number"` + ReviewPlatform string `json:"platform"` + Reviews []jsonReview `json:"reviews"` + Authors []jsonUser `json:"authors"` + Commits []jsonCommit `json:"commits"` } -type jsonDefaultBranchCommit struct { - // ApprovedReviews *jsonApprovedReviews `json:"approved-reviews"` - Committer jsonUser `json:"committer"` - MergeRequest *jsonMergeRequest `json:"mergeRequest"` - CommitMessage string `json:"commitMessage"` - SHA string `json:"sha"` +type jsonCommit struct { + Committer jsonUser `json:"committer"` + Message string `json:"message"` + SHA string `json:"sha"` // TODO: check runs, etc. } @@ -111,49 +110,35 @@ type jsonRawResults struct { DependencyUpdateTools []jsonTool `json:"dependencyUpdateTools"` // Branch protection settings for development and release branches. BranchProtections []jsonBranchProtection `json:"branchProtections"` - // Commits. - DefaultBranchCommits []jsonDefaultBranchCommit `json:"defaultBranchCommits"` + // Changesets + DefaultBranchChangesets []jsonDefaultBranchChangeset `json:"defaultBranchChangesets"` } //nolint:unparam func addCodeReviewRawResults(r *jsonScorecardRawResult, cr *checker.CodeReviewData) error { - r.Results.DefaultBranchCommits = []jsonDefaultBranchCommit{} - for i := range cr.DefaultBranchCommits { - commit := cr.DefaultBranchCommits[i] - com := jsonDefaultBranchCommit{ - Committer: jsonUser{ - Login: commit.Committer.Login, - }, - CommitMessage: commit.Message, - SHA: commit.SHA, - } - - // Merge request field. - mr := jsonMergeRequest{ - Number: commit.AssociatedMergeRequest.Number, - Author: jsonUser{ - Login: commit.AssociatedMergeRequest.Author.Login, - }, - } + r.Results.DefaultBranchChangesets = []jsonDefaultBranchChangeset{} - for _, l := range commit.AssociatedMergeRequest.Labels { - mr.Labels = append(mr.Labels, l.Name) - } + for i := range cr.DefaultBranchChangesets { + cs := cr.DefaultBranchChangesets[i] - for _, r := range commit.AssociatedMergeRequest.Reviews { - mr.Reviews = append(mr.Reviews, jsonReview{ - State: r.State, - Reviewer: jsonUser{ - Login: r.Author.Login, + // commits field + commits := []jsonCommit{} + for i := range cs.Commits { + commits = append(commits, jsonCommit{ + Committer: jsonUser{ + Login: commits[i].Committer.Login, }, + Message: commits[i].Message, + SHA: commits[i].SHA, }) } - com.MergeRequest = &mr - - com.CommitMessage = commit.Message - - r.Results.DefaultBranchCommits = append(r.Results.DefaultBranchCommits, com) + r.Results.DefaultBranchChangesets = append(r.Results.DefaultBranchChangesets, + jsonDefaultBranchChangeset{ + RevisionID: cs.RevisionID, + Commits: commits, + }, + ) } return nil } @@ -216,7 +201,7 @@ func addDependencyUpdateToolRawResults(r *jsonScorecardRawResult, return nil } -//nolint +//nolint:unparam func addBranchProtectionRawResults(r *jsonScorecardRawResult, bp *checker.BranchProtectionsData) error { r.Results.BranchProtections = []jsonBranchProtection{} for _, v := range bp.Branches { diff --git a/e2e/code_review_test.go b/e2e/code_review_test.go index 2fb2898c362..e9fc55f7717 100644 --- a/e2e/code_review_test.go +++ b/e2e/code_review_test.go @@ -48,7 +48,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() { expected := scut.TestReturn{ Error: nil, Score: checker.MinResultScore, - NumberOfWarn: 30, + NumberOfWarn: 0, NumberOfInfo: 0, NumberOfDebug: 0, } @@ -73,7 +73,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() { expected := scut.TestReturn{ Error: nil, Score: checker.MinResultScore, - NumberOfWarn: 30, + NumberOfWarn: 0, NumberOfInfo: 0, NumberOfDebug: 0, } diff --git a/go.mod b/go.mod index 91e18abb447..0664ec5808e 100644 --- a/go.mod +++ b/go.mod @@ -105,6 +105,7 @@ require ( github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f // indirect github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29 // indirect + golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 golang.org/x/net v0.0.0-20220722155237-a158d28d115b // indirect golang.org/x/oauth2 v0.0.0-20220718184931-c8730f7fcb92 // indirect golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4 // indirect diff --git a/go.sum b/go.sum index 6868ab2df38..1f268da301c 100644 --- a/go.sum +++ b/go.sum @@ -673,6 +673,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= +golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 h1:MDc5xs78ZrZr3HMQugiXOAkSZtfTpbJLDr/lwfgO53E= +golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 1e940f5ea20..637f18ab064 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -22,7 +22,6 @@ import ( "time" "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" sce "github.com/ossf/scorecard/v4/errors" ) @@ -103,20 +102,21 @@ type jsonCompany struct { // TODO: other info. } -//nolint:govet -type jsonMergeRequest struct { - Number int `json:"number"` - Labels []string `json:"labels"` - Reviews []jsonReview `json:"reviews"` - Author jsonUser `json:"author"` +type jsonDefaultBranchChangeset struct { + // ApprovedReviews *jsonApprovedReviews `json:"approved-reviews"` + RevisionID string `json:"number"` + ReviewPlatform string `json:"platform"` + Reviews []jsonReview `json:"reviews"` + Authors []jsonUser `json:"authors"` + Commits []jsonCommit `json:"commits"` + // TODO: check runs, etc. } -type jsonDefaultBranchCommit struct { - // ApprovedReviews *jsonApprovedReviews `json:"approved-reviews"` - MergeRequest *jsonMergeRequest `json:"mergeRequest"` - CommitMessage string `json:"commitMessage"` - SHA string `json:"sha"` - Committer jsonUser `json:"committer"` +type jsonCommit struct { + Message string `json:"message"` + SHA string `json:"sha"` + Committer jsonUser `json:"committer"` + // TODO: check runs, etc. } @@ -166,7 +166,6 @@ type jsonOssfBestPractices struct { Badge string `json:"badge"` } -//nolint type jsonLicense struct { File jsonFile `json:"file"` // TODO: add fields, like type of license, etc. @@ -184,7 +183,6 @@ type jsonWorkflowJob struct { ID *string `json:"id"` } -//nolint type jsonPackage struct { Name *string `json:"name,omitempty"` Job *jsonWorkflowJob `json:"job,omitempty"` @@ -223,7 +221,7 @@ type jsonTokenPermission struct { Type string `json:"type"` } -//nolint +//nolint:govet type jsonRawResults struct { // Workflow results. Workflows []jsonWorkflow `json:"workflows"` @@ -252,7 +250,7 @@ type jsonRawResults struct { // structure for it. Contributors jsonContributors `json:"Contributors"` // Commits. - DefaultBranchCommits []jsonDefaultBranchCommit `json:"defaultBranchCommits"` + DefaultBranchChangesets []jsonDefaultBranchChangeset `json:"defaultBranchChangesets"` // Archived status of the repo. ArchivedStatus jsonArchivedStatus `json:"archived"` // Repo creation time @@ -462,6 +460,7 @@ func (r *jsonScorecardRawResult) addSignedReleasesRawResults(sr *checker.SignedR return nil } +//nolint:unparam func (r *jsonScorecardRawResult) addMaintainedRawResults(mr *checker.MaintainedData) error { // Set archived status. r.Results.ArchivedStatus = jsonArchivedStatus{Status: mr.ArchivedStatus.Status} @@ -500,7 +499,7 @@ func (r *jsonScorecardRawResult) addMaintainedRawResults(mr *checker.MaintainedD r.Results.RecentIssues = append(r.Results.RecentIssues, issue) } - return r.setDefaultCommitData(mr.DefaultBranchCommits) + return nil } func getStrPtr(s string) *string { @@ -509,51 +508,31 @@ func getStrPtr(s string) *string { } // Function shared between addMaintainedRawResults() and addCodeReviewRawResults(). -func (r *jsonScorecardRawResult) setDefaultCommitData(commits []clients.Commit) error { - if len(r.Results.DefaultBranchCommits) > 0 { - return nil - } - - r.Results.DefaultBranchCommits = []jsonDefaultBranchCommit{} - for i := range commits { - commit := commits[i] - com := jsonDefaultBranchCommit{ - Committer: jsonUser{ - Login: commit.Committer.Login, - // Note: repo association is not available. We could - // try to use issue information to set it, but we're likely to miss - // many anyway. - }, - CommitMessage: commit.Message, - SHA: commit.SHA, - } - - // Merge request field. - mr := jsonMergeRequest{ - Number: commit.AssociatedMergeRequest.Number, - Author: jsonUser{ - Login: commit.AssociatedMergeRequest.Author.Login, - }, - } - - for _, l := range commit.AssociatedMergeRequest.Labels { - mr.Labels = append(mr.Labels, l.Name) - } - - for _, r := range commit.AssociatedMergeRequest.Reviews { - mr.Reviews = append(mr.Reviews, jsonReview{ - State: r.State, - Reviewer: jsonUser{ - Login: r.Author.Login, +func (r *jsonScorecardRawResult) setDefaultCommitData(changesets []checker.Changeset) error { + r.Results.DefaultBranchChangesets = []jsonDefaultBranchChangeset{} + + for i := range changesets { + cs := changesets[i] + + // commits field + commits := []jsonCommit{} + for j := range cs.Commits { + commit := cs.Commits[j] + commits = append(commits, jsonCommit{ + Committer: jsonUser{ + Login: commit.Committer.Login, }, + Message: commit.Message, + SHA: commit.SHA, }) } - com.MergeRequest = &mr - - com.CommitMessage = commit.Message - - r.Results.DefaultBranchCommits = append(r.Results.DefaultBranchCommits, com) + r.Results.DefaultBranchChangesets = append(r.Results.DefaultBranchChangesets, + jsonDefaultBranchChangeset{ + RevisionID: cs.RevisionID, + Commits: commits, + }, + ) } return nil } @@ -565,7 +544,7 @@ func (r *jsonScorecardRawResult) addOssfBestPracticesRawResults(cbp *checker.CII } func (r *jsonScorecardRawResult) addCodeReviewRawResults(cr *checker.CodeReviewData) error { - return r.setDefaultCommitData(cr.DefaultBranchCommits) + return r.setDefaultCommitData(cr.DefaultBranchChangesets) } //nolint:unparam