Monero: Streamline pull request review process

Created on 23 Sep 2018  路  9Comments  路  Source: monero-project/monero

A bunch of pull requests were merged by @fluffypony which did not receive a single mark of approval (except of fluffy's). This "review process" needs to be improved.
Monero is a community project and it is unacceptable to merge obviously broken changes such as this one: https://github.com/monero-project/monero/pull/4401/files#diff-04c6e90faac2675aa89e2176d2eec7d8R119

Most helpful comment

@leonklingele , yes make the bot! just do it. don't ask permission, seek approval, confer with the spirits, throw the I ching... just do it.

unofficial monero dev slogan, hacked from nike: just code it

All 9 comments

Review it then ?

The PR I've linked to was open for a mere 3 days. It wasn't critical to merge that early.
Why not add a bot which pings various GitHub accounts if a pull request didn't receive any reviews within 7 days or so. Something similar to this maybe? I can help in creating a custom one too.

Part of it is also a CI problem. Build Bot is red far too often. This makes it hard for developers to submit pr's that compile on all platforms, do not contain conflict markers (can be avoided with a simple linter) and do not break existing tests, but also for reviewers, because they need to go through half baked pr's .

What's wrong with the PR you linked? I don't see anything "obviously broken" with it.

@plavirudar conflict markers were left inside the commit for example.

The reason it was merged early is that we need to release asap now. Otherwise PRs stay open for ~10 days at least. But even then, some PRs stay unreviewed for weeks. The people who review more than the occasional PR are myself, stoffu, and sometimes vtnerd. It will only become better if more people (with the skills to do do obviously) start reviewing. You don't even have to review everything, just PRs that didn't get reviewed yet. It adds up.

@leonklingele , yes make the bot! just do it. don't ask permission, seek approval, confer with the spirits, throw the I ching... just do it.

unofficial monero dev slogan, hacked from nike: just code it

I suggest that labels are added to the pull requests. I could then easily sort through them and look for changes to the build system for example. This is something maintainers would have to do though.

While examining various GitHub bots, I found the following which might be of interest:

  1. https://github.com/probot/stale Auto-replies to Pull Requests after a period of inactivity
  2. https://github.com/facebookarchive/mention-bot @-mentions potential reviewers (based on their previous contributions and reviews)
  3. https://probot.github.io/apps/minimum-reviews/ Prevents merging of unreviewed Pull Requests

Who can set them up?

Was this page helpful?
0 / 5 - 0 ratings