Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PULL_REQUEST_TEMPLATE: Add encouragement to review PRs #58098

Merged
merged 1 commit into from Apr 4, 2019

Conversation

@Infinisil
Copy link
Member

commented Mar 22, 2019

Motivation for this change

As you might have realized, the number of open PR's keeps increasing, we don't have enough people that review, test and merge PRs. This PR hopes to make this a bit better by informing people of this when they open a new PR and encouraging them to review contributions on their own.

If you have rewording suggestions, I'd love to hear them.

This is what I suggested in #50105 (comment)

Ping @edolstra @7c6f434c @alyssais @samueldr @FRidh @Mic92 @domenkozar @Drakonis @aanderse

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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?

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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.

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

@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.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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 😄

@worldofpeace
Copy link
Member

left a comment

No problems with this change.
It provides immediate exposure to our issue every time someone opens a pr.

@dotlambda

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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

@veprbl

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

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

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2019

@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.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

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…

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

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?

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2019

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).

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

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.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

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.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

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.

@dotlambda

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

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.

@oxij

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

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

@worldofpeace
Copy link
Member

left a comment

Looks good, not sure about the divergence of linking to the unstable manual though.
It's better to link to that one, but we really should have something like that arranged at nixos.org
in the future.

@Ekleog

Ekleog approved these changes Mar 26, 2019

@oxij

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@Infinisil Infinisil force-pushed the Infinisil:encouraging_reviews branch from be1eecd to bbf50d7 Mar 26, 2019

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

Done did the squashing

@alyssais

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

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

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

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.

@Infinisil Infinisil force-pushed the Infinisil:encouraging_reviews branch from bbf50d7 to 0dab43c Mar 26, 2019

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

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:
unexpanded

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

oxij added a commit to oxij/nixos-homepage that referenced this pull request Mar 27, 2019

@oxij oxij referenced this pull request Mar 27, 2019
@oxij

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@Infinisil Infinisil force-pushed the Infinisil:encouraging_reviews branch from 0dab43c to b7409da Mar 28, 2019

@Infinisil Infinisil force-pushed the Infinisil:encouraging_reviews branch from b7409da to 4e398a3 Mar 28, 2019

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

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

@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.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Can we do the

<!---

and

--->

on newlines?

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

@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.

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

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

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@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 👍

@worldofpeace worldofpeace merged commit 1d38a2f into NixOS:master Apr 4, 2019

10 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details

@Infinisil Infinisil deleted the Infinisil:encouraging_reviews branch Apr 4, 2019

@nixos-discourse

This comment has been minimized.

Copy link

commented Apr 7, 2019

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

@Ekleog

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

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…

@timokau

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.