Making the pull-request CI workflow faster

[mightyiam]

From #55, I understand that cachix is much faster than magic nix cache (also from experience in this project).

Also from #55 I understand that there are some security concerns for which we have two workflows, pull-request with the slow magic-nix-cache and push with the fast cachix.

Is there any way we could mitigate the security concern so that we could have only fast workflows with cachix?

@lorenzleutgeb

[lorenzleutgeb]

I think that this is related to #102: Because of rate limits, not all things that could be cached are cached, leading to higher build times. I don't see any good obvious solution.

What we could try would be to use Cachix instead of MNC. That would mean creating a second cache on Cachix, e.g. "ngi-untrusted" which we allow to be writable from PRs. With this, we would not run into rate limits for PRs. We'd just have to check how we can have the builder fallback to the "ngi" cache in case the "ngi-untrusted" cache misses.

[mightyiam]

Or perhaps explicit job approvals?

[lorenzleutgeb]

Good idea. Even better if PRs by members would then be automatically approved.

[jleightcap]

Ah, I believe this is the issue I'm running into in #168 (https://github.com/ngi-nix/ngipkgs/pull/168/checks). CI runs correctly when PR is not based from a fork here (I had forgotten I have direct commit access to this repo.)

If I understand correctly, this error (Error: Resource not accessible by integration) is because the job isn't approved, and there is no mechanism to do so? If that's the case, outside contributors don't seem to have a mechanism to test against CI.

[fricklerhandwerk]

How does it work on other repos that have a button "Approve and run"?

[lorenzleutgeb]

Ah, I believe this is the issue I'm running into in #168 (https://github.com/ngi-nix/ngipkgs/pull/168/checks). CI runs correctly when PR is not based from a fork here (I had forgotten I have direct commit access to this repo.)

Maybe I am misunderstanding you, but I don't think so. I think you are running into the permission issue with thollander/actions-comment-pull-request for which I have proposed a fix at #134.

If I understand correctly, this error (Error: Resource not accessible by integration) is because the job isn't approved, and there is no mechanism to do so?

No. Approval about GitHub Actions from forks is not about giving them the same permission as a repo-internal PR by a member, but about executing it at all. Quoting from the GitHub docs:

Anyone can fork a public repository, and then submit a pull request that proposes changes to the repository's GitHub Actions workflows. Although workflows from forks do not have access to sensitive data such as secrets, they can be an annoyance for maintainers if they are modified for abusive purposes.

To help prevent this, workflows on pull requests to public repositories from some outside contributors will not run automatically, and might need to be approved first. By default, all first-time contributors require approval to run workflows.

Here they emphasize that workflows from forks do not have access to sensitive data (this coincidentally also prevents them from commenting on the PR...), but can still be annoyance. Someone could fork NGIpkgs and create PR which contains a GitHub Actions workflow that mines cryptocurrencies. That's something that GitHub wants to avoid, so it requires the maintainers to approve.

If that's the case, outside contributors don't seem to have a mechanism to test against CI.(

This is correct, but not for the reason that you are suggesting. Instead we should merge #134 (of course I think that this should be preferred) or remove the nix flake show diff commenting altogether.

How does it work on other repos that have a button "Approve and run"?

AFAIK this cannot be explicitly enabled/disabled. GitHub will decide whether approval should be required, and the default seems is to require approval for the first contribution from a non-member (see quote above). They might have changed that policy since they introduced it, but I haven't heard about that.

The only other feature I know of where one can configure approvals is deployment, which I believe is not applicable in our case.

I think the two earlier comments by @mightyiam and me were based on the assumption that approvals would somehow elevate privileges, but they don't. So, approvals in the sense of the "Approve and run" button won't help us solve our caching issue.

[lorenzleutgeb]

Since #177, "internal" PRs are now just as fast as the workflow that previously ran on the "push" event, and they use Cachix. With #181 I am making the next step: Removing Magic Nix Cache in favor of two the Cachix caches ngi and ngi-untrusted. The latter is considered world-writable. Pull requests from forks will use it to cache, and the "push" event (after merge by a member) will the populate the ngi Cache.

(Edit: Fixed wrong reference to #104 instead of #181, sorry.)

[lorenzleutgeb]

I think we've made good progress here, all workflows now use Cachix as was initially requested. @mightyiam if you agree feel free to close the issue, otherwise point me at jobs that are still taking too long for your taste. Thanks!

[fricklerhandwerk]

I'll close it as completed, let's open new, more specific issues when needed.

[mightyiam]

Thank you!