On 27 December 2017 at 02:27, Glyph <glyph@twistedmatrix.com> wrote:
With more and more ecosystem projects on Github, more or less maintained by the same team that maintains Twisted proper, our "review keyword in trac" workflow is increasingly awkward.
Github itself has an individualized "review queue" of sorts - https://github.com/pulls/review-requested - where you can see open pull requests where your review has been requested. What makes this particularly interesting is that you can request review from a team, rather than from an individual.
Of course, like everything interesting (ahem, labels), you need repo:write permission to do this, so we'd still need a bot.
But!
It appears that the CODEOWNERS file - https://help.github.com/articles/about-codeowners/ - can automatically request code reviews when pull requests touch relevant code - so we could add "* @twisted/$PROJECT-reviewers" to each project on that file and, I believe, have a fairly orderly review queue that can be seen in a nice unified view by each org member, and by using -reviewers teams, we could allow for folks to join or leave any individual repo review responsibility that they want.
Actually setting up all the stuff needed to test this requires more time than I have on my hands at the moment. Does anyone else know for sure if this works as I imagine it would - specifically, that third party contributors would get an automatic review request opened, at least initially?
We might still need a bot to re-request reviews, but this seems like a much less ad-hoc way of doing this process migration than labels.
I think that we can give it a try as is easy to implement. ---------- I tried CODEOWNERS and for my project (which is multi platforms) and it does not work for me. The main reason is that GitHub is "simple" and a soon as you create the review it will think that you want someone to review it. Most of the time, the review should be requested after all the tests are green... and you create the PR to trigger the tests. With many platforms and with lots of tests, it is probable that a dev will not run all the test before the push. The tests might fail, and then you might need another day of work before requesting the review. ...or you create the PR at end of the day, and you want to do a self review in the morning. -------- And then you need the bot anyway for 're-review' ... so why not use the same bot for explicitly requesting when the review is ready. ------- You might have a team member just leaving a comment, without any intention to provide a reject/approve review in the future. With this action, the team is removed from "Reviewers", hence removed from the review queue.. and the PR might be lost. This can be solved by asking team members to just leave simple comments, and not review comments, but review comments are a nice way to organize multi-chuck comments. -------- I am +1 for moving all the tickets from Trac to GitHub so that we will get rid of the "review" keyword and the tentative ticket flow in Trac. It will be painful, but I think that it will make it easier to implement a dev process / workflow. In an ideal world, we would have plenty of time to document the workflow, and contributors would have plenty of time to read the dev documentation, but I think that we should recognize that everyone is short on time, so in order to make it easier for everybody to contribute I don't like that we depend that much on GitHub, but I don't see how there will be time to keep custom infrastructure. There is little time to do review, and at this stage I think that is more important for people to do reviews and work on patches rather than working on infrastructure. -- Adi Roiban