[Twisted-Python] Responding to PRs

[Glyph]

> On Jun 13, 2016, at 8:33 PM, Mark Williams <markrwilliams at gmail.com> wrote:
> 
> On Mon, Jun 13, 2016 at 02:35:01PM -0700, Glyph wrote:
>> 
>> Thus far all discussion has been on the mailing list.  I feel like putting it on the wiki would not be that useful, though; hopefully the discussion will continue for at most another month or two, and it's mostly just a question of coming to consensus about how exactly we're going to use the queue and model discrete review/resubmit events than doing a bunch of work.  https://github.com/markrwilliams/txghbot is probably most of what we need already, perhaps with one or two slight tweaks?
> 
> I'm the owner of txghbot.  I hope it ends up being useful for Twisted!

I strongly suspect that it will be the official solution.  Thanks so much for doing this - the existence of this code is a structural expression of the setup process which short-circuits me needing to read and process all the developer documentation ;).

> Despite the stern warning at the top of the README, the process it describes should result in a functioning GitHub bot.

Cool.  I will set that up soon.

> If you feel adventurous you can write your own webhooks.  They're Twisted plugins that implement this interface:
> 
> https://github.com/markrwilliams/txghbot/blob/master/txghbot/_core.py#L42-L79
> 
> I'll claim the lack of any abstraction over the GitHub Webhook API is intentional; this remains the authoritative documentation:
> 
> https://developer.github.com/webhooks/
> 
> Please let me know if there's anything I can do to make txghbot make PRs easier for everyone.  If it ends up being at all useful I'm happy to transfer ownership to the Twisted organization.


The first thing that comes to mind is that you could get Tom Prince to give you write access to txghbot on PyPI so that this could be 'pip install'ed like anything else, instead of cut live from master at HEAD :-).

I think that some folks were really over-focused on the whole "closing PRs" part of the previous discussion, when what I was really trying to get at was the need for a single, clear "review queue".  Something like txghbot is necessary no matter how we do it because non-(maintainer|reviewer|person with repo:write access|we need a good standard word for this)s will not be able to manipulate the labels.

At this point I think closing PRs creates more problems than it solves.  In particular:

It means that people can't push changes to their PRs to experiment with travis build status, because reviewers will keep closing them, which prevents further pushes from running in CI.
It means that force-pushing has weird and confusing side-effects (even weirder and confusinger than usual).  I am not a big fan of the rebase/force-push workflow, but judiciously used (i.e. with interactive rebases) it can help with splitting up big changes, and it also mitigates Github's atrocious management of the diff display of long-running branches.
It is definitely perceived as an "abnormal" way of doing reviews on Github.

Point 3 was not enough to dissuade me, but it certainly isn't a point in favor, and in combination with the other two I don't think it looks good.

So, instead of treating /pulls as our review queue, I think something like <https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-label%3A%22awaiting+review+response%22+-status%3Afailure> will have to do.  By subtracting a label from the review query rather than adding one, we can make it so that if our bot breaks down, contributors can still get their new submissions reviewed, and in the worst case where it is down when they want to resubmit, they can work around a broken bot by closing their "awaiting" PR and opening a new one.  (The ability to work around broken infra is important because I have to assume that things are going to go wrong with this, it being a distributed system on the public Internet.)

This means that txghbot's responsibility, instead of reopening the PR, will be to add and remove the 'awaiting review response' label.  If you wanted to write the actual plugin for doing that it might be helpful.  And then setting up a repo where we can play with it to test it out before turning it on for twisted/twisted :).

-glyph
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160613/0af604d7/attachment-0002.html>