Skip to content

ci: init pr.yml and merge-queue.yml workflows #3628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

ci: init pr.yml and merge-queue.yml workflows #3628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d92c17
ci/lint: explicitly define permissions
MattSturgeon Aug 22, 2025
d1a00bf
ci/lint: explicitly define substituters
MattSturgeon Aug 22, 2025
2eab9f2
ci: init `pr.yml` and `merge-queue.yml` workflows
MattSturgeon Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,30 @@
name: lint

on:
pull_request:
paths-ignore:
- "**.md"
- "**.svg"
- ".gitignore"
- "LICENSE"
- "flake.lock"
workflow_call:
inputs:
ref:
required: true
type: string

permissions:
contents: read

jobs:
treefmt:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4
with:
ref: ${{ inputs.ref }}

- name: Install Nix
uses: cachix/install-nix-action@v31
with:
extra_nix_config: |
extra-substituters = https://nix-community.cachix.org
extra-trusted-public-keys = nix-community.cachix.org-1:mB9FSh9qf2dCimDSUo8Zy7bkq5CX+/rkCWyvRCYg3Fs=

- name: Run treefmt check
run: nix build .#checks.x86_64-linux.treefmt --accept-flake-config
run: nix build .#checks.x86_64-linux.treefmt
39 changes: 39 additions & 0 deletions .github/workflows/merge-queue.yml
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optionally, we could combine pr.yml and merge-queue.yml into a single workflow.

We'd need to do things like ${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha }} and it'd be more difficult to specify different checks for PRs vs the Queue.

But it would remove a lot of duplication.

Thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes a lot of sense to re-use the same file when you can. It looks like this should be easily possible here.

You could even change the run-name so you don't get "PR" for both.

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: Merge Queue

on:
merge_group:

permissions: {}

jobs:
lint:
uses: ./.github/workflows/lint.yml
with:
ref: ${{ github.event.merge_group.head_sha }}

# This job posts the "Required Status Checks" to satisfy our ruleset.
required-checks:
# It "needs" all the jobs that should block the Merge Queue.
# Modify this list to add or remove jobs from required status checks.
needs:
- lint

name: Required checks
runs-on: ubuntu-24.04-arm
permissions:
statuses: write
steps:
- uses: actions/github-script@v7
with:
script: |
const { serverUrl, repo, runId, payload } = context
await github.rest.repos.createCommitStatus({
...repo,
sha: payload.merge_group.head_sha,
target_url: `${serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${runId}`,
// WARNING:
// Do NOT change the context name or it will not match the ruleset.
// This would prevent all PRs from merging.
context: 'PR checks successful',
state: 'success',
})
43 changes: 43 additions & 0 deletions .github/workflows/pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: PR

on:
pull_request_target:
Comment on lines +3 to +4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside of going with the pull_request_target event: Changes to the workflow file now won't be tested in the respective PR anymore. Now you need to start looking into whether to add the pull_request trigger as well.

Overall, it might be simpler to use the non-javascript-status approach here with the old "skipped vs failed job" approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside of going with the pull_request_target event: Changes to the workflow file now won't be tested in the respective PR anymore. Now you need to start looking into whether to add the pull_request trigger as well.

Yeah, this is frustrating. Maybe I should consider adding a "testing" on: pull_request trigger to this PR so that it's done.

Overall, it might be simpler to use the non-javascript-status approach here with the old "skipped vs failed job" approach.

I'm of two minds here:

  1. On the one hand pull_request is much simpler.
  2. On the other hand, if we want to do anything that needs secrets or write permissions then we'll eventually need to switch.
    If so, we may as well do it now and get access to the better "required check" implementation.

I like the idea of the better impl because we can choose a saner name for the required check from the start. It's also less confusing for contributors if we have a green check, rather than a grey one.


concurrency:
group: pr-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }}
cancel-in-progress: true

permissions: {}

jobs:
lint:
uses: ./.github/workflows/lint.yml
with:
ref: ${{ github.event.pull_request.head.sha }}

# This job posts the "Required Status Checks" to satisfy our ruleset.
required-checks:
# It "needs" all the jobs that should block merging a PR.
# Modify this list to add or remove jobs from required status checks.
needs:
- lint

name: Required checks
runs-on: ubuntu-24.04-arm
permissions:
statuses: write
steps:
- uses: actions/github-script@v7
with:
script: |
const { serverUrl, repo, runId, payload } = context
await github.rest.repos.createCommitStatus({
...repo,
sha: payload.pull_request.head.sha,
target_url: `${serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${runId}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you leave out the ?pr= suffix out here on purpose? This is a bit annoying when clicking the link, because the "back" link from the summary page won't go to the PR, but to the actions tab.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I copied this from your merge-group.yml without spotting that pr.yml had a different URL.

// WARNING:
// Do NOT change the context name or it will not match the ruleset.
// This would prevent all PRs from merging.
context: 'PR checks successful',
state: 'success',
})