Nixpkgs diff for forks #196
Conversation
This is preparation for a future commit that makes this workflow usable for PRs from forks
Makes it possible to use treefmt in the sync-pr.sh script without it being single-threaded slow
infinisil
force-pushed
the
nixpkgs-diff-for-forks
branch
from
79c4675 to
93bd412
Compare
|
The downside is that we'll need separate PRs to test changes to CI, because the workflows are taken from the base branch of PRs, not the PR branch. This is also why CI won't run in this PR. I did just create a PR to verify it works though: tweag#3 |
|
Could you make it that CI takes it from the current branch if it is not a fork? |
|
I think that only works in a hacky way by having it trigger for both |
|
I see. Could you maybe make this based on the branch name? I.e. only trigger that CI when the branch is called |
infinisil
force-pushed
the
nixpkgs-diff-for-forks
branch
from
93bd412 to
484fa57
Compare
Previously it ran potentially untrusted code (via result/bin/nixfmt). Now it runs all fetched code in derivations for safety. This makes it possible to use it safely for processing PRs from forks (intermediate commit had `-- .` added to the `git checkout` command) Fix sync-pr.sh script when commits existed already Fixes the problem seen in https://github.com/NixOS/nixfmt/actions/runs/8824022484/job/24226382708?pr=196 This happens because `git checkout` only updates HEAD when no path was given. And we need it to update HEAD, because otherwise it thinks our files get overridden when we try to switch branches.
infinisil
force-pushed
the
nixpkgs-diff-for-forks
branch
from
a61d385 to
276014f
Compare
By switching from pull_request to pull_request_target, which gives access to secrets even for forks. This is only safe because of parent commits making sure that all untrusted code is run sandboxed in derivations.
More efficient and restricted
Allows running `nix-build -A ci` without relying on `nix-build`'s recursion
Turns out `github.ref` doesn't point to the pull request's ref when using `pull_request_target`: NixOS/nixpkgs#306430
CI is getting too slow, rebuilding too many derivations, especially now that we depend on a patched treefmt (ironically the patch is there to speed it up)
Hides warnings for: - Not having a default branch name set - Checking out non-branches
Now it can be run with nixfmt URLs that have any casing. This was really annoying when I tried to debug CI using ./sync-pr.sh https://github.com/nixos/nixfmt 196 https://github.com/NixOS/nixpkgs until I realised that CI used https://github.com/NixOS/nixfmt with a cased "NixOS"!
infinisil
force-pushed
the
nixpkgs-diff-for-forks
branch
from
276014f to
19d12a6
Compare
|
I tried really hard, but it's just not reasonably possible. I managed to kind of get it working, but this is super hacky and broken in some cases. So without that, I'm just testing it in a fork, and that does work: tweag#3. And the |
|
Can confirm that it works, see #193 :) |
This is super tricky stuff, but this should allow us to run the Nixpkgs diff for forks without safety issues (e.g. for #193). See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ for some background.
Also included is:
Best reviewed commit-by-commit.
This work is sponsored by Antithesis ✨