Do we have a reviewers template for package updates?

Defining a set of things to do and what to look for can make it even clearer for someone to just jump in and start reviewing.

We will also get across what kind of quality we expect from reviewers.

I think «test something, report what you tested» might be a good thing to try and see. We have previously seen questions whether all of the checklist items in the PR are mandatory (not sure if we have seen any PR with all of the items from the template checked!). I think once we have enouogh non-committer reviews to want trading quantity for consistency, we can try improving recommendations; I believe this moment is not yet now.

I think «test something, report what you tested» might be a good thing to try and see. We have previously seen questions whether all of the checklist items in the PR are mandatory (not sure if we have seen any PR with all of the items from the template checked!). I think once we have enouogh non-committer reviews to want trading quantity for consistency, we can try improving recommendations; I believe this moment is not yet now.

I think we should encourage at least a baseline of "I've done x things to test and because those were successful this can be merged" and that they've read what changes happened to the software and deduced that there's no breakages.
Without at least that it's very likely, for me personally, to ignore the review because I don't know what they've done to come to a conclusion.

And if they don't have access to the builder pasting the build output showing the success.

Argh, GitHub UI is horrible as usual, it doesn't show the actual difference (I added «what you tested»→«what and how you tested»).

For me «what you tested» sounds like «what functionality you tested», but after @worldofpeace comment I understood this can honestly be perceived as «what package you tested» which is indeed of little use.

Pastebinning build output might be a good idea to recommend.

Summarising changelog is something where trusting qualification is indeed needed; maybe we could ask for highlighting any parts of changelog that the commenter finds particularly relevant?

Pastebinning build output might be a good idea to recommend.

I've noticed pr reviewers who do this already in nixpkgs (non commiters) see a quick merge.
They present verifiable proofs to the commiter so that they can personally deduce a change as mergeable.

Summarising changelog is something where trusting qualification is indeed needed; maybe we could ask for highlighting any parts of changelog that the commenter finds particularly relevant?

Yep, the reviewer should read the changelog/release notes and identify items of importance.

I feel like if we could link to https://nixos.org/nixpkgs/manual/#reviewing-contributions-package-updates in the note, and improve that substantially with what we've already coalesced as helpful would give our new reviewers a recipe for success.

@7c6f434c

For me «what you tested» sounds like «what functionality you tested», but after @worldofpeace comment I understood this can honestly be perceived as «what package you tested» which is indeed of little use.

There's lots of PR's that touch multiple packages, such as updates of libraries, in which case it's important to know what was tested.

@worldofpeace

Do we have a reviewers template for package updates?

Defining a set of things to do and what to look for can make it even clearer for someone to just jump in and start reviewing.

I'd definitely love for something like this to exist in the future, but until then I think such a simple encouragement in the PR template would be a good start.

We will also get across what kind of quality we expect from reviewers.

At this point I'd rather have some reviews at all instead of people merging PR's with less and less testing (due to amount of them), causing more breakages in the end. I don't want to scare everybody away by indicating we only want quality reviews.

At this point I'd rather have some reviews at all instead of people merging PR's with less and less testing (due to amount of them), causing more breakages in the end. I don't want to scare everybody away by indicating we only want quality reviews.

Substitute quality for formulation. We all already have our formula, that's how we get by.
It just needs to communicated and out in the open.

I'd definitely love for something like this to exist in the future, but until then I think such a simple encouragement in the PR template would be a good start.

Let's see what we can do, I'm ready for trying to get something out in the works 😄

How about suggesting to run nix run nixpkgs.nix-review -c nix-review pr <pr-number>?

I think we should clarify that we are asking people to contribute a review of other peoples PR's.

@dotlambda Not sure about that. nix-review probably works well for PRs that update a dependency that updates a couple dependent packages. But it won't be practical for PRs that change 100 dependents (nobody wants to build all of those). And it would also encourage users to just run that command and not test the thing properly. Also there's lots of PRs that don't just update packages, for which nix-review can't do anything. So I'd rather keep these instructions very general so they can apply to all PRs and leave room for human decisions for what makes sense to test.

@veprbl I think it's pretty obvious that we don't mean to review their own PRs. And even if not, people testing their own PRs should be done in any case, so there's no harm if they do that (which they'll only consider if they haven't already anyways). Also seeing PR authors tend to their PRs makes it more likely for them to be merged anyways. But if you have a good suggestion for how to word this, I'd love to see it.

I would say we could suggest the option of nix-review with testing a random subset of rev-deps if either the rebuild amount is low or the person feels like running a long build.

I don't know if we want to remind people to read the diff and test the changes they understand and find probably OK: malicious changes that are dangerous to test are possible, and the risk probably grows with popularity…

I would say we could suggest the option of nix-review with testing a random subset of rev-deps if either the rebuild amount is low or the person feels like running a long build.

There's only so much to communicate in an encouragement before you send them on their way, and isn't nix-review in Things done now?

I think more detailed descriptions of how and what to test in each scenario could go into the Reviewing Contributions section instead. Maybe we could link that section as a suggestion but make it clear that it's no requirement (to not discourage people, because that section doesn't look very beginner friendly).

Actually I'm not so sure anymore about linking that section: Right now it only gets updates every 6 months, the current one still mentions nox-review for example. I think we're better off not linking to a potentially always outdated manual, especially for that section, because we alter PR workflows more quickly than only exactly every 6 months.

If we had an always up-to-date render of the master/unstable version of the manual I wouldn't mind linking that at all.

Actually I'm not so sure anymore about linking that section: Right now it only gets updates every 6 months, the current one still mentions nox-review for example. I think we're better off not linking to a potentially always outdated manual, especially for that section, because we alter PR workflows more quickly than only exactly every 6 months.

If we had an always up-to-date render of the master/unstable version of the manual I wouldn't mind linking that at all.

I think it makes sense for the nixpkgs manual to always be the version on master.
That's where new contributions go anyways.

Note I've suggested linking to this as well in this thread

I feel like if we could link to https://nixos.org/nixpkgs/manual/#reviewing-contributions-package-updates in the note, and improve that substantially with what we've already coalesced as helpful would give our new reviewers a recipe for success.

Technically, we have this link: https://hydra.nixos.org/job/nixpkgs/trunk/manual/latest/download/1/nixpkgs/manual.html

If we advise people to try the stable version first, then the landing-page manual should be for the stable version, and master version should be linked in contexts intended for contributors.

If we advise people to try the stable version first, then the landing-page manual should be for the stable version, and master version should be linked in contexts intended for contributors.

That is certainly true for the NixOS manual but I think it's worth considering a change w.r.t. the nixpkgs one.

Sounds good, I added this link. I also encased both links with <> as per Apendix E of RFC 2396

@dotlambda @oxij Is this PR looking good for you?

Done did the squashing

Does GitHub soft wrap long lines, or will lots of this be cut off?

I pushed that commit to my master branch, so you can see what it looks like when you make a new PR here. I think these long spaces after and before the comment should go.

Removed the big spaces

@alyssais It does wrap them

This is how it currently looks to open a new PR in the default github look:

I'd like to make this a bit shorter, I'll try rewording it a bit and using link shorteners (any recommendations?)

I shortened the wording by a lot, such that at least the first heading is visible:

@oxij Thanks, very cool.

Since I feel like that PR will take some time to be merged though, I think we should merge this one soon in any case.

Can we do the

<!---

and

--->

on newlines?

@worldofpeace I removed this in recent commits, because it only takes up space, and I'd like for users to at least see the "Motivation for this change" line by default. With these on separate lines that's almost impossible.

@worldofpeace Is it alright like this or do you think we need those newlines?

@worldofpeace Is it alright like this or do you think we need those newlines?

This change has looked good for a while. How much can newlines help with right 😄

Though it would make it look less flowy' because its grouped between something.

So I think we should be good to merge 👍

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1

Seeing @timokau's discourse thread linked above, maybe it would make sense to add a link to the discourse thread in the PR template? Not sure what @timokau thinks about it yet, though, as it may be better to start that discourse thread experiment without too much incentive.

Now, I'm wondering whether discourse is a proper UI, as if multiple committers monitor the thread there is no good way of marking a comment over there as “done” so only one needs to click the link I think, but…

Seeing @timokau's discourse thread linked above, maybe it would make sense to add a link to the discourse thread in the PR template? Not sure what @timokau thinks about it yet, though, as it may be better to start that discourse thread experiment without too much incentive.

I'd be happy with that, but maybe give it a few ways to see how it goes :)

Now, I'm wondering whether discourse is a proper UI, as if multiple committers monitor the thread there is no good way of marking a comment over there as “done” so only one needs to click the link I think, but…

https://discourse.nixos.org/t/discussion-pr-reviews/2619