chore: modernize PR template #92612

blaggacao · 2020-07-07T17:59:27Z

Also add links to marvin labels.
@Mic92 This is an intention-aware refactoring/strip down according to your nix-review package.

I also referenced @timokau 's marvin labels. They are unstable, so we should remember I put them here.

Follow up of #92244

Also add links to <experimental> marvin labels.

blaggacao

Some doubts ...

.github/PULL_REQUEST_TEMPLATE.md

blaggacao · 2020-07-07T18:09:20Z

.github/PULL_REQUEST_TEMPLATE.md

- [ ] Ensured that relevant documentation is up to date
+   - [ ] other Linux (Ubuntu, Archlinux, Alpine, etc.). Please specify: ...
+
+- [ ] If available*: tested via one or more NixOS test(s) `GITHUB_TOKEN=<YOUR_TOKEN> nixpkgs-review pr -p nixosTests.<test> <package> <pr-number>` (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests)


Is this <test> <package> or more precisely rather <package> <package>? The usage example was ambiguous.

.github/PULL_REQUEST_TEMPLATE.md

blaggacao · 2020-07-07T20:16:20Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Tested sucessful built of final PR `GITHUB_TOKEN=<YOUR_TOKEN> nixpkgs-review pr <pr-number>`.
+      If suceeded, within the resulting `nix-shell`:
+   - [ ] Manually tested execution of all binary files (in `./results/bin/`)
+   - [ ] Include the manual checks and validations you did at the end in `editor ./report.md`


Akin to Mic92/nixpkgs-review#113

`editor ./report.md` fits the bill

Mic92 · 2020-07-07T20:19:34Z

.github/PULL_REQUEST_TEMPLATE.md

+Requirements: 
+- install `nixpkgs-review` for good with `nix-env -f '<nixpkgs>' -iA nixpkgs-review`
+- consult usage documentation: https://github.com/Mic92/nixpkgs-review#usage
+- setup github api token: https://github.com/Mic92/nixpkgs-review#github-api-token


I think we can skip this step. The github token is only required for advanced features.

I have a split argument about this:

pull requester will be nudged into graduating into reviewers

it helps towards a standardized review process if the pull requester already does: nixpkgs-review post-result

(and also: don't be too easy on pull requester, to shift the balance away from reviewers)

What do you think about those arguments?

Could we make the request for API token part of first-time running nixpkgs-review/using the advanced features? That way we would not have to mention it here.

@jtojnar This is a really nice suggestion! I'll take it out here and open an issue to track it.

.github/PULL_REQUEST_TEMPLATE.md

Co-authored-by: Jörg Thalheim <Mic92@users.noreply.github.com>

nixos-discourse · 2020-07-07T21:17:17Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/making-the-pull-request-template-more-beginner-friendly-nixpkgs/388/5

jtojnar

Nice. I like the simplification and having a single tool do as much as work possible will make reviewing much easier.

Only serious issue is making the local eval mandatory. Sure, nixpkgs-preview can take eval results from ofborg but that is not available before PR is opened and even a while after. Not sure if the local eval is even necessary, maybe we could just disable it, ofborg will eval it either way.

.github/PULL_REQUEST_TEMPLATE.md

jtojnar · 2020-07-07T23:07:47Z

.github/PULL_REQUEST_TEMPLATE.md

+- setup github api token: https://github.com/Mic92/nixpkgs-review#github-api-token
+-->
+
+- [ ] Tested sucessful build of final PR `GITHUB_TOKEN=<YOUR_TOKEN> nixpkgs-review pr <pr-number>`.


Also not sure it makes sense to mention pr <pr-number> in the checklist since when a person is contributing, there is no PR-yet.

For “built on platforms”, I can do that before submitting the PR, but here it forces me to leave the terminal to go edit the pull request after submitting it. I use gh so my current workflow is running nix-build and then checking the checkboxes in the text editor, before submitting.

This looks like a good workflow. However:

The pullrequesters (now) iare encuraged to complete the same worfklow as the reviewers

That is, they are encouraged to nixpkgs-review post-result this is only available once the pr exists

This can't be addressed in this PR, but it might make sense to support creating a PR with nixpkgs-review altogether so that already the commit message can combine:

PR template for the respective PR type (module update, package update, new, etc) with type-specific check boxes

concatenated with the result of the nixpkgs-review evaluation

Mic92/nixpkgs-review#115

Well, there is nixpkgs-review rev HEAD which can be used by the pullrequester, maybe we could add a comment like

.github/PULL_REQUEST_TEMPLATE.md

jtojnar · 2020-07-07T23:16:08Z

.github/PULL_REQUEST_TEMPLATE.md

- [ ] Ensured that relevant documentation is up to date
+   - [ ] other Linux (Ubuntu, Archlinux, Alpine, etc.).
+
+- [ ] If available: tested via one or more NixOS test(s) `GITHUB_TOKEN=<YOUR_TOKEN> nixpkgs-review pr -p nixosTests.<test> <package> <pr-number>` (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests)


We might also want to mention passthru.tests. Or implement Mic92/nixpkgs-review#77 and add the mapping to every package with NixOS test.

Could you make a suggestion until the mentioned PR is merged, or should we just be pationt until then and leave this as it is?

Maybe we could add a comment like “look for passthru.tests within the package expression” or “try running nix-build -A <package>.tests”. Or just ignore it for now, since ofborg will already run passthru.tests so there is not much need to run them locally. I guess that is fine, no need to ask reviewers to do what machines can.

.github/PULL_REQUEST_TEMPLATE.md

Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

This is since for some reviewers, memory usage is fencing off it's usage

blaggacao · 2020-07-08T00:35:46Z

I have a better idea, am preparing PR with all the feedback incorporated.

jtojnar · 2020-07-08T00:36:10Z

.github/PULL_REQUEST_TEMPLATE.md

+   - [ ] other Linux distributions (Ubuntu, Arch Linux, Alpine, etc.).
+
+- [ ] If available: tested via one or more NixOS test(s) `nixpkgs-review pr -p nixosTests.<test> <package> <pr-number>` (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests)
+<!-- Note, that only few tests are available, if you'd want to write your own have a look at: https://github.com/NixOS/nixpkgs/issues/34987 and furthermore read through https://nixos.org/nixos/manual/index.html#sec-nixos-tests. Looking through the source code should be helpful as well. -->


Really, the installed tests are only relevant to fraction of GNOME and GNOME-adjacent software, most of which already has a package so I would not waste characters on promoting it here. Saying that as person interested in these tests.

blaggacao · 2020-07-08T06:21:12Z

Superseded by #92676

chore: modernize PR template

36fc2f7

Also add links to <experimental> marvin labels.

ofborg bot added 6.topic: policy discussion 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jul 7, 2020

blaggacao commented Jul 7, 2020

View reviewed changes

David Arnold added 4 commits July 7, 2020 13:11

fixup

52d5a3d

wording

1c5a7b3

fixed results path

f266e96

encourage modifying report

ec9de26

blaggacao commented Jul 7, 2020

View reviewed changes

David Arnold added 2 commits July 7, 2020 15:17

wording

1623986

obsolete comments instruction

040cdcf

`editor ./report.md` fits the bill

Mic92 reviewed Jul 7, 2020

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

Update .github/PULL_REQUEST_TEMPLATE.md

cc8dd52

Co-authored-by: Jörg Thalheim <Mic92@users.noreply.github.com>

blaggacao requested a review from Mic92 July 7, 2020 20:25

typo

4e95aef

jtojnar reviewed Jul 7, 2020

View reviewed changes

David Arnold and others added 8 commits July 7, 2020 18:46

Update .github/PULL_REQUEST_TEMPLATE.md

4adc442

Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

Update .github/PULL_REQUEST_TEMPLATE.md

28ea62b

Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

Update .github/PULL_REQUEST_TEMPLATE.md

ea56a6c

Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

link docs about testing

da5ebc5

Update .github/PULL_REQUEST_TEMPLATE.md

adf1c98

Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

offload token init to nixpkgs-review

d420624

reword to strongly suggest

71d89d6

This is since for some reviewers, memory usage is fencing off it's usage

typo

4cb2da3

blaggacao closed this Jul 8, 2020

jtojnar reviewed Jul 8, 2020

View reviewed changes

blaggacao deleted the patch-2 branch August 1, 2021 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: modernize PR template #92612

chore: modernize PR template #92612

blaggacao commented Jul 7, 2020

blaggacao left a comment

blaggacao Jul 7, 2020

blaggacao Jul 7, 2020

Mic92 Jul 7, 2020

blaggacao Jul 7, 2020

jtojnar Jul 7, 2020 •

edited

blaggacao Jul 7, 2020

nixos-discourse commented Jul 7, 2020

jtojnar left a comment

jtojnar Jul 7, 2020

blaggacao Jul 8, 2020

blaggacao Jul 8, 2020

jtojnar Jul 8, 2020

jtojnar Jul 7, 2020

blaggacao Jul 8, 2020

jtojnar Jul 8, 2020

blaggacao commented Jul 8, 2020

jtojnar Jul 8, 2020

blaggacao commented Jul 8, 2020

chore: modernize PR template #92612

chore: modernize PR template #92612

Conversation

blaggacao commented Jul 7, 2020

blaggacao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar Jul 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented Jul 7, 2020

jtojnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaggacao commented Jul 8, 2020

Choose a reason for hiding this comment

blaggacao commented Jul 8, 2020

jtojnar Jul 7, 2020 •

edited