ci: added treefmt workflow to gha and removed from buildbot #3611
Conversation
.github/workflows/treefmt.yml
Outdated
| - name: Install Nix | ||
| uses: cachix/install-nix-action@v31 | ||
| - name: Run treefmt check (nix flake check) | ||
| run: nix flake check --show-trace |
There was a problem hiding this comment.
This should build the specific derivation, rather than all checks.
E.g. nix build .#checks.x86_64-linix.treefmt
(It may not be named treefmt, I'm on my phone and can't verify)
There was a problem hiding this comment.
Oh! I didn't even know you can interact with checks like that. You learn something new everyday I guess. Fixed (it is called treefmt btw; https://github.com/numtide/treefmt-nix/blob/1298185c05a56bff66383a20be0b41a307f52228/flake-module.nix#L73-L75).
.github/workflows/treefmt.yml
Outdated
| paths-ignore: | ||
| - "**.md" | ||
| - "**.svg" | ||
| - "LICENSE" | ||
| - "flake.lock" |
There was a problem hiding this comment.
Ignoring files should be done by the treefmt configuration, not by GHA.
Although, if this is a true subset of our treefmt excludes then it is okay as a performance optimisation... Assuming we remember to keep it up to date if the treefmt config changes.
There was a problem hiding this comment.
It is indeed a subset. This was actually intended to ignore changes to the filetypes which are most frequently changed. Minimum here is flake.lock imo since that changes so much.
c4patino
force-pushed
the
ci/treefmt
branch
3 times, most recently
from
be9c745 to
a6bfab9
Compare
|
Hi @MattSturgeon is there anything else that you need on this? |
There was a problem hiding this comment.
This is complicated a little by merge queues and "required status checks", however I'm fine merging this initially without adding it as a "required" check.
#3611 can then be responsible for implementing the required checks for PRs and merge queues.
A few minor things to address or discuss below, then I think we can get this merged.
Thanks again!
| push: | ||
| branches: | ||
| - main | ||
| paths-ignore: | ||
| - "**.md" | ||
| - "**.svg" | ||
| - ".gitignore" | ||
| - "LICENSE" | ||
| - "flake.lock" |
There was a problem hiding this comment.
We don't really need to do this on push, since we require all changes go through a PR. But we should do it on merge_group so that it runs on the merge queue.
Additionally, we'll need to add this or a meta-job to our “required status checks”; cc @GaetanLepage
I'll try and look into adding a meta-job we can use for this, similar to the no pr failures job in nixpkgs.
There was a problem hiding this comment.
I'll handle merge queues and required checks in #3611
For now it is fine to just remove the on: push event.
| push: | |
| branches: | |
| - main | |
| paths-ignore: | |
| - "**.md" | |
| - "**.svg" | |
| - ".gitignore" | |
| - "LICENSE" | |
| - "flake.lock" |
| paths-ignore: | ||
| - "**.md" | ||
| - "**.svg" | ||
| - ".gitignore" | ||
| - "LICENSE" | ||
| - "flake.lock" |
There was a problem hiding this comment.
GHA recently added support for yaml anchors and references, so we can avoid needing to write this list twice.
The first instance should declare the anchor with &ignored, then the second instance can be replaced with a reference to it with *ignored
| paths-ignore: | |
| - "**.md" | |
| - "**.svg" | |
| - ".gitignore" | |
| - "LICENSE" | |
| - "flake.lock" | |
| paths-ignore: &ignored | |
| - "**.md" | |
| - "**.svg" | |
| - ".gitignore" | |
| - "LICENSE" | |
| - "flake.lock" |
Not relevant if we're removing the on: push event, though.
It'd also be nice to see a comment on this list, cross-referencing it with the treefmt-nix configuration so we remember to keep both in sync.
There was a problem hiding this comment.
GHA recently added support for yaml anchors and references, so we can avoid needing to write this list twice.
whoot? TIL. That's great to hear. Although from looking into it, it seems to be pretty limited :/.
There was a problem hiding this comment.
Support is tracked in actions/runner#1182
I assume most limitations come from workflows not supporting arbitrary unknown keys?
There was a problem hiding this comment.
Arbitrary keys + merge keys were what I used a lot for GitLab. Also GitLab can reference "templates" across files (different syntax, but same concepts). This would be so much better than re-usable workflows or custom actions for code reuse...
|
|
||
| jobs: | ||
| treefmt: | ||
| name: Check formatting with treefmt |
There was a problem hiding this comment.
This name shows up in the status check list as Workflow name / Job name; in this case Formatting / Check formatting with treefmt.
Bike-shedding the names doesn't block this PR, because we can change them at any time in other PRs. But my personal preference would be to be less verbose and avoid redundancy between workflow & job names.
How about Formatting / treefmt or Lint / formatting or Check / Formatting?
| - name: Install Nix | ||
| uses: cachix/install-nix-action@v31 | ||
| - name: Run treefmt check | ||
| run: nix build .#checks.x86_64-linux.treefmt |
There was a problem hiding this comment.
Running with --accept-flake-config will cause the flake config to be used. That's useful here because it configures the nix-community cachix.
Alternatively, we could use a cachix GHA step, like we do in some other workflows. That way we can also push builds to cachix.
Seemed like low hanging fruit in #3561