Assigning committers to pull requests

Development
#1

Hi everyone,

given that we have currently 53 open pull requests, some of them being quite old, shows that we aren’t doing a good job at handling contributions by non-committers (some are also by committers and on hold for various reasons, but this definitely doesn’t apply to all of them). As this can deter new contributors and doesn’t make contributing to XWiki a nice experience, I would suggest to create an effort to change that. At the moment, all committers are responsible for all pull requests, and I think this creates part of the problem as nobody feels truly responsible for checking that a pull request is actually either merged or closed at some point.

To change this, I suggest that we start making use of the “Assignee”-feature in GitHub. I suggest that we aim to achieve the following:

  • When a pull request is opened, at least one committer assigns him/herself to the pull request.
  • It is then the responsibility of the assignee to
    • Either directly review the pull request or to ask other committers for feedback
    • Merge the pull request when all relevant feedback has been addressed
    • Apply the changes to stable branches where appropriate
    • Close the Jira issue and make sure that the documentation is updated when necessary
    • Handle any breakages that are caused by the pull request, if possible in coordination with the contributor
    • Take over the work to complete the pull request when the contributor is unable to address the feedback or doesn’t respond anymore at some point if this seems reasonable, e.g., when the work already done by the contributor provides a value to XWiki and it wouldn’t be faster to just start from scratch.
    • Close the pull request (without merging) when this seems reasonable
  • To ensure that contributions are accepted in a timely manner, we commit on a best-effort basis to assign a committer within two working days and merge contributions for the next RC release if they were submitted at least one week before it and are in a reasonably good state.

Due to the time requirements for this, the expectation would be that primarily committers who are working full-time on XWiki take the responsibility of assigning themselves to pull requests. By default, committers would assign themselves to their own pull requests, but I would suggest we explicitly offer that another committer takes the assignee role for pull requests created by committers who aren’t working full-time on XWiki or who are new. The expectation would be that while the committer who created the pull request does most of the work, the assignee would help with time-critical tasks when the author of the pull request is unavailable and the assignee could also help with following all best practices that might not be that well-known to a committer who contributes only occasionally.

To make sure we also handle the existing pull requests, I suggest that every committer who is working on XWiki as part of the monthly roadmaps picks an open pull request every week, assigns himself and tries to handle it as part of the bug fixing days, with the goal of either merging or closing it until the next bug fixing day.

If we agree on this proposal, I suggest to document these rules on the committership document, basically as a more detailed version of

  • Committers must do a best effort of accepting Pull Requests (PRs) from contributors, reviewing them and when they are satisfied the quality and relevance are good enough, applying them.

Any opinions on these ideas?

#2

+1 for the general idea.

Regarding the way to assign tasks to committers, I’d like to propose the use of the auto-assignment of reviewers provided by GitHub.

Some of the benefit:

  • This would distribute the load evenly
  • Committers are encouraged to take this as an opportunity to grow on new topics if they assigned to something new (and can always call others committers for advice)
  • It is still possible trade assignments with other committers (or to mark yourself as busy). If somebody is always delegating, he will be auto-assigned to new PRs anyway as he would have a low load

The main constraint I can think of is how to handle legacy PRs. But for those, I guess we can always do a manual round-robin assignments of old PRs.

#3

Disclaimer: I’m replying quickly with the first thing that comes off the top of my head. I’ll need a few days/weeks to let this sink in.

I think the intent is good but I have a small difference in POV. I don’t think all contributions are equal and I don’t think that from the POV of the XWiki project, the time of committers (knowledgeable in XWiki) has the same value as that of external contributors (less knowledgeable).

Everyone’s time is limited and the ones who have the less time are actually the full-time committers since they’re busy all the time working on various thing in XWiki (and only they can do complex stuff). If they have to spend, say 2 hours to reply to PRs and educate a contributor, then it’s 2 hours not spent improving XWiki. I also think that good contributions (good PRs) are well done and easy to apply. If you have to spend the same amount of time to fix the PR than if you were coding it yourself, then something is wrong and you’re just swapping your own roadmap vs that of the contributor.

So I wouldn’t push at all cost to replace committer’s own times to work on other people’s needs. That would just be bad for the project globally. And the idea that they will contribute more is far from guaranteed (and only happens infrequently IMO).

So for me, it’s more about making sure that we don’t miss the good PRs, that we reply in a timely manner to open PRs if we can (same as we do about jiras or forum posts) but not that we spend more time on them.

That sounds good but the devil is in the “best effort” part :wink: Your proposal is more than a best effort to me.

Let’s see what others think.

#4

BTW and FYI I talked to Freddy Mallet yesterday about ReviewPad, a project he’s working on to help make sense/apply PRs. He told me that the product should be in a good shape in about 2-3 months for us to try it if we want. He’ll ping me. Something to explore, to see if it could help us or not.

#5

My goal here is not that the assignee spends a lot of time to educate new contributors. An assignee can also just close the PR with a link to whatever best practices the contributor violated or with a comment that we’re not interested in that feature and that it should be implemented as an extension instead. Also, the assignee doesn’t need to review the PR. For me, the job of the assignee is to make sure that something happens with the PR and it doesn’t just stay open with or without feedback.

We can also make it easier to review pull requests by automating as many quality checks as possible and executing them on the code in the pull requests such that a committer can immediately see if there are serious quality issues.

Also, I wouldn’t see time spent on educating contributors as lost time. If the contributor comes back with another, better pull request after being educated this might make up for the time “lost” educating the contributor the first time.

#6

Sure, that’s the theory but it rarely works. If you check any open source project, you’ll see that only a very very small percentage of people do 90%+ of the work. Most people don’t have the time/wish/knowledge to do more. And for the rare people who can really contribute significantly, you can be sure that they’ll be recognized easily and that their PRs will show that (and that we will fight for applying/replying to these PRs ;)).

In any case, I’m not saying that we shouldn’t help contributions. We already do. What I’m questioning is the systematic aspect of your proposal. It’s the same for the forum; I wouldn’t want a systematic mechanism that forces us to reply to all posts. I’m very fine with all proposals on this topic that help us accept PRs faster/better but less so for proposals that suggest committers should swap significantly more of their current work time in favor of PR reviews, as I believe we’re already having a hard time working on important aspects of XWiki (such as security fixes, bug fixes, automated tests, fixing flickers, etc).

+1 to that, anything that can be automated and take less committer time will help.

#7

I agree with Vincent. On my side it won’t be easy to apply what you propose (I already find it hard to follow all the discussions on the forum, charts, JIRA, etc.). But since you mentioned BFD, maybe we can replace / complement one BFD per month with a PR day (to review old pending PRs).

Thanks,
Marius

#8

Okay, I understand your concerns. What do you think about dropping the promises regarding time and also the part about taking over the work to complete it and we drop the idea that this would be “primarily committers who are working full-time on XWiki”?

For me, the main idea of assigning committers to pull requests is to have somebody who takes care of the pull request and, e.g., occasionally checks its status and ensures that there is progress in some way and ensures that after the pull request has been accepted, it is actually merged, the Jira issue is closed, documentation for release notes is created etc… These are all things that contributors cannot do due to missing permissions. When I wasn’t a committer, I had to frequently ping people on the chat to make these things happen as we have simply no process for it and the “all committers should do it” frequently doesn’t work as nobody feels responsible. Constantly pinging everybody on the chat is also not nice and if I hadn’t been told to do it I wouldn’t have done it.

At last with my original idea, there was no part of forcing any committer to be an assignee for any pull request. This would be on a voluntary basis and committers could pick the pull request they’re most interested in. Note that being a committer already means that you agreed to doing all this work, the assignee role would just mean that a certain committer agrees to manage a certain pull request similar to how committers agree to handle a certain bug or a certain release.

Alternatively, we could also establish a team of committers that handles these tasks similar to how we have committers who manage security or dependency upgrades and then we would assign committers from this team round-robin to pull requests. Other committers could still handle pull requests, committers from this team could also assign pull requests to other committers if it makes sense, and in particular reviews would still be expected of all committers as the role of the assignee is not necessarily to also review the code.

That sounds like a good plan.