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
Co-authored PR suggestions #439
Comments
I think this makes sense and I've encountered the same problem before. I could see implementing this either as an allowance (e.g. One thing to consider is how GitHub represents multi-author commits in the API. If you can create something that looks like a suggestion by adding a |
Good call on the potential bypass. I will do some testing and try to put
together a PR. It will probably be a couple weeks before I can get time for
it.
Thanks!
…On Wed, Jun 15, 2022, 11:31 PM Billy Keyes ***@***.***> wrote:
I think this makes sense and I've encountered the same problem before. I
could see implementing this either as an allowance (e.g. allow_coauthors),
which would allow approval from contributors who co-authored a commit, or
as a suggestion-specific property (e.g. ignore_suggestions), which would
remove the users who provided the suggestion from the contributor list or
ignore the commit completely for computing contributors.
One thing to consider is how GitHub represents multi-author commits in the
API. If you can create something that looks like a suggestion by adding a
Co-authored-by line to a regular commit message, would that allow someone
to abuse this property to bypass approval? If so, it may be desirable to
also check the commit was created by the GitHub UI.
—
Reply to this email directly, view it on GitHub
<#439 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATEQT7HEPNYCAD6JHNQ6C2LVPKUYTANCNFSM5YZFJYPA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
After digging into this a bit today, it looks like co-authors aren't showing up in the github api responses at all and I was able to pass policies even though the approver was a co-author on one commit. I could have sworn when we tested this a few weeks ago co-authors couldn't approve, but it looks like it must have been something else. For now, I'm going to close this issue since it looks like things are already working as intended. |
I've noticed that suggestions on PRs result in a commit that includes a co-author. This co-author then ends up as a contributor. Github required reviews don't apply any restriction on reviews to users who have contributed in this way. However policy bot does.
From reading the code, it looks like as-is there is no way to differentiate between an external contributor who pushed a change to the branch and suggestions through the UI.
Would it be acceptable to include some kind of option that doesn't ban based on co-authorship -- basically don't consider commits with multiple contributors in the approver banning code. If this sounds like a plausible path forward, I can put together a PR and we can bikeshed what to call this option :)
My goal is to create a set of review requirements that is like required github required reviewers but with exceptions. This is the only blocker right now.
The text was updated successfully, but these errors were encountered: