Requiring pull request reviews

[dpryan79]

@bioconda/all When the Bioconda project was originally created it needed to be extremely simple for anyone and everyone to add new recipes and modify old ones, since there was no base of recipes to build upon. Today, Bioconda has 500+ members, approximately 1000 contributors, almost 7000 packages and has become a primary part of the global bioinformatics infrastructure. Given this, we need to ensure that new and modified recipes adhere to best practices such as those outlined at https://bioconda.github.io/contributor/guidelines.html. To do so, we would like to begin requiring pull request reviews for almost all bioconda members beginning June 1st. Any bioconda member (not just @bioconda/core!) can review and approve pull requests, so we expect the turn-around time on pull requests to continue to be low.

After June 1st, please use the “please review & merge” label for PRs once you’re satisfied with them and would like to request a review. It’s very easy to peruse such labeled PRs, so we hope this will be adopted by the community. You’re additionally welcome to post a link to your PR on Gitter or ping one of the expert teams or persons, both for getting help and for getting a review. We hope that this will also increase interaction between community members, improve exchange of knowledge and foster best-practices.

To review a pull request with the "please review & merge" label, check to make sure it satisfies the guidelines and kindly point out any improvements that could be made (and don't forget to adhere to our code of conduct). Feel free to pull in Bioconda's expert teams to take a look, or call in specific people you know could help. Once it looks good, simply mark the pull request as approved. After that, either you or the submitter can merge it (either directly, or by typing @BiocondaBot merge).

We welcome feedback on this proposal before June 1st, as we would like Bioconda to continue to be the vibrant and open community that it is today.

[apeltzer]

Makes a lot of sense - also, to get feedback for the one submitting PRs!

[holtgrewe]

Would it be possible to get an exception for trivial changes?

For example, I have taken quite some effort to make it simple/trivial to bump recipes such as jannovar-cli but putting the version and SHA256 as variables at the top.

I'd be very much in favour of getting a review of everything at least once but subsequent changes that only touch the variables at the top could be exempted IMHO.

[dpryan79]

@holtgrewe There's no way to do that on github, so we've discussed modifying @BiocondaBot to do that. The code to do so doesn't exist yet, so PRs there are welcome. Given of the number of people on this repository, though, it's already unlikely that the wait time will be more than an hour.

[epruesse]

@holtgrewe Our bot can scan for new upstream releases and autobump the recipe to a new version (beta feature). The PR opened comes from the bot, so you will be allowed to review and merge those.

[PertuyF]

Looks like a great idea! It is a step further building collective responsibility on our community.

Regarding simple version bump, having the Bot's autobump running would be a nice feature, but note that the test packages are directly available once test succeeds now.
So as long as it builds, no one should be blocked using a bioconda package, even if the review-merge takes a few hours to complete.

Also, don't you think using a maintainers list (as conda-forge does) would be nice here?
Maintainers would be members from the core team or previous contributors to a given package, with some knowledge of why this was done this way in some complex recipe.
And using a ping @bioconda/<recipe> from a PR would ask for review from this group.

Anyway, I'm in!

[nsoranzo]

2 comments:

• In this issue description above, can we clarify that also merging someone else's PR is equivalent to approving it?
• We can use a CODEOWNERS file to help people being notified on PRs touching packages they are interested in.

[apeltzer]

@holtgrewe Our bot can scan for new upstream releases and autobump the recipe to a new version (beta feature). The PR opened comes from the bot, so you will be allowed to review and merge those.

What would be nice in this case:

• that the PR opened is automatically assigned to the person listed as last author (?), so that person gets a notification
• maybe everyone else who worked on that recipe already (should be possible to fetch that throug the git history of that specific recipe?)

[dpryan79]

In this issue description above, can we clarify that also merging someone else's PR is equivalent to approving it?

Does that work? I assumed you still had to approve the PR first.

We can use a CODEOWNERS file to help people being notified on PRs touching packages they are interested in.

We discussed that possibility. It sounds nice in theory, but with so many packages we're worried that it will get unwieldy almost immediately.

[epruesse]

(Parts in bold for TL'DR)

• I would encourage people to add themselves to the extra: recipe-maintainers: list for recipes they have worked on. That way, if things need to be done to it, we can (automatically) ping / request review from people who are knowledgeable with that particular tool. Special idiosyncrasies should still be in comments (for maintainers) or the extra: notes: section (for users). Should we need more rules down the line, we then already know who's maintaining which recipe (the git history isn't that informative, often /core is on there due to bulk maintenance changes).

• The directly available packages are for testing only. The artifacts will disappear whenever CircleCI decides to. They are very useful for testing the built packages beyond what the automated tests can do, but not so useful as distribution mechanism.

• Building and updating the CODEOWNERS file from the maintainers list in each recipe would be simple. It would need to be updated automatically though, because we'd quickly have a few thousand lines in there. We can still do this at any time should it become necessary as long as we have the maintainers list. For now, I think one step at a time is best.

• Before merging, the PR would have to have one approving review. Approving and merging can be done by the same person, whether using the merge button or asking the bot to do fast upload & merge. So it's not equivalent, but it's two steps that can be done quickly.

• Github suggests reviewers. Not sure how they come up with the list, it appears based on previous editors of the file, perhaps recency, perhaps keywords in the PR. If that proves insufficient, having maintainers, we could have the bot a) list them, b) ping them or c) request reviews from them. I think the best might be just asking on Gitter for someone with a few minutes to look things over once a PR goes green (or before for help).

• I guess we could have @bioconda/<xyz>-maintainers as Conde-Forge does. That many items, whether repos or teams, makes some of the UI break, though. It might be easier to just have the bot ping on demand, or better, have issue templates with a place that the name of the relevant recipe goes and then ping maintainers directly.

• We (/core) were debating "senior member" or "reviewers" or "maintainers" as teams. Still might do it. For now, we tried to apply KISS. go in small steps, see where it leads and most importantly hear what you all thought.

[mbargull]

• I would encourage people to add themselves to the extra: recipe-maintainers: list for recipes they have worked on.

+1
It's been "a while" since the last time I had a look at our docs; if we don't already do, we could/should recommend extra/recipe-maintainers in there.
(It's also been ages I checked out bioconda-utils, so I'm not sure if this has been implemented in the meantime, but suggesting to add oneself to extra/recipe-maintainers could be one of those non-error lints, i.e., suggestions the bot could make.)

• Building and updating the CODEOWNERS file from the maintainers list in each recipe would be simple. It would need to be updated automatically though [...]

+1
I would expect having that automatically maintained CODEOWNERS file could give a nice GitHub UX.
But then again:

For now, I think one step at a time is best.

+2

• I guess we could have @bioconda/<xyz>-maintainers as Conde-Forge does. That many items, whether repos or teams, makes some of the UI break, though. It might be easier to just have the bot ping on demand, or better, have issue templates with a place that the name of the relevant recipe goes and then ping maintainers directly.

There can be some (small) value in having @bioconda/<xyz>-maintainers teams (n.b.: I'd prefer @bioconda/maintainers-of-<xyz> / @bioconda/MNT-<xyz> to reduce clutter in team lists) -- however, if (at some point in the future) the bot uses the extras/recipe-maintainers key (and possibly the CODEOWNERS file), I agree that something like BiocondaBot ping maintainers is less maintenance-intensive than having to add/remove teams/team members.

For now, we tried to apply KISS. go in small steps, see where it leads and most importantly hear what you all thought.

+10

---

N.B.: I'm glad to see the positive reactions and am thankful for the constructive comments!

[daler]

When merging the autobumped recipes, should this also require two sets of humans to approve?

That is, given this "four eyes required" merge policy, how many eyes does the bot have?

[epruesse]

Perhaps we should differentiate between new recipes or updates that significantly change a recipe (e.g. rewriting the build.sh, changing dependencies, adding outputs, etc) and minor changes such as a minor or patchlevel upstream bump in which only the sha and version change?

[dpryan79]

@epruesse Long-term that would be good. Of course there's no way to do that on github, so we'd have to have the bot autoreview PRs.

[dpryan79]

This policy change will go live shortly.