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.
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.
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.
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?
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
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!
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.
Alrighty, I think it got it now 
Merging #10 on templates now.
Understood 
Iāll make sure I approve stuff before merging.
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.
(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).
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.
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.
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.
It never hurts to have another set of eyes on things, if only for a spellcheck.
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)
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.