We have no chance of getting a token that can request the team endpoints
in the pull_request context. This makes sense, because non-members of
the org are also not allowed to view the teams' memberships.
Thus, just fake an empty team - that's fine for the Test workflow.
Running the nixpkgs-merge-bot in GitHub Actions instead of a separate
workflow has multiple advantages:
- A much better development workflow, with improved testability.
- The ability to label PRs with a "merge-bot eligible" label from the
same codebase.
- Using more data for merge strategy decisions, for example the number
of rebuilds.
This commits re-implements most of the features from the current
nxipkgs-merge-bot directly in the bot workflow. Instead of reacting to
webhook events, this now runs on the regular 10 minute schedule. Some
merges might be delayed a few minutes, but that should not be a problem
in practice.
To give the user early feedback, there are additional workflows running
when a comment or review is posted. These react with "eyes" to make the
user aware that the comment has been recognized.
The only feature not taken over was the size check for files in the PR.
This kind of check is not really relevant for maintainer merges only -
if we want to prevent bigger files from making it into the tree, then we
need a generic CI check, which is out of scope for the merge-bot.
Other than that, everything should be implemented - any omissions are by
accident.
This workflow / script is already doing more than must labeling: it's
already auto-closing package request issues.
Since we're going to migrate the nixpkgs-merge-bot into this workflow,
we'll rename things to a more generic name.
To be able to disable the pr.yml workflow on GitHub, we need to rename
it to a different name. Let's use the long name for consistency with
merge-group.yml. This only affects the GitHub-internal name, not the
visible name in the PR checklist, which is still "PR". This visible name
is also used by nixpkgs-review, so that won't break.
Instead of setting the maintainer-related labels based entirely on Eval
results, this uses the new maintainer map from the target branch. This
allows labeling PRs correctly, that had been created *before* a
contributor became a maintainer of the respective package.
There is no point in passing these arguments around between all the
different parts of the eval chain - these global settings should surely
not be modified between different steps.
This also makes it simpler to add new arguments like extra nixpkgs
configuration.
This creates a simple mapping of all packages to github ids of all their
maintainers. This is uploaded as an artifact and is then available for
download on the latest commit of each branch with a merge queue. This
will allow scheduled jobs to use this information for setting
maintainer-related labels, to request reviewers and to implement the
merge-bot.
The advantage over querying this information directly via Nix in each
case: The scheduled job does not need to install Nix and does not need
to checkout the target branch.
Compared to obtaining the maintainer information just for a single PR
during Eval, this will allow retroactively changing maintainers for a
package: For example, it allows to request a new maintainer as reviewer
for a PR that was created before they became maintainer, but is still
open - and similarly for maintainer labels and merge-bot rights.
None of these extensions are implemented by this PR, yet.
Currently the `diff-<system>` artifacts are 6-7 MB in size - and almost
all of that is the `paths.json` file, which is only used to generate the
diff itself. This had been stored in the artifact previously for
debugging purposes. Ever since we moved to Cachix this is not required
anymore, since it's possible to run the same eval locally and thus fetch
the `eval.singleSystem` result, including `paths.json`, from Cachix.
This will be even more helpful when the next step adds `meta.json` -
which is magnitudes bigger than `paths.json`.
The before and after of
nix-instantiate --eval -A lib.teams --strict --json | jq 'walk(if type == "array" then sort else . end)'
has been ensured to be negligible, only consisting of minor team
shortName and scope differences
Creates a team sync workflow that pushes the current state of teams to a
JSON file, which can then be ingested by `lib.teams` to expose member
lists.
Co-Authored-By: Alexander Bantyev <alexander.bantyev@tweag.io>
`zizmor` is a tool that uses static analysis to find potential security
issues in GitHub Actions [0]. (Yes, it's a bit absurd that GitHub
made a CI system so complicated that tools like this were created, but
I digress.)
Given our increase in GHA usage recently, I think this is a good step
towards keeping our security posture in tip-top shape. (It also keeps
with the theme of automating as many things as possible!)
The rule related to the usages of dangerous-triggers have been disabled
to avoid false-positives. Explanations about the usage of
`pull_request_target` and expectations around its usage can be found in
`.github/workflows/README.md`.
[0]: https://woodruffw.github.io/zizmor/
Co-authored-by: Thomas Gerbet <thomas@gerbet.me>
In a recent change, the path matching was simplified in maintainers.nix.
This revealed a pre-existing logic bug: Packages without `meta.position`
would get an empty string as their file name. The change would then
cause this empty string to always be matched, which lead to maintainer
pings for these packages in seemingly random PRs, when some of their
dependencies were changed.
We should never try to ping maintainers through package aliases, this
can only lead to errors. One example case is, where an attribute is a
throw alias, but then re-introduced in a PR. This would trigger the
throw. By disabling aliases, we can fallback gracefully.
Over the last couple of months we have been migrating a lot of the old
bash code to JavaScript, which is supported in GitHub Actions via
`actions/github-script`. This change documents a "manual ratchet check"
for this migration - new code should only be introduced as JavaScript
and not as Bash. This will help us to eventually succeed with the
migration and ensure quality and maintainability.
We are migrating to JavaScript, because:
1. Using JavaScript is GitHub's [recommendation] against injection attacks.
Using `actions/github-script` has first-class support for the event
context and does not require to resort back to environment variables in
most cases. When environment variables need to be used, these are
accessed via `process.env`, without a risk for accidental injections.
Using `actions/github-script` is also recommended in a recent
[survey] of open source supply chain compromises:
> Finally, since two out of three compromises were due to shell injection,
> it might be safer to use a proper programming language, like JavaScript
> with actions/github-script, or any other language accessing the context
> via environment variables instead of YAML interpolation.
2. Handling even environment variables in Bash safely is almost
impossible. For example arithmetic expressions cause arbitrary code
execution vulnerabilities. While a lot of contributors are somehwat
familiar writing Bash code for builders, writing *safe* Bash code for
CI is a very different matter. Few people, if any, know how to do
this.
3. GitHub Action's security model is quite unintuitive and even if some
code runs with trusted inputs today, it may later be used in a more
exposed context. Instead of making judgement calls about language
choice case by case, a clear policy helps writing things defensively
from the beginning.
4. We have developed a framework around our github-script based tools in
`ci/github-script`. This provides a local `nix-shell` environment
with the right dependencies and a local runner for these scripts for
quick testing, debugging and development. No matter, whether you're
developing a new feature, fixing bugs or reviewing a PR - this allows
much quicker verification of the scripts, *without* running
everything in a fork or test organization.
5. This framework also provides helpers for challenges that come up with
GHA. One example is rate-limiting, where we have a helper script that
will handle all rate-limiting needs for us, preventing us from
running out of API calls and thus breaking CI entirely. We can only
use these tools consistently, if we consistently use JavaScript code.
6. Using JavaScript allows us to handle JSON natively. Using
`octokit/rest.js` provides first-class integration with GitHub's API.
Together, this makes these scripts much more maintainable than
resorting to `gh` and `jq`.
[recommendation]: https://docs.github.com/en/actions/reference/security/secure-use#use-an-action-instead-of-an-inline-script
[survey]: https://words.filippo.io/compromise-survey/
Some PRs are empty on purpose, for example the yearly notification about
the election for voters. We should not close these because the merge
commit is empty - only if there was a change intended, but the merge
commit *becomes* empty, we should act.
The parse check runs multiple `nix-instantiate` processes in parallel -
and they can error out with "SQLite database '...' is busy" while
setting up the state directories. This was observed once locally.
Initialising the store should fix this.
There is no point in running the much slower `parse-each` part for each
interpreter/version. The CI job is not meant as a development tool that
should report all parse errors at once, but as a confirmation that no
parse errors are present on *different interpreter versions*.
Once this test fails, Eval, nixpkgs-vet and treefmt will most likely
fail as well - with more information for multiple parse errors.
This is another attempt at fixing the annoying nixpkgs-vet errors in CI,
which just throw with `error: SQLite database '...' is busy`.
The assumption is that this happens while initially setting up the state
directories. nixpkgs-vet runs `nix-instantiate` on both the base and the
head commit and these two could interfere.
If the change of a PR has already been merged to the target branch
elsewhere, the PR will not be auto-closed by GitHub - and will still
show the same original diff. Still, the temporary merge commit is
actually empty. This causes all kinds of strange CI behavior, from not
showing rebuilds to not pinging maintainers.
We check the merge commit during labeling anyway, to see whether a merge
conflict is present. It's easy to just look a the number of affected
files in this merge commit - and if there are none, we can just
automatically close the PR as no longer relevant.
This allows requesting reviewers for pure refactor PRs, which don't
cause a rebuild of the package. This is only possible for by-name,
because only here the package names can be inferred from the filenames.
This adds support to ping maintainers when arbitrary files in by-name
are changed, as long as they still cause a rebuild. For example, this is
the case when changing .json files with version metadata. These were
previously not detected as belonging to the package, and didn't cause
maintainer pings.
The only reason for the additional `lib.hasSuffix` check was, that the
`lib.removePrefix` was broken - it was never adjusted when porting this
from ofborg, so the relative path was wrong and no prefix ever removed,
since no packages are in `ci/`.
This additionally strips the leading `/`, so that `relevantFilenames`
will then have paths starting with `pkgs/...`, similar to how git
reports those paths in the `changedpathsjson` file. This allows simple
equality comparison.
