Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Ignore Bot commits when checking for Code Review #2311

Closed
wants to merge 2 commits into from

Conversation

raghavkaul
Copy link
Contributor

What kind of change does this PR introduce?

Ignore bot commits when checking for code review. This is an update to the evaluation logic as well as clients. It checks for Bots without relying solely on committer names.

What is the current behavior?

Today, bot commits that are pushed directly to the default branch are considered unreviewed.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #2302

Special notes for your reviewer

Raw result format is also updated.

* Update clients
* Update scoring

Signed-off-by: Raghav Kaul <[email protected]>
* More detailed information for Changesets
* If there's no Revision ID, use the Commit SHA instead

Signed-off-by: Raghav Kaul <[email protected]>
@raghavkaul raghavkaul temporarily deployed to integration-test September 29, 2022 21:04 Inactive
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #2311 (5b490d3) into main (29893ae) will decrease coverage by 0.12%.
The diff coverage is 13.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2311      +/-   ##
==========================================
- Coverage   40.51%   40.39%   -0.13%     
==========================================
  Files         112      112              
  Lines        8818     8858      +40     
==========================================
+ Hits         3573     3578       +5     
- Misses       4984     5018      +34     
- Partials      261      262       +1     

@github-actions
Copy link

Integration tests success for
[5b490d3]
(https://github.com/ossf/scorecard/actions/runs/3154580487)

Copy link
Contributor

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left one main question

@@ -57,6 +58,13 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
}

func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) {
if changeset.Commits[0].Committer.IsBot {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it right to always ignore these bots? Say, AllStar / Dependabot sends a PR to fix something.. should it be manually reviewed? Ignoring unilaterally creates a precedent that any random bots someone sets up won't need reviews.
Is there a way to distinguish?

Also, I think it's feasible for someone to set the bot name themselves when creating a PR / commit.

Shall we use the User.id instead https://github.com/raghavkaul/scorecard/blob/5b490d3e583bbb1b774a75c459caaf0496292a90/clients/user.go#L23 to identify "real" bots? I think this one cannot be forged... but should verify my claim.

If this works, shall we keep a list of "acceptable" bots? And add it to our documentation of the check?

@github-actions
Copy link

Stale pull request message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Code Review Check handle commits made by version bump bots
2 participants