PRs already reviewed

3 posts were merged into an existing topic: [Discussion] PR reviews

@tomberek thank you for starting with the reviews :slight_smile:

Please do remember to do your review as a GitHub review (Files changed → Review changes). That has several advantages:

  • the PR will no longer show up in the filtered view I link from the top post
  • it is easier to understand the reasoning later
  • you will get subscribed to the PR in case there will be future discussion
  • as a little bonus there is a gamification aspect: it will be possible to generate statistics who reviewed how many PRs

This post is only to get someone to act on the review. It should be enough to just post “merge: <link>” or you could copy your review here. But it should not only be here, especially if these posts are deleted.

Regarding your second review: Please only post clear-cut reviews here. Your review is of course helpful and you could open a discussion in the PR and maybe ask for advice on IRC or a separate discourse post. But this post should be reserved for cases where the committer only has to “review a review”, not make a decision themselves or be particularly familiar with the particular packages.

Please don’t read the remarks in any negative way, I’m glad you started with the reviews :slight_smile: This is just the first step in refining the process.

2 Likes
Finished ~~merge: https://github.com/NixOS/nixpkgs/pull/58681~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58703~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58762~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58862~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57478~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58008~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58005~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57335~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58656~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58316~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58673~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58299~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58883~~ ~~re-run ofborg: https://github.com/NixOS/nixpkgs/pull/58909~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/49868~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/52589~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/52948~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/56809~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57937~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59077 (approved by @matthewbauer ,awaiting merge, minimal impact)~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59307~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/56418~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59502~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59474~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59493~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59476~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59447~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59383~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59436~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59536~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59634~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59649~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60051~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60205~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60190~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60156~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59643~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58774~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60338~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60160~~ ~~needs more reviewers: https://github.com/NixOS/nixpkgs/pull/60102~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60484~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60486~~ merge: https://github.com/NixOS/nixpkgs/pull/60494 merge: https://github.com/NixOS/nixpkgs/pull/60531 merge: https://github.com/NixOS/nixpkgs/pull/60532 merge: https://github.com/NixOS/nixpkgs/pull/60547 merge: https://github.com/NixOS/nixpkgs/pull/60550 merge: https://github.com/NixOS/nixpkgs/pull/60307 merge: https://github.com/NixOS/nixpkgs/pull/60281 merge: https://github.com/NixOS/nixpkgs/pull/60315 merge: https://github.com/NixOS/nixpkgs/pull/60277

needs more testing/reviewers: gawk: 4.2.1 -> 5.0.1 by r-ryantm · Pull Request #59503 · NixOS/nixpkgs · GitHub
merge or deny: https://github.com/NixOS/nixpkgs/pull/47794
merge: nixbox: init at unstable-2019-04-20 by LinArcX · Pull Request #59967 · NixOS/nixpkgs · GitHub
fix darwin: bfs: 1.3.3 -> 1.4 by r-ryantm · Pull Request #60474 · NixOS/nixpkgs · GitHub
merge: oxidized: 0.26.2 -> 0.26.3 by WilliButz · Pull Request #60480 · NixOS/nixpkgs · GitHub

5 Likes

I’ll start easy :wink:

~~merge: https://github.com/NixOS/nixpkgs/pull/58643~~
~~merge: https://github.com/NixOS/nixpkgs/pull/56464~~
~~merge: https://github.com/NixOS/nixpkgs/pull/58442~~

I don’t know enough about node to say this is good, but I have reviewed the module and everything there looks good. If someone would like to review the package that would be appreciated:
https://github.com/NixOS/nixpkgs/pull/58096

1 Like

A post was merged into an existing topic: [Discussion] PR reviews

These are simple updates. I tested both with nix-review but since both have higher rebuild count I would like to have a second opinion:
~~review: https://github.com/NixOS/nixpkgs/pull/58803~~
~~review: https://github.com/NixOS/nixpkgs/pull/53460~~

2 posts were merged into an existing topic: [Discussion] PR reviews

~~merge: https://github.com/NixOS/nixpkgs/pull/57359~~
~~merge: https://github.com/NixOS/nixpkgs/pull/58615~~
~~add tag “status: wait-for-upstream”: https://github.com/NixOS/nixpkgs/pull/54179~~

let’s check this one
merge: aws-xray-daemon-V3.0.0 | awx-xray-daemon: init at V3.0.0 by NRHelmi · Pull Request #53565 · NixOS/nixpkgs · GitHub

@NRHelmi please review the list of requirements for PRs to post here. I understand that you’re probably frustrated, especially since your first contribution has been sitting there for 3 months. You’re encouraged to

  • ping someone you think might be qualified/interested to review in the PR. Infinisil has already reviewed it once and may be willing to give it another look if you remind him
  • ask for a review on IRC or on discourse. Maybe start another iteration of “PRs ready for review”

Unfortunately I’m not very qualified to review nixos modules myself and there is one outstanding comment by Infinisil (aws-xray-daemon-V3.0.0 | awx-xray-daemon: init at V3.0.0 by NRHelmi · Pull Request #53565 · NixOS/nixpkgs · GitHub) who has much more experience with that than I do.

3 Likes

Please check/merge:
https://github.com/NixOS/nixpkgs/pull/57752
https://github.com/NixOS/nixpkgs/pull/57429

Those PRs both have merge conflicts, so are not ready to merge. They are also from nixpkgs members, so posting them here is not necessary (they can merge themselves after you’ve given a positive review).

~~merge - https://github.com/NixOS/nixpkgs/pull/59942~~
~~merge - https://github.com/NixOS/nixpkgs/pull/59971~~
~~merge - https://github.com/NixOS/nixpkgs/pull/59899~~
~~merge - https://github.com/NixOS/nixpkgs/pull/59950~~
merge - tmux: 2.8 -> 2.9 by xrelkd · Pull Request #60194 · NixOS/nixpkgs · GitHub
merge - ipfs: 0.4.19 -> 0.4.20 by elitak · Pull Request #60148 · NixOS/nixpkgs · GitHub
merge - Datadog: update packages by Izorkin · Pull Request #59872 · NixOS/nixpkgs · GitHub

If someone can validate or propose a comment for this pr?

It fixes a bug that prevents to use diskImageFuns.debian9x86_64 and thus generate debian9 debs for example. The workaround is to use the debian8 image for the moment.

https://github.com/NixOS/nixpkgs/pull/60868

Regards

Done :slight_smile:

davidak via Nix community nixos1@discoursemail.com writes:

I left a review for this one:

Merge: https://github.com/NixOS/nixpkgs/pull/58649

These two have both been reviewed by someone else, but could be acted on quickly by a committer:

Close: https://github.com/NixOS/nixpkgs/pull/37353
Tag with WIP: https://github.com/NixOS/nixpkgs/pull/52424

Reasoning: The description of #52424 mentions that it obsoletes #37353, and the most recent comment in #52424 says the contributor is currently testing an additional proposed change.

1 Like

Merge: https://github.com/NixOS/nixpkgs/pull/55361
Close: https://github.com/NixOS/nixpkgs/pull/63476 because this change was included in #63876 which has been merged.

I’m posting here my own PR, because I got 2 positive reviews, but I think it got lost:
merge: ~~https://github.com/NixOS/nixpkgs/pull/63942~~

1 Like

Close: https://github.com/NixOS/nixpkgs/issues/39388
Close: https://github.com/NixOS/nixpkgs/pull/52632

Reason: These have been obsoleted and resolved by #60250.