17 May
2017
17 May
'17
3:30 p.m.
Hi Sergiu,
On 16 May 2017, at 07:36, Sergiu Dumitriu
<sergiu(a)xwiki.org> wrote:
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
How is that different from what we’re currently doing?
Right now we have:
* Committers (they’ve been voted committers because they have proven their knowledge on
our platform + long term dedication to the project). They’re the “senior” of your team
* Contributors, who need to go through PR to get their code in. They’re the “juniors” of
your other team.
2. Protect the master branch. Require pull requests
and reviews before
any code is merged in it. Restrict push only to the master committers.
Main pros for me:
* we can ask for the CI to pass before merging, thus disturbing less other committers.
* easier to converge for releases but *only* if we’re ready to drop features. If we’re not
then it’s the opposite and makes it harder to converge.
Main cons:
* takes a LOT of time before code is merged. You can be sure that it’ll need several hours
before it gets merged and that you’ll have moved on to something else when the comment
comes in. And since your new work has an important chance of being related you might end
up chaining the PRs.
3. Everybody works on separate branches
How does the CI ensure that all branches are merged before the tests are executed?
Example: I’m working on something and I’m modifying some @Unstable API. Someone else is
working with the previous API. We’ll only discover the issue late when both PRs are
merged.
4. Once the code is almost done, make a pull request
and require reviews
from two junior committers
Why from junior committers? So you’d be asking junior devs to review senior devs’s code?
How can they do it and achieve it? While the junior may learn stuff, they’ll most likely
have no knowledge to do a proper review.
FYI just had lunch with some Docker guys today who had this setup in place. It was a
nightmare and they had to drop it after a few months. Was just too painful to ask for 2
reviews. They’re now down to 1 reviewer.
How do you require reviews? How can you ensure they’ll have the time right now? It seems
this would be painful.
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
wow … so 3 reviewers!
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,
It’s a lot more work indeed.
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
In theory I agree. In practice I have the feeling this means slowing down the pace of
development in favor of more shared knowledge.
Personally I know that I cannot review any PR if I don’t understand what the dev is doing
and the code alone is almost never enough. I need to talk to the person to understand the
goal. Otherwise I just do cosmetic comments which don’t help spread the knowledge. That’s
unless it’s on a topic that I completely master.
Globally I have the feeling it would slow us down substantially (or it doesn’t that it
wouldn’t bring anything more at the expense of more constraints). I like the idea of
spreading the knowledge but I’m not sure this would work for us. However, I like to try
new stuff and decide based on real feedback.
* larger community, since people can more quickly
become junior
committers instead of having to invest many months of years of forkwork
before committership
This I don’t understand. Contributors can already do PR so I don’t see a difference (see
my point above). Also this would mean defining on what new rule you make someone a
committer and the committership rules would probably need to change (voting, etc).
Also right now we have xwiki-contrib which is open to anyone in term of committership and
we’re pushing more and more to it so all this might not be actually needed (except maybe
on contrib if too many errors are introduced by code that is not reviewed - I’m more
worried about this than for the xwiki repos TBH).
* 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?
Sounds pretty ambitious to me :)
The part that I prefer is the part about the CI building before merging (see above) but
even that is not so simple. Can we make dependent jobs also build so that all tests are
executed including functional tests in xwiki-enterprise?
I really don’t like the 3 reviews asked (even 2). One would already be a lot IMO. It’d
mean a lot of pinging around to find someone to review your PR :)
I’d be willing to try it out if others are interested but for a limited amount of time
based on these changes:
* Same committership rules as now
* Master is protected and all code needs to go through PR
* Only 1 reviewer is required. This reviewer would do the merge.
* The onus is on the committer to find a reviewer but everyone should play the game. IMO
that’s the hard part and we risk seeing always the same person being asked to review.
* We continue to do time-boxing for releases and we stop merging PRs 2 days before the
release.
* We try this for 3 releases (i.e. 3 months - June-August) and then do a postmortem to
decide if we continue or not
* We need to define some PR rules and reviewer "power". What if I comment to an
existing committer that they need to split their PR into several ones so that I can
understand it better? Would they be willing to do that? What if the PR is good but could
be better by following better some coding style. Are we willing to annoy the committer and
refuse the PR for that reason? What if the PR has no tests?
In conclusion I’m pretty sure this would slow down a lot and we’re already a pretty small
team. The real question is whether more knowledge sharing and higher code quality is more
important than pace of development or not. It’s hard to decide on that (at least for me).
Also I don’t like changing something for the sake of changing it. Are there any recent
experience that make you think that we need this change?
In any case, thanks for starting this interesting topic.
Thanks
-Vincent
--
Sergiu Dumitriu
http://purl.org/net/sergiu/