<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hooray! We're on github now. Next: there's the question of how to deal with pull requests?<div class=""><br class=""></div><div class="">It occurs to me that what we really need from our code review system is mainly one thing: the review <i class="">queue</i>: 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.</div><div class=""><br class=""></div><div class="">The operations on the queue are:</div><div class=""><br class=""></div><div class=""><ol class="MailOutline"><li class="">Proposing a change should put it into the queue.</li><li class="">Accepting a change should remove it from the queue.</li><li class="">Reviewing a change should remove it from the queue.</li><li class="">Responding to review feedback should re-add it to the queue.</li><li class="">A reviewer should be able to examine just the things in the queue so they can quickly grab the next one, without seeing noise.</li></ol><div class=""><br class=""></div><div class="">Our current workflow maps this into Trac via the following:</div></div><div class=""><br class=""></div><div class=""><ol class="MailOutline"><li class="">Proposing: add the "review" keyword.</li><li class="">Accepting: remove the "review" keyword, merge.</li><li class="">Reviewing: removing the "review" keyword, reassign</li><li class="">Responding: add the "review" keyword again</li><li class="">Viewing: <a href="https://twistedmatrix.com/trac/report/25" class="">https://twistedmatrix.com/trac/report/25</a></li></ol><div class=""><br class=""></div></div><div class="">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:</div><div class=""><div class=""><br class=""></div><div class=""><ol class="MailOutline"><li class="">Proposing: Just open a pull request. Any open pull request should be treated as part of the queue.</li><li class="">Accepting: A committer pushes the big green button; this </li><li class="">Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should <i class="">close</i> 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: <a href="https://github.com/markrwilliams/txghbot" class="">https://github.com/markrwilliams/txghbot</a></li><li class="">Responding: A submitter can open a new PR, or, once we start running txghbot, reopen their closed PR.</li><li class="">Viewing: <a href="https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed" class="">https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed</a></li></ol><div class=""><br class=""></div></div><div class="">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.</div><div class=""><br class=""></div><div class="">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: <a href="https://docs.travis-ci.com/user/docker/" class="">https://docs.travis-ci.com/user/docker/</a>. 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.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">Thoughts?</div><div class=""><br class=""></div><div class="">-glyph</div></div></body></html>