[Twisted-Python] overview: new review queue venue

[Glyph]

Hooray!  We're on github now.  Next: there's the question of how to deal with pull requests?

It occurs to me that what we really need from our code review system is mainly one thing: the review queue: a single place for reviewers to look to find things that need to be reviewed.  This is important because proposed changes need to be responded to in a timely manner, so that the code in them doesn't get stale, and so that contributors don't get frustrated.  We have limited resources for doing so, of course, so sometimes we fall short of this objective, but the point is we need to apply our limited resources to it.

The operations on the queue are:

Proposing a change should put it into the queue.
Accepting a change should remove it from the queue.
Reviewing a change should remove it from the queue.
Responding to review feedback should re-add it to the queue.
A reviewer should be able to examine just the things in the queue so they can quickly grab the next one, without seeing noise.

Our current workflow maps this into Trac via the following:

Proposing: add the "review" keyword.
Accepting: remove the "review" keyword, merge.
Reviewing: removing the "review" keyword, reassign
Responding: add the "review" keyword again
Viewing: https://twistedmatrix.com/trac/report/25

It is therefore tempting to map it into GitHub via labels and webhooks and bot workflows.  However, I think a better mapping would be this:

Proposing: Just open a pull request.  Any open pull request should be treated as part of the queue.
Accepting: A committer pushes the big green button; this 
Reviewing: This is the potentially slightly odd part.  I believe a review that doesn't result in acceptance should close the PR.  We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit.  Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot
Responding: A submitter can open a new PR, or, once we start running txghbot, reopen their closed PR.
Viewing: https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed

The one thing that this workflow is missing from trac is a convenient way for committers, having eyeballed a patch for any obvious malware, to send it to the buildbots.

We could also potentially just replace our buildbot build farm with a combination of appveyor and travis-ci; this would remove FreeBSD from our list of supported platforms, as well as eliminating a diversity of kernel integrations.  However, for the stuff that doesn't work in containers (mostly inotify) we could run one builder on non-container-based infrastructure, and for everything else (integrating with different system toolchains) we can test using Docker: https://docs.travis-ci.com/user/docker/.  I am very much on the fence about this, since I don't want to move backwards in terms of our test coverage, but this would accelerate the contribution process so much that it's probably worth discussing.

10 years ago or so, we would routinely discover kernel bugs in our integration test harness and they would be different on different platforms.  But today's platform realities are considerably less harsh, since there are a lot more entities in the ecosystem that have taken responsibility for testing that layer of the stack; I couldn't find anything since 2008 or so where we really saw a difference between Fedora and Ubuntu at the kernel level, for example.

Thoughts?

-glyph
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://twistedmatrix.com/pipermail/twisted-python/attachments/20160521/3b678224/attachment-0001.html>