Improve code review check to account for diff author-committer usecase.

[inferno-chromium]

See

$ go run . --repo=https://github.com/protocolbuffers/protobuf --show-details --checks=Code-Review
Starting [Code-Review]
Finished [Code-Review]

RESULTS
-------
Code-Review: Pass 9
    found different author and committer for pr: 8053
    found different author and committer for pr: 8052
    found review approved pr: 8048
    found review approved pr: 8045
    found different author and committer for pr: 8043
    found review approved pr: 8035
    found review approved pr: 8032
    found review approved pr: 8030
    found review approved pr: 8029
    found review approved pr: 8028
    found review approved pr: 8026
    found review approved pr: 8025
    found review approved pr: 8024
    found review approved pr: 8023
    found review approved pr: 8022
    found different author and committer for pr: 8014
    found different author and committer for pr: 8013
    found review approved pr: 8011
    found review approved pr: 8010
    found review approved pr: 8006
    found review approved pr: 8005
    found different author and committer for pr: 8003
    found review approved pr: 8000
    found different author and committer for pr: 7997
    github code reviews found

[inferno-chromium]

Without this, this check fails, E.g. PR protocolbuffers/protobuf#8052 , in github people are used to just committing without pressing the approve button. Thoughts ? This was pointed by Harvey (Envoy) in his doc as well.

[htuch]

FWIW, in Envoy we require approval before merging, GitHub can enforce this. Maybe we should ask Protobuf maintainers to put in place this check and the existing check is WAI?

[inferno-chromium]

FWIW, in Envoy we require approval before merging, GitHub can enforce this. Maybe we should ask Protobuf maintainers to put in place this check and the existing check is WAI?

Do you block merge and require explicit approval using https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/enabling-required-reviews-for-pull-requests. I want to be realistic here, we had enabled it on our three projects and there comes a time when someone has to merge a tiny fix, and this policy completely bricks it. So, most people would be disabling it or keep the default (disabled). In that case, people just press the "Merge pull request" directly (i do it too sometimes). I dont think we should penalize this. As long as someone is opening a pull request and then it gets committed by someone other than main author, it feels ok as a review. Thoughts @htuch ?

[htuch]

Yep, we do that. Why can't tiny fixes have a quick review? In Envoy, we will have approval/merge on almost everything; a subset of senior maintainers are able to override for situations that really demand it, but I'd say the vast, vast bulk of our PRs have an approval stamp prior to merge.

I agree with pragmatism; can we have some nuance in the scorecard here? I.e. "passes all PRs have approval stamp", "passes all PRs are merged by or reviewed by someone other than the author"? Maybe distinct checks?

[inferno-chromium]

@htuch - using --show-details shows the verbose mode and we can customize it to any message. Currently, it does show "found review approved pr: <pr_num>" and "found pr with committer different than author: <pr_num>" in details view on last 30 prs anaylzed.

[htuch]

Got it; one other consideration, we want to consume this data from a tool oriented flow; is it possible to get JSON/proto structured data? Making a distinction here without having to scrap via regex would be useful.

[inferno-chromium]

Got it; one other consideration, we want to consume this data from a tool oriented flow; is it possible to get JSON/proto structured data? Making a distinction here without having to scrap via regex would be useful.

yes --format=json exists, it was missing details field, so i just added it in latest patch.