[Discussion] PR reviews

Random thoughts about PRs:

Could Nixpkgs adopt a policy to ease the update burden? Perhaps a set of tests that would signal to ofborg that the community would accept minor version bumps for specific packages without manual review? This has the cost of increased breakages for people on bleeding edge. But those breakages focus the effort to where it is most critical.

Increase the number of people who can commit? Assign areas of responsibility?

Encourage reviewers/commuters to modify PRs directly instead of relying on the originator review-recommend-edit cycle.

Question to reviewers: what slows you down the most? What gives you the most reason the doubt a PR?

Is the broken/not broken system too binary?

Any other ways we could improve the nix-review process? Is there a danger of over-reliance on nix-review? (Perhaps a list of problems that are often missed by nix-review).

I suspect nix-fmt could have a strong positive influence on the PR effort, reducing manual syntactic changes.

Remove old/stale PRs, or backlog them?

Any other policies that people apply during the review process that could be automated?

Anything the Guix community does that could help?

3 Likes

Some incomplete answers:

Even if we wanted to do that, how would we automatically differentiate between minor version bumps and potentially malicious PRs?

That has been part of the solution for a while, but its not working very well. I feel like new committers often tend to lose interest / burn out. Its not infinitely scalable either: each committer is a potential security risk for nixpkgs users.

I think that is the exact opposite of what we need for two reasons:

  • we have plenty of people who want to get changes in, not many people who review. Shifting burdon from the first group to the second is not going to help
  • the people proposing changes are not going to learn that way

Lack of clear commenting of intentions is one thing. Missing upstream reports another. GitHubs UX yet another.

I still think a process of

  • contributor proposes PR, it is tagged with needs:review
  • reviewer reviews, tags with needs:changes / merges / tags with needs:merge
  • contributor makes necessary changes, re-tags with needs:review.

Then, we can just automatically close all PRs that have been in needs:changes for too long and reviewers have a clear place to look.

Even if we wanted to do that, how would we automatically differentiate between minor version bumps and potentially malicious PRs?

Because a minor version bump changes exactly two lines in the code: version and sha256 in a predictable way.

That has been part of the solution for a while, but its not working very well. I feel like new committers often tend to lose interest / burn out. Its not infinitely scalable either: each committer is a potential security risk for nixpkgs users.

So do old committers, though.

Changing version to something that is not an upstream version may have security implications though. I’m not sure if it is really that “predictable”.

Now just trusting r-ryantm would be a different matter. I think more or less the only thing blocking that is more thorough automated reverse-dependency testing.

Of course, I didn’t mean to imply that the spoiled youth has become lazy :wink: Just that making more people committers does not have as strong an impact as one would hope.

Changing version to something that is not an upstream version may have security implications though.

Well, if the source is fetched by a version-based URL from a known-good upstream mirror, we do get some version from upstream. Checking that version actually increases should not be that hard. Whether the version is the latest is a better question, of course. (On the other hand, the more open PRs we have, the easier it is to make a human miss this question, too)

Of course, I didn’t mean to imply that the spoiled youth has become lazy :wink: Just that making more people committers does not have as strong an impact as one would hope.

Well, if we do not add committers and do not add a way for non-committer maintainers to merge or something like that, the situation is getting worse on its own; if we add many committers, the situation might be getting slightly better, even if slower than desired…

What about adding committers for specific packages? Or rather, meta.maintainers could approve a PR, and a passing ofborg run could be enough to commit. It would provide more meaning to be listed as a maintainer.

As a maintainer and user of a package, I am in a much better position to evaluate PRs related to it. When trying to review other packages, I often cannot do a proper evaluation due unfamiliarity with the package/ecosystem/etc.

2 Likes

I also find it “frustrating” sometimes there are small commits that we could validate that would save time for other more “experienced” committers who would be more beneficial in their time to validate new packages or complex modifications.