.github/PULL_REQUEST_TEMPLATE: added md-to-db reminder

[cab404]

Motivation for this change

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[cab404]

E.g #100057

[teto]

could we instead or in complement suggest the install of a pre-push hook that checks this ?

[mohe2015]

Pre-commit? And when then addition I think.

[cab404]

I think there is, although nobody waits for it

…

On October 16, 2021 4:07:17 PM GMT+03:00, Matthieu Coudron ***@***.***> wrote: could we instead or in complement suggest the install of a pre-push hook that checks this ? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #141820 (comment)

[cab404]

Oh, not pre-commit, push ones on GitHub side. I think adding pre-commit checks is whole another discussion.

…

On October 16, 2021 4:45:01 PM GMT+03:00, Moritz Hedtke ***@***.***> wrote: Pre-commit? And when then addition I think. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #141820 (comment)

[mohe2015]

I don't think we can add hooks on the server side. The CI already checks afaik so I don't think there is much room for improvement there

[Atemu]

CI fail on this should probably block merges then.

[r-burns]

Hmm, any objections? We can certainly do more here but I think this 1-line addition is good to go.

[Atemu]

A CI check should be added but having both is also good.

[nixos-discourse]

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

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

[SuperSandro2000]

A blocking commit hook is especially annoying when not properly done and failing in rebases. Also starting a nix-shell and then generating the docs takes best case a few seconds and worst case when nix starts to build things on staging a really long time.

Blocking a merge because docs fail to build or are out of sync is annoying especially if the issue is unrelated and not easily fixable even by a commiter.

In my opinion commiters just need to watch out if both files change and we should probably regenerate the docs in the CI that already builds the docs and make that fail if there is a diff but don't make it blocking.

Also I think we should link to the full docs https://nixos.org/manual/nixpkgs/unstable/#chap-contributing to not duplicate instructions and keep a single source of truth.

[cab404]

Also I think we should link to the full docs https://nixos.org/manual/nixpkgs/unstable/#chap-contributing to not duplicate instructions and keep a single source of truth.

I think that it's not sufficent: that doc is not structured as a cheklist, but PR needs one.

In my opinion commiters just need to watch out if both files change and we should probably regenerate the docs in the CI that already builds the docs and make that fail if there is a diff but don't make it blocking.

This is already in place, but many commiters just tend to skip checks and merge right away.

Also, we can try implementing checklist checker, which would be a small CI task comparing touched nixpkgs areas with checkboxes ticked in PR -- but it even sounds obnoxious)

[SuperSandro2000]

This is already in place, but many commiters just tend to skip checks and merge right away.

Is the check then grey or red? If it is grey, it should become red. If people ignore it anyway please mention that the next you see that happening.

Also, we can try implementing checklist checker, which would be a small CI task comparing touched nixpkgs areas with checkboxes ticked in PR -- but it even sounds obnoxious)

Checking the boxes is not required and many core contributors ignore it and don't fill the template in at all, so that wouldn't really work.

[SuperSandro2000]

I think that it's not sufficient: that doc is not structured as a checklist, but PR needs one.

Also this is duplicated with the point one level up. Why not convert that into a checklist instead of duplicating the info that is linked there?

[cab404]

If I understood you correctly, then yeah — restructuring doc you've mentioned may prove useful.

Solution I propose tries to mitigate a pain point associated with adding "release notes" section.
As I see it, checklist itself is not designed to be a full documentation for how to contribute to nixpkgs -- it is only here to address critical-ish stuff people should really be reminded of checking before making PRs.

Even if they don't check those boxes, they would at least see that there is such thing as "generating release notes if release notes were added".

And "single source of truth" as good as it sounds wouldn't be read by most of our conributors :(