Reviewing and merging PR/MRs

One of the most important things with any projects is PR/MRs. This discussion has been created to help us members of the SIG Core team to triage PR/MRs and get them reviewed and merged as quickly as possible.

One thing I would like is to not allow members to self merge their own PR/MRs. Another thing I wish to address is how many approvals do we need before a merge. And better yet who amongst us is allowed to merge PR/MRs.

9 Likes

What is your stance on self-merging critical PRs with a set number of reviews? E.g. if a critical change has not yet received attention from more than one maintainer, but has built consensus towards the positive impact of said package, should that be held back until another maintainer is available?

This is a very valid question, and I think if the contributor is trustworthy enough then it is seems fine to allow a critical PR be merged. Otherwise, I would still prefer that other maintainers are around to give their two cents. But then this also becomes a question of what you would consider a critical change.

I think it’s much more feasible to actively discourage (not ban) self-merges, instead note down that exceptions may apply on a case-by-case basis. Obviously we should avoid “QUICK MERGE THIS BEFORE IT BITROTS!” kinds of approaches, but no guideline should prosecute a maintainer for attempting to respond quickly.

2 Likes

I feel like this is a much more realistic implementation of the guidelines and I’m happy to roll with this as a set of rules.

1 Like

Right now, since SIGs are on the small-ish (ie. single digit members) size, I think 1 approval from anyone other than the PR creator is good enough for now - any more and we’d be requiring a sizable chunk of the team to give the OK on something, which could slow things down at the start. We should definitely revisit it later.

As for commit access - I think we should probably start thinking about some sort of structure within the SIG, since I think it makes sense for a subset of “SIG owners” to have commit access. Don’t have any particular ideas on how we should decide what that looks like, open to suggestions. Again, ties into a wider discussion into how the SIG is structured - maybe better left to another thread?

2 Likes

So, just so I’m following along.
If there’s a PR that’s been approved but not merged, would it be alright if I went ahead and merged it?

For example, this one: feat(system): change nixpkgs rev to nixos-unstable by aemogie · Pull Request #10 · auxolotl/templates · GitHub

1 Like

If you are on that SIG and the PR is necessary for the project (and the author wants it merged), then it is absolutely your prerogative to merge!

3 Likes

For example, if I am on the Docs SIG and there is a completed PR that improves the project. I should be able to merge it if there are no issues left.

4 Likes

Alrighty, I think it got it now :smile:
Merging #10 on templates now.

2 Likes

@axel this is a bit of a nitpick but I would prefer if we approved PRs before merging

re: fix(darwin): config argument missing on system.nix by licebmi · Pull Request #12 · auxolotl/templates · GitHub

1 Like

Understood :+1:t4:
I’ll make sure I approve stuff before merging.

1 Like

Just to add to this, please let me know if I make any more faux pas.
I’m not used to working on this side of an open source project, so I’d really appreciate the input. :smile:

2 Likes

(I’m not 100% sure if the following fits here; please feel free to move this to a different topic if it doesn’t)

On the topic of reviewing: To me, reviewing nixpkgs PRs always felt very cumbersome, because the review guidelines focus only on the high-level aspects. For me, this created the problem that many remarks on PRs regarding idiomatic nix(pkgs) code felt arbitrary.

In turn, this lead to me giving more or less up on reviewing PRs - since I wasn’t even able to predict which code would be considered idiomatic for my own PRs.

I’m not sure if this is simply a ‘me problem’, or if it also effects other people. But I also have no idea how to solve it, lol.

(I probably don’t need to say this anyway, but this is not supposed to dunk on the nixpkgs review process at all. It’s just something that made reviewing nixpkgs PRs a lot more difficult for me).

2 Likes

I completely agree. This is also one of the reasons I have not contributed much to NixPkgs directly. For a lot of these things I think some of my favorite advice from an ex-coworker of mine applies:

If you like it, put a lint rule on it

A large amount of the nits can be taken care of via automation (formatter, linter, tests). We should not have to waste effort on these things.

5 Likes

In one company I worked there was a flag to force the commit without approval for critical situations, but unless an approval was given within a timeframe it was automatically rolled back.

The org was designed around CODEOWNERS being responsible for certain subfolders of the monorepo and IIRC you could only force into a section that you were listed as codeowner.

With the forced commit being a special flag it was easy to do any form of follow-ups incl. understanding who might abuse the system.

I’m a huge fan of defining exceptions for the standard processes, it helps thinking through the process and recognizing when/ where and why a different solution than the standard is helpful. Instead of squeezing every possible situation into a complicated ruleset.

3 Likes

We should allow for docs team to self merge – there is often not a need for review, considering the most breaking change is just confusion. We should also automate this, by only alliowing changes to .md files or something.

1 Like

It never hurts to have another set of eyes on things, if only for a spellcheck.

3 Likes

Funny you should mention this, that’s actually part of the PR I just submitted! And I think that that’s kind of why we shouldn’t have checks be necessary. The URL in the templates readme was broken, and that’s something that I should be able to fix without needing to wait for anyone. I think that people in the SIGs should have that power and decide themselves if this is something that’s worth review or not. (at least for now, we could revise this once the community grows, but I want to see stuff happen fast for now)

2 Likes

I agree that self merging would help expedite stuff a lot right off the bat, but we would have to be sure to start requiring reviews again at some point down the line when the docs and our process is a bit more stable to ensure a high standard of quality.

2 Likes