Currently, the way we request for reviews of pull requests (PRs) is loosely defined and is a combination of:
Mention on Element
Mention on GitHub
Hoping for someone to start reviewing by their own will
GitHub review requests mechanism
This makes it hard to answer a newcomer asking how to make sure their PRs are reviewed.
I also feel like having a shared way to work helps have confidence in the process.
The assignee is the one making sure the PR is merged. They are also responsible for making sure the right reviewers are requested.
A review request expects an answer. A requested reviewer needs to either review, say when they will, or remove themselves and suggest someone else. A review request never blocks the merge.
Re-request when ready again. After addressing feedback, the author uses GitHub’s “re-request review” button, so the PR shows up again in the reviewers’ review-requested:@me list.
Committers PRs PR created by a committer, or created automatically and assigned a committer (e.g., renovate) don’t have to get reviewers
xwiki-contrib. As usual, the same rule applies for contrib projects
Finding reviewers In case of inactivity, or when it is unclear who is relevant, it is encouraged to ask on the #xwiki chat.
Options
A. How reviewers get requested
A1 Manually, by the author or the assignee. (my preference)
A2 A1 + automatic request to a random committer (feel like noise)
A3 Using CODEOWNERS. Mentioning it for exhaustiveness but feels too advanced for our needs
B. Response time
B1 No target. But, committers are expected to know the projects goals, and take them in account (e.g., PRs important for the next release take priority) (my preference)
B2 A best-effort target of one week for the first answer to a review request (not for the full review).
C. Reminders
C1 None; everyone uses the review-requested:@me filter. (my preference)
C2 A weekly digest of PRs with no reviewer or with unanswered requests (channel to be decided).
Documentation
If accepted, I’ll document the result in a new “Requesting a review” subsection of the Development Practices, add a line about reviewers to the PR template, and update the Contributing page so that external contributors know what to expect (including: if nobody is assigned after a while, ask on the forum or on Matrix).
1-6 sounds good to me. However since we’re here I see 2 things that we should also document in relation:
who is responsible for handling the ticket when the merge is performed
who is responsible of the backports
Also you didn’t mention in your proposal how to handle PRs with missing assignee
Ok for me
Honestly it’s not always easy to know the priority and I think it will not become easier. We could decide to use labels provided by authors here to help prioritizing. I guess we’d need feedback for contributors who aren’t committers for this.
Could be useful also for PRs with missing assignees.
Indeed, I left that out considering the previous discussion formalizing the use of assignees was covering that. But, reading the rule and the previous discussion, I confirm that status is not so clear.
So let go for option D
D1 Anyone starting work that is expected to result in a PR must actively look for an assignee before starting the work (this means find a committer that would support the work, discuss the design beforehand and so on, which imo increases the chances of getting the PR merged)
D2 On PR creation, devs are asked to assign any committers they think fit (or randomly as a fallback), assigned committers have to find a replacement if they don’t want to do the assignee’s duty
+1 for D1 as it seems more efficient
I agree, but setting a fixed delay to react wouldn’t help either. If a review is lagging behind, they are expected to escalate the issue if they are not able to deal with it (e.g., the reviewing load is threatening their own roadmap).
Could be an interesting follow-up, but I’m afraid discussing the details of this option will make the discussion harder to follow.
I feel this proposal is focusing quite a lot of active contributors who know the team reasonably well (so my answer below only apply to them). But I don’t think it’s really realistic to expect all this from occasional contributors creating PRs for some small things they noticed.
While generally clear who is an active committer for xwiki-commons/rendering/platform, it’s a lot fuzzier for contrib projects (not even mentioning pretty much abandoned projects) so it’s not that obvious to me.
+1
+1
Like @surli, I feel this might still be needed (especially to highlight PRs without any reviewer set). We can still decide later to drop it when we are more used to the process.
I looked at what we actually do before voting. These are the last 12 months of xwiki-platform PRs, excluding bots. “Committers” means the members of the core-committers team.
Authors
PRs
With an assignee
With a reviewer requested or a review given
Median time to first committer reaction
Waited > 7 days / > 30 days
Median time to merge
Committers
598
86%
30%
–
–
0.1 days
Org members who aren’t committers
160
74%
84%
3.2 days
47 / 22
5.8 days
External contributors
21
38%
43%
0.9 days
5 / 3
5.8 days
Commons and rendering have very few non-committer PRs (12 in total), so this is mostly about platform.
What I take from it:
The process in the proposal is mostly what we already do: non-committers already set assignees and request reviewers on most of their PRs. Writing it down is useful, especially for newcomers.
The real problem is response time, not process: about 30% of non-committer PRs waited more than a week for any committer reaction. Pending review requests are also concentrated on a few committers, so spreading them more widely (A1) matters.
External contributors are few and usually get a quick answer.
So:
+1 for 1-6
+1 for A1
+1 for B2 (best-effort one week for a first answer, which can just be “I’ll review it on X”). The numbers show a target makes sense, and it costs nothing.
+1 for C1. We can reconsider a digest if B2 doesn’t improve the numbers.
-0 for D1: assignees are already set on most PRs without it, and it’s too heavy for small contributions.