Pull requests becoming stale

[bobvanderlinden]

Issue description

Pull requests seem to become stale at a time when they are ready to merge. I see this happening in a number of highly upvoted open pull requests. This issue isn't so much for these PRs specifically, but rather the problem in general: how can we avoid PRs from becoming stale.

Should something like probot-stale be added to Nixpkgs, so that we can get reminder-comments when a PR is becoming stale?

[bobvanderlinden]

Just to illustrate why I created this issue: when I sort the PRs by number of 👍's I'm seeing a lot of PRs that seem to be ready for quite some time, but are not being merged. It is likely they still need some changes, but they aren't getting feedback:

• Add pkgs.nixos function #32422 (:tada:)
• nixos: set default for boot.tmpOnTmpfs to true #17494
• display-managers: make lightdm the default #30890 (:tada:)
• fetchurl: add user agent #17757 (:tada:)

There are also some that are so old and large that it would be quite a bit of effort to get it mergable again:

• Reworking test infrastructure #18142
• Shipping Security Updates #10851

[KiaraGrouwstra]

I wonder what the bottleneck is here:

• if core maintainers no longer have sufficient time, could we authorize other contributors to help out on this?
  • who can merge PRs now? - NixOS members
  • what existing contributors would make good candidates? - contributors by commits
• if only a small core team has the Nix/NixOS know-how to handle PRs, would it be possible to train other contributors to this end?
  • what knowledge is essential here?
    • is this documented to give prospective maintainers a good starting point?

[7c6f434c]

There is around a hundred people with commit access. People with a convincing track record (tens of accepted PRs, recent PRs being technically fine from the beginning, not completely inactive recently) get commit access if they ask — finding and informing such people that they could just ask might be a useful community service.

Unfortunately, there are PRs that get missed by mistake — more eyes would help here — and there are PRs that are missed because people are unsure if there is a «wrong approach» flamewar waiting to happen and do not want to be in the middle of it. Reading lightdm PR, for example, I am not completely sure if it is of the second kind, and I guess nobody is…

[KiaraGrouwstra]

@7c6f434c:

People with a convincing track record [...] get commit access if they ask — finding and informing such people that they could just ask might be a useful community service.

If we notice someone has a great track record, might it help to mark them as contributors without waiting for them to ask? This might lower the barrier to entry, and hopefully get this group more excited about contributing further.

[7c6f434c]

I think people who can give commit access would generally prefer not to monitor the statistics on their own (looking up track records of people who ask can be done with a search per person). And I think when I compiled a list of recommendations, there were no objections to giving access to everyone on the list who asks, but there were some reservations about giving out access without asking.

[matthewbauer]

Just merged #32422. That one is pretty useful & harmless to those not using it.

All of the others are mostly questions of what defaults should be. These are harder to get consensus on even if they are probably good changes. More discussion is needed!

[oxij]

Reminder bot looks ok to me. I'd also like to have the mention bot back, I used to use it mentions as a criteria in filtering and it worked pretty well, now I have to use keywords instead. Why did it get nuked? OfBorg could also just schedule PRs for automatic merging if they have some 👍/LGTMs and no 👎/Objections from trusted users. E.g.: 1) OP> Here's a PR. OfBorg> Blame information suggests @TrustedUser and @AnotherTrustedUser as reviewers. TrustedUser> LGTM. AnotherTrustedUser> LGTM. OfBorg> All checks pass, got 2 positive reviews. Scheduled for merge on YYYY-MM-DD unless someone objects. [YYYY-MM-DD 00:00] OfBorg> Merging today at 23:59 unless someone objects. [YYYY-MM-DD 23:59] OfBorg> Merged. 2) OP> Here's a PR. OfBorg> Blame information suggests @TrustedUser and @AnotherTrustedUser as reviewers. TrustedUser> LGTM. AnotherTrustedUser> LGTM. OfBorg> All checks pass, got 2 positive reviews. Scheduled for merge on YYYY-MM-DD unless someone objects. YetAnotherTrustedUser> Objection! OfBorg> Merge unscheduled. This PR is stalled. ... some discussion ... ... some changes ... [week after last update] OfBorg> This PR now conflicts with PR #XXX recently merged to master, please rebase onto REVISION to continue. ... rebase ... ... some more discussion ... [week after last update] OfBorg> All checks pass, got 2 old positive reviews, 1 old negative review. This PR is stalled. TrustedUser> Still LGTM. AnotherTrustedUser> Still LGTM. OfBorg> All checks pass, got 2 positive reviews, 1 old negative review. This PR is stalled. YetAnotherTrustedUser> Well, fine, LGTM, I guess. OfBorg> All checks pass, got 3 positive reviews. Scheduled for merge on YYYY-MM-DD unless someone objects. [YYYY-MM-DD 00:00] OfBorg> Merging today at 23:59 unless someone objects. [YYYY-MM-DD 23:59] OfBorg> Merged. This will create pressure to review things.

[KiaraGrouwstra]

@oxij those sound like really good ideas!

@7c6f434c:

people who can give commit access would generally prefer not to monitor the statistics on their own

That's fair, yeah, this does sound like extra burden.
My experience here is mostly from smaller personal projects, but for those when accepting a PR from someone I remembered to have made several good contributions before, I'd just mark them as contributor -- so here I wouldn't be actively monitoring fwiw. But I can't speak for bigger projects like this, admittedly.

[7c6f434c]

And of course the people who do have access to giving out commit bits don't often have time to review PRs, either…

[teto]

If we could give per folder/files commit rights, then it would be easier to give those rights. Like I wouldn't mind asking commit rights for neovim (and some other packages I use, ns-3, cmd2, protocol) but asking them for the whole nixos repositoy is intimidating.

[matthewbauer]

Maybe we need to move back to SVN?

[bobvanderlinden]

Since creating this issue yesterday already 2 of the mentioned PRs were merged! 🎉 😄 (thanks @matthewbauer)

I think the maintainership and rights is a related, but separate issue. Contributors can just as easily approve PRs and if there are enough approvals it's easier for member to merge those PRs.

However the problem is also partly that usually PRs are only approved/reviewed by one person (or at least one is giving their approval). This might be solved by automatically adding contributors as reviewers as well, so that reviews become part of their 'flow'.

I really like the proposal of @oxij. If there are enough people thinking this is a good idea, what would be needed to implement this? I'm quite unaware how OfBorg is currently working (is it a bot based on an existing one? Where can I find its code?)

[7c6f434c]

I really like the proposal of @oxij. If there are enough people thinking this is a good idea, what would be needed to implement this?

There is a general idea that ofborg development should be careful and conservative. There is a plan to add conditional-merge, delayed-merge and remind-me commands, but it is work and testing. Obviously these have to work well before even considering what amount of approvals without explicit merge command should mean what and who is trusted more or less.

I'm quite unaware how OfBorg is currently working (is it a bot based on an existing one? Where can I find its code?)

How to find: click the nickname, got to the profile, look at the homepage, it is a repo. Where to find: this will lead you to https://github.com/NixOS/ofBorg/ There is a README.md, and development is indeed done via PRs and issues.

[bobvanderlinden]

Ah, interesting. I was thinking that repo was used for the builder of OfBorg, but it's also the bot itself.

I think the following issues are very much related:

• Ping meta.maintainers
• Automatic Merging Implementation
• Feature Request: Automatic merging

Should another issues be created to notfy that a PR is becoming stale? The exact details are yet to be defined, but I can imagine something that a reminder for people of a PR being inactive for some time could bump the activity again.

[7c6f434c]

I think there is also an issue about periodic re-evaluation? It is probably also relevant.

I would say that unless you have a specific design that can be discussed, there are enough prerequisites well-described in issues that creating further issues can wait.

[FRidh]

Earlier there was an experiment with nixbot which merges when asked by maintainers of expressions. With some small improvements, mainly getting rid of maintainer-paths.json, I think that would be a major step forward. We should also include at that point an additional meta attribute from blacklisting certain derivations to be merged that way though.

[bjornfor]

If we could give per folder/files commit rights, then it would be easier to give those rights.

Maybe we need to move back to SVN?

Git with gitolite supports per folder/file permissions. (When nixpkgs moved from svn to git I was kind of hoping we'd use gitolite.)

[doronbehar]

@bjornfor Nixpkgs uses now the codeowners feature. See https://github.com/NixOS/nixpkgs/blob/a61bd1e42d5512f0e5180edf7ea6090afcd9544b/.github/CODEOWNERS

[stale]

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

1. Search for maintainers and people that previously touched the
   related code and @ mention them in a comment.
2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
   irc.freenode.net.

[jtojnar]

LOL.

[ryantm]

RFC51 added a stale bot, which I think addresses the main actionable issue in the original post I don't think it gets us much to leave this open. If people want to discuss general improvement ideas they can use Discourse, and if they want to discussion specific actionable issues, they can open a new issue.

I'm going to close this based on that, but please let me know if you object and we can reopen.