search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.35k stars 13.59k forks source link

Pull requests becoming stale #41793

Closed bobvanderlinden closed 4 years ago

bobvanderlinden commented 6 years ago

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 commented 6 years ago

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:

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

KiaraGrouwstra commented 6 years ago

I wonder what the bottleneck is here:

7c6f434c commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 :+1:/LGTMs and no :-1:/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 commented 6 years ago

@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 commented 6 years ago

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

teto commented 6 years ago

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 commented 6 years ago

Maybe we need to move back to SVN?

bobvanderlinden commented 6 years ago

Since creating this issue yesterday already 2 of the mentioned PRs were merged! :tada: :smile: (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 commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 4 years ago

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

stale[bot] commented 4 years ago

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 commented 4 years ago

LOL.

ryantm commented 4 years ago

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.