From 44d0e0299aca3a7212068a1e8e2317e4f84e46a1 Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Fri, 16 Dec 2022 21:25:11 +0000 Subject: [PATCH] Address PR comments * Test coverage * Docs * Raw results Signed-off-by: Raghav Kaul --- checks/evaluation/code_review_test.go | 16 ++++++++++++++++ clients/githubrepo/graphql.go | 2 ++ docs/checks.md | 8 +++++++- docs/checks/internal/checks.yaml | 8 +++++++- pkg/json_raw_results.go | 2 ++ 5 files changed, 34 insertions(+), 2 deletions(-) diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go index 0d6e136dda09..0eda37fb9acf 100644 --- a/checks/evaluation/code_review_test.go +++ b/checks/evaluation/code_review_test.go @@ -63,6 +63,22 @@ func TestCodeReview(t *testing.T) { }, }, }, + { + name: "Unreviewed human and bot changes", + expected: scut.TestReturn{ + Score: checker.MinResultScore, + }, + rawData: &checker.CodeReviewData{ + DefaultBranchChangesets: []checker.Changeset{ + { + Commits: []clients.Commit{{SHA: "a", Committer: clients.User{IsBot: true}}}, + }, + { + Commits: []clients.Commit{{SHA: "b"}}, + }, + }, + }, + }, { name: "all human changesets reviewed, missing review on bot changeset", expected: scut.TestReturn{ diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go index 784413fadfbe..eb3302f27046 100644 --- a/clients/githubrepo/graphql.go +++ b/clients/githubrepo/graphql.go @@ -397,6 +397,8 @@ func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commi string(pr.Repository.Name) != repoName { continue } + // ResourcePath: e.g., for dependabot, "/apps/dependabot", or "/apps/renovate" + // Path that can be appended to "https://github.com" for a Github resource openedByBot := strings.HasPrefix(string(pr.Author.ResourcePath), "/apps/") associatedPR = clients.PullRequest{ Number: int(pr.Number), diff --git a/docs/checks.md b/docs/checks.md index ae885e1b8432..1056304005b4 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -207,7 +207,8 @@ malicious code (either as a malicious contributor or as an attacker who has subverted a contributor's account), because a reviewer might detect the subversion. -The check determines whether the most recent (~30) commits have a Github-approved review +The check determines whether the most recent changes (over the last ~30 commits) have +an approval on GitHub or if the merger is different from the committer (implicit review). It also performs a similar check for reviews using [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels @@ -215,6 +216,11 @@ performs a similar check for reviews using If recent changes are solely bot activity (e.g. dependabot, renovatebot, or custom bots), the check returns inconclusively. +Scoring is leveled instead of proportional to make the check more predictable. +If any bot-originated changes are unreviewed, 3 points are deducted. If any human +changes are unreviewed, 7 points are deducted if a single change is unreviewed, and +another 3 are deducted if multiple changes are unreviewed. + Note: Requiring reviews for all changes is infeasible for some projects, such as those with only one active participant. Even a project with multiple active contributors may not have enough active participation to be able to require diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 9b1c84225542..3e28e405c44f 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -307,7 +307,8 @@ checks: subverted a contributor's account), because a reviewer might detect the subversion. - The check determines whether the most recent (~30) commits have a Github-approved review + The check determines whether the most recent changes (over the last ~30 commits) have + an approval on GitHub or if the merger is different from the committer (implicit review). It also performs a similar check for reviews using [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels @@ -315,6 +316,11 @@ checks: If recent changes are solely bot activity (e.g. dependabot, renovatebot, or custom bots), the check returns inconclusively. + Scoring is leveled instead of proportional to make the check more predictable. + If any bot-originated changes are unreviewed, 3 points are deducted. If any human + changes are unreviewed, 7 points are deducted if a single change is unreviewed, and + another 3 are deducted if multiple changes are unreviewed. + Note: Requiring reviews for all changes is infeasible for some projects, such as those with only one active participant. Even a project with multiple active contributors may not have enough active participation to be able to require diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 216588ea24b7..af4ebaacb083 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -86,6 +86,7 @@ type jsonUser struct { // Companies refer to a claim by a user in their profile. Companies []jsonCompany `json:"company,omitempty"` NumContributions int `json:"NumContributions,omitempty"` + IsBot bool `json:"isBot"` } type jsonContributors struct { @@ -555,6 +556,7 @@ func (r *jsonScorecardRawResult) setDefaultCommitData(changesets []checker.Chang State: r.State, Reviewer: jsonUser{ Login: r.Author.Login, + IsBot: r.Author.IsBot, }, }) }