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
Improve code review check to account for diff author-committer usecase. #77
Conversation
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
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. |
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 ? |
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? |
@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. |
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. |
See