[xwiki-devs] [Proposal] GitHub code reviews and protected branches

Hi devs, A while ago GitHub introduced several features that I think can help boost even more the code quality, reduce the bus factor, and make it more attractive to contribute to the project. = Protected branches = Branches can be configured as protected in several ways: * Require pull request reviews before merging ** no direct commits are allowed, everything must go through a pull request ** a pull request must be reviewed by at least one other trusted committer before it is allowed to be merged * Require status checks to pass before merging ** pull requests must pass automated checks, for example by being successfully built by a CI service * Restrict who can push to this branch ** official branches (master + stable-x.y) could be restricted to approved committers, but we could grant write access to all other interested developers more easily = Reviews for pull requests = Comments on all commits as well on pull requests have been supported since the early days of GitHub, but more recently it is easier to submit (and request) actual reviews for pull requests. Instead of simply commenting, and changing that special field on Jira, an official pull request status is supported, with three states: - review required - changes requested - approved PR authors can either await for reviews from anybody, or request someone in particular for a review. Committers can filter PRs by their status, and they can also show only the PRs that require their review. = Getting more eyeballs on the code = As Linus' Law states, "given enough eyeballs, all bugs are shallow". We've been experimenting with mandatory pull requests for a while in my other XWiki-based project, with great success so far. So here's a proposal for a different development workflow: 1. Set up 2 teams, one with master (senior) committers, fully vetted committers that have proven they understand the XWiki code, as well as the XWiki product, and one with junior committers 2. Protect the master branch. Require pull requests and reviews before any code is merged in it. Restrict push only to the master committers. 3. Everybody works on separate branches 4. Once the code is almost done, make a pull request and require reviews from two junior committers 4a. Or, instead of requesting, let committers volunteer themselves, but it's less likely that someone will volunteer on time 5. The junior committers must then carefully review the code, and either require changes, just comment, or approve the pull request 6. Once the junior committers have approved the pull request, the PR author must request review from a senior committer 7. The senior committer can either approve and merge the pull request, or send it back to step 3. e. We can define allowed exceptions: don't require PRs from senior committers if the fixes are really small and obvious, like fixing typos, or small and important bugfixes that must be merged quickly, but these should really be rare exceptions While it does sound like more work for an already overworked team, this has many benefits: * the code will have better quality * awareness of: ** what's new / changing ** how others are approaching a problem (especially juniors learning from seniors by being exposed to more code) ** the existing code, since the codebase is large and otherwise people have few occasions to look at many of the parts of XWiki * this means a larger bus factor for new code, and slowly increasing it for existing code that's being touched by one and reviewed by many * theoretically, less time spent doing reviews, since all committers should look over every commit anyway, but this way they are explicitly told when they should look, instead of wasting time reviewing work in progress * larger community, since people can more quickly become junior committers instead of having to invest many months of years of forkwork before committership * easier collaboration on code, since it's very hard to work on someone else's fork branches, but easy to work on an origin branch So, what do you think? -- Sergiu Dumitriu http://purl.org/net/sergiu/