Reviewer of Day - Reloaded


#1

Let’s bring back Reviewers of Day!

Reviewer of Day - person who has the daily duty to review unmerged PRs and help getting them moved forward. The main goal this is trying to achieve is to reduce the time between PR creation and merging and overall improve our efficiency and satisfaction, by attempting to identify and address the bottlenecks and blind alleys in the process.

We had this experiment when we introduced RODs a while ago. The immediate reason was to improve the PR lifecycle, reduce the unmerged backlog, regressions and the dreaded rebasing with conflicts. The experiment was largely successful, but at some point we dropped the idea - I think largely because a lot of the routine ROD-ing tasks were taken over by various github bots.

Right now we’re in a much better shape with respect to PR flow, but still at the time of this writing there are 31 unmerged PRs and PR review ceremony is cited as one of the top reasons causing discomfort to the contributors.

So, I say let’s bring the RODs back then and try to unclog things again. Off the top of my head, listing some of the tasks we could do daily to improve the PR flow:

  1. Assign reviewers to PRs without any assigned
  2. If familiar with the code, review the PR yourself (really review, don’t just “green” it)
  3. Take a look at the stale PRs. Is there any chance they might still be merged? If yes, try pinging whoever can make it happen. If not, just close it off - politely, but confidently.
  4. Take a note whenever you notice that something can be improved in the PR flow - e.g. can anything else be automated with a bot, can we enforce the squashing rule automatically, can we add meaningful PR checklists?
  5. PR flow health greatly depends on automated testing - help with e2e test blockers, either by fixing them yourself or by pinging whoever can
  6. etc…

I’ll start by volunteering for status-react on say Mondays. Anyone wants to help me fill this plan fully?

Monday - @goranjovic
Tuesday - @vitaliy
Wednesday - @cammellos
Thursday -
Friday - @andrey

Enlist now - you know you want to! :slight_smile:


#2

Friday? works for me


#3

How about making sure PR creators own their PR lifecycle?

It should be everyone responsibility to make sure a PR is merged. This is when your job is done.
Some of the items you mention falls down to PR ownership.


#4

^ Looks like you already started with this one. :slight_smile:

Sure, that would move us long way. Even if we need to handle bounty PRs differently.


#5

While this might be true in theory, and we can certainly encourage it more, evidently this doesnt always work in practice. We also have non core contributors. That’s where ROD comes in and work as a safeguard to make sure PRs are merged quickly and smoothly, or closed, as opposed to staying open for a long time.

Kudos to Goran for pushing this!


#6

The last time we did ROD-ing it was a mostly janitorial process consisting of tasks that were so routine that they are now being done by a bot.

While there will plenty of janitorial stuff this time too, I think we should focus more on the “what can we do to make this better” part.

E.g. your observation @julien that we lack PR ownership and that we’d fix a lot of things if we increase that somehow is really a good example of this. Another thing I’ve noticed is that bounty PRs tend to take much longer to get merged, simply because there is no one within the organization “pushing” for it. One immediate fix for that issue could be to assign a core developer to every “external” PR to follow through.


#7

I do feel that the best way is to eventually get rid of this, but it’s clear we are not quite there and it’s not clear how to make it work without a rota, so I can take Wednesdays or Thursdays :slight_smile:


#8

Should be rod’s goal to get rid of rod then! :smiley:


#9

I was going to say that! :slight_smile:


#10

I think ROD should really take care only of non-core PRs since as @julien said the core PRs are the responsibility of those who make them.


#11

And for non-core PR why not have a bot assign them to core members on a rotating basis?


#12

Bots can’t anticipate every scenario. If things fall between cracks, it has to be us who’ll handle that, there is no algorithm to help there.

IMO, this reboot of RODs is really just a periodic review of the PR flow itself. That said, sounds like you’re interested @yenda? There’s still a nice free slot at Thursdays, whaddaya say? :slight_smile:


#13

Did something happen with this? I notice we have a bunch of PRs that have been open for over a month https://github.com/status-im/status-react/pulls?page=2&q=is%3Apr+is%3Aopen - maybe it’d be good to clear out the backlog and close old PRs?

ROD/(Maintainer?) functionality will also be invaluable when it comes to dealing with other contributors as well, to make sure we give them their proper attention, without having a single person being a bottleneck in the process.


#14

Yeah, we already cleaned up obviously dead PRs in the first sweep (i.e. unsalvageables). The ones you linked are up next.


#15

Would be great with mini updated in this thread or so about any lessons learned doing this. The most important is probably consistency and making sure there’s closure/follow up on old PRs from everyone who volunteers as a ROD. It’s a much appreciated form of maintainership!