pr: add GitHub approval upon success with --approve-pr

[ethancedwards8]

This feature will add an approval to Github upon all packages building when pr is passed the flag --approve-pr.

I found this especially needed when reviewing r-ryantm PRs as they usually can be approved if they build/pass tests

[ethancedwards8]

@GaetanLepage sorry for the ping, but wanted to know your thoughts/feedback?

[GaetanLepage]

This is a very good idea indeed!
I see myself using this.

Wdyt @SuperSandro2000 @Mic92?

[SuperSandro2000]

I think I like the idea of this but I lack the time to properly review this.

[GaetanLepage]

Thanks for the input @SuperSandro2000 I have reviewed the implementation.

@ethancedwards8 it seems like there is an unused instance of approve_pr: https://github.com/Mic92/nixpkgs-review/actions/runs/13470012783/job/37955868668?pr=471

[ethancedwards8]

Fixed it. Thanks!

[Mic92]

Mhm. It feels a bit odd to me that we encourage not testing any of the build packages this way. it feels like it would make your approval less meaningfull if you haven't even tried to run the build package. I would prefer if the approval would also state that it was generated by nixpkgs-review --approve-pr in a review comment.

This is especially needed for ryantm PRs where the automatic testing already happened.

[ethancedwards8]

If you mean making sure the binary works/runs, couldn't that just be solved by adding versionCheckHook (or something similar) to more packages?

[GaetanLepage]

Mhm. It feels a bit to me that we encourage not testing any of the build packages this way. it feels like it would make your approval less meaningfull if you haven't even tried to run the build package. I would prefer if the approval would also state that it was generated by nixpkgs-review --approve-pr in a review comment.

This is especially needed for ryantm PRs where the automatic testing already happened.

I agree in general, but this is not always possible.
For instance, I work mainly on python libraries. When I review such PRs, my workflow is:

1. Review the diff and check the upstream code changes if the PR is a version bump.
2. Run nixpkgs-review
3. Approve & merge

This scenario is a significant share of the PRs I review. This feature would make my life easier for sure.

Maybe we can print a message in the terminal to warn the user about the risks of such automation.
Also, I agree that the approval message should give extra context in the GH thread.

[Mic92]

Could expand approve_pr like this and add the message? Than it's up to the merger what to do with this message.

     def approve_pr(self, pr: int, comment: str = "") -> Any:
         "Approve a PR with an optional comment"
         print(f"Approving {pr_url(pr)}")
         data = {"event": "APPROVE"}
         if comment:
             data["body"] = comment
         return self.post(
            f"/repos/NixOS/nixpkgs/pulls/{pr}/reviews",
             data=data,
         )

[ethancedwards8]

I added that feature. See here: NixOS/nixpkgs#385798

[GaetanLepage]

LGTM!

[GaetanLepage]

I tried it and got:

urllib.error.HTTPError: HTTP Error 422: Unprocessable Entity

[ethancedwards8]

Which PR did you test it on?

[ethancedwards8]

Worked for me: NixOS/nixpkgs#386703

[ethancedwards8]

Did you run it on a PR which you opened? It might have trouble then. Not sure. I will try that

[ethancedwards8]

Yeah. I ran it on a PR that I opened. NixOS/nixpkgs#373125

^^^^^^^^^^^^^^^^^^^
  File "/nix/store/ffxlyz7jrxxyxg70cn3h961lr91lan2j-python3-3.12.8/lib/python3.12/urllib/request.py", line 630, in http_response
    response = self.parent.error(
               ^^^^^^^^^^^^^^^^^^
  File "/nix/store/ffxlyz7jrxxyxg70cn3h961lr91lan2j-python3-3.12.8/lib/python3.12/urllib/request.py", line 559, in error
    return self._call_chain(*args)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/ffxlyz7jrxxyxg70cn3h961lr91lan2j-python3-3.12.8/lib/python3.12/urllib/request.py", line 492, in _call_chain
    result = func(*args)
             ^^^^^^^^^^^
  File "/nix/store/ffxlyz7jrxxyxg70cn3h961lr91lan2j-python3-3.12.8/lib/python3.12/urllib/request.py", line 639, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 422: Unprocessable Entity

Since you can't approve your own PR, it freaks out.

[GaetanLepage]

Since you can't approve your own PR, it freaks out.

Is there a way to check that?
Or at least failing more graciously?

[Defelo]

Is there a way to check that?

The graphql api provides a field that tells you whether you authored the PR:

query {
  repository(owner: "NixOS", name: "nixpkgs") {
    pullRequest(number: 387466) {
      viewerDidAuthor
    }
  }
}

Alternatively you could use the rest api to get your own user id via /user -> .id and then the PR author id via /repos/nixos/nixpkgs/pulls/NUMBER -> .user.id and compare them

[Ma27]

I have a little remark about this: a successful run of nixpkgs-review is not necessarily sufficient to consider a change safe. My favorite example for this is Nextcloud: the review essentially checks if the package builds, i.e. if the sources can be copied into the store-path. And this is definitely not the only case.

I do understand that this is useful in many other cases and I don't think the feature itself shouldn't land. However, I'd be very grateful if you could add a note to the docs (and perhaps even a link or short note about that in the helptext) that peoople should carefully consider whether the review they're running is actually sufficient to approve a PR.

[GaetanLepage]

I have a little remark about this: a successful run of nixpkgs-review is not necessarily sufficient to consider a change safe. My favorite example for this is Nextcloud: the review essentially checks if the package builds, i.e. if the sources can be copied into the store-path. And this is definitely not the only case.

I do understand that this is useful in many other cases and I don't think the feature itself shouldn't land. However, I'd be very grateful if you could add a note to the docs (and perhaps even a link or short note about that in the helptext) that peoople should carefully consider whether the review they're running is actually sufficient to approve a PR.

Yes, your comment is very relevant. As mentioned above, this should only been used in specific trivial cases where the reviewing has already been done and nixpkgs-review is the only step left before merging.
In practice, I find that this is the case when updating python libraries. Once the diff and upstream changes have been checked, nixpkgs-review is the last checkup to be done.

I agree that a warning/documentation would be a good addition.

[Mic92]

I have a little remark about this: a successful run of nixpkgs-review is not necessarily sufficient to consider a change safe. My favorite example for this is Nextcloud: the review essentially checks if the package builds, i.e. if the sources can be copied into the store-path. And this is definitely not the only case.

I do understand that this is useful in many other cases and I don't think the feature itself shouldn't land. However, I'd be very grateful if you could add a note to the docs (and perhaps even a link or short note about that in the helptext) that peoople should carefully consider whether the review they're running is actually sufficient to approve a PR.

That was also my concern, which is why suggested to make approvals created by nixpkgs-review visually different from normal approvals: https://github.com/Mic92/nixpkgs-review/pull/471/files#diff-068fea7dedadb885b7dcccc0fe1bc843caa6f1da0c6622ee7a7d56e99c31475aR421

Than it's up to the merge what to do with this information.

[Mic92]

Since you can't approve your own PR, it freaks out.

Is there a way to check that? Or at least failing more graciously?

I think we can just catch the error and check for the http result code and print a warning with possible explanations but without failing nixpkgs-review (since it's not crucial to normal usage of nixpkgs-review if the approval works or not).

[pbsds]

Would posting a GH approval ✔️ be smart if it only serves to decrease the signal to noise ratio of human review? Wouldn't it instead make sense to indicate in the nixpkgs-review report with a unique searchable string that all builds succeeded? A human can already quickly glance that a nixpkgs-review has passed, but not search for such PRs currently. I frequently search for terms like "nixpkgs-review pr " and "- [x] Tested basic functionality of all binary files".

Using a unique string would both aid discoverability while not diluting the approvals: n labels. Then more granularity could be included: All builds succeeded for one platform, All builds succeeded for all platforms, etc...

[GaetanLepage]

My understanding of the use case for this flag is the following:
"I have checked the diff, and now, the only thing left before I can approve (and merge) the PR is a succesfull nixpkgs-review run."
-> This lets me do those two last steps in one go.
I reiterate that I don't expect to be using it for all PRs.
Actually, in this sense, we shouldn't even have to add a custom message to the approval. It's only a shortcut for the committer which should know what he is doing.
A successful nixpkgs-review run is obviously not the only necessary condition for a proper PR approval.

Maybe, people could misunderstand this, and I'm fine with adding a custom message.

TLDR: For me, such automatically posted approvals should be used, and understood as plain, legitimate approvals.

[ethancedwards8]

Alright, thank you everyone for your patience with me. I've had a crazy few weeks. I am a high school senior so I have been visiting colleges and wrapping up with exams.

I tested catching the error against this PR: NixOS/nixpkgs#388325 and it works. Let me know if anyone has any questions.

[ethancedwards8]

Also, fwiw, I have been using this for a little over two weeks with no complaints or concerns over it. https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr%20Approved%20automatically%20following%20the%20successful%20run%20of%20nixpkgs-review.%20is%3Aclosed . I think people are generally understanding of what the approval means. Like Mr. Lepage mentioned, this is pretty helpful for python package bumps.

[GaetanLepage]

Final nit, otherwise, LGTM!

[ethancedwards8]

Tested warn here NixOS/nixpkgs#388319

[GaetanLepage]

Thanks @ethancedwards8 !