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.

Sign up for GitHub

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

Jump to bottom

chore: add typed pr templates #92676

Closed
wants to merge 5 commits into from
Closed

chore: add typed pr templates #92676

wants to merge 5 commits into from

Conversation

blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Jul 8, 2020 •
edited

Motivation for this change

Replacing #92612 & #92244

Further evolution blocked by: Mic92/nixpkgs-review#117

@blaggacao
Copy link
Contributor Author

blaggacao commented Jul 8, 2020 •
edited

Example rendered New Module PR

Motivation for this change
General review
  • module path fits the guidelines
  • module tests succeed on ARCHITECTURE
  • options have appropriate types
  • options have default
  • options have example
  • options have descriptions
  • No unneeded package is added to environment.systemPackages
  • meta.maintainers is set
  • fits CONTRIBUTING.md.
Package build
Linux 5.4.0-7634-generic x86_64
---
No LSB modules are available.
Distributor ID:	Pop
Description:	Pop!_OS 20.04 LTS
Release:	20.04
Codename:	focal
  • package & dependend packages successfully built
nixpkgs-review rev HEAD 
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/78451/head:refs/nixpkgs-review/1
$ git worktree add /home/blaggacao/.cache/nixpkgs-review/pr-78451-11/nixpkgs 29cf4de2054b6d44fcba8a1b1a30257150c78ffa
Preparing worktree (detached HEAD 29cf4de2054)
Updating files: 100% (22067/22067), done.
HEAD is now at 29cf4de2054 Merge pull request #92596 from r-ryantm/auto-update/stress-ng
$ git merge --no-commit 5458f54a8301f59a8acf5d42856d84f8019efd8d
Already up to date.
warning: unknown setting 'experimental-features'
warning: unknown setting 'experimental-features'
$ nix build --no-link --keep-going --option build-use-sandbox relaxed -f /home/blaggacao/.cache/nixpkgs-review/pr-78451-11/build.nix
warning: unknown setting 'experimental-features'
https://github.com/NixOS/nixpkgs/pull/78451
1 package built:
operator-sdk

warning: unknown setting 'experimental-features'
$ nix-shell /home/blaggacao/.cache/nixpkgs-review/pr-78451-11/shell.nix
warning: unknown setting 'experimental-features'

Result of nixpkgs-review pr 78451 1

1 package built:
- operator-sdk
  • ./results/operator-sdk/bin/operator-sdk --help: ok
Documentation
  • module documentation is declared in meta.doc
  • introduced no ortographical errors
Possible improvements
Comments

@blaggacao
Copy link
Contributor Author

Eventually this whole secction could be implemented automatically by nixpkgs-review:
image

Ma27
Ma27 reviewed Jul 8, 2020
View reviewed changes
.github/PULL_REQUEST_TEMPLATE/module_update.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/module_update.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/new_module.md Show resolved Hide resolved
@blaggacao
Copy link
Contributor Author

@Ma27 Thanks for this great review, I resolved part of your comments which would fall under the category "don't shoot the messenger". However, they are valuable feedback for improving the reviewer documentation itself in the future. THis PR tries to wire up things better than they are wired up currently and improve further from that future baseline.

@Ma27
Copy link
Member

Ma27 commented Jul 9, 2020

However, they are valuable feedback for improving the reviewer documentation itself in the future

Agreed. As soon as we're ready with this PR, would you mind opening an issue for that (and reference the above conversation)?

blaggacao
blaggacao commented Jul 9, 2020
View reviewed changes
.github/PULL_REQUEST_TEMPLATE/leaf_package_update.md
Comment on lines +41 to +55
nixpkgs-review rev HEAD

```
</details>

<!--
substitute this command by it's own output when run within the nipkgs-review shell
-->
cat ./report.md

<!--
substitute this command by it's own output when run within the nipkgs-review shell
and test "not ok" binaries manually
-->
for cmd in $(ls ./results/**/**/**); do if $($cmd --help > /dev/null); then echo "- [x] \`$cmd --help\`: ok"; else "- [ ] \`$cmd --help\`: not ok -- tested otherwise"; fi; done
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mic92 Let's replace this whole section by standardized output from the nixpkgs-review's report.md

@evils
Copy link
Member

evils commented Jul 11, 2020

there's a typo in ortographic error
i think this term is too obscure to be used in such a prominent document
i also think no one would intentionally uncheck this (that's to say, if anyone knew they were introducing an error, they wouldn't be introducing an error)
if the goal of that checkbox is to remind a submitter to proofread their work, i suggest

  • checked spelling

additionally, dependend -> dependent

@blaggacao
Copy link
Contributor Author

I'm definitively the first adressee... 😄

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-many-people-are-paid-to-work-on-nix-nixpkgs/8307/21

@blaggacao
Copy link
Contributor Author

Upstream stalled. Desisting.

@blaggacao blaggacao closed this Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thra11 Thra11 Thra11 left review comments

@Ma27 Ma27 Ma27 left review comments

Assignees
No one assigned
Labels
6.topic: policy discussion 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@blaggacao @Ma27 @evils @nixos-discourse @Thra11