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
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
