Skip to content

ci: init get-merge-commit workflow #361494

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

Merged
infinisil merged 3 commits into from
Dec 4, 2024
Merged

ci: init get-merge-commit workflow #361494

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
b5a6aeb
ci: init get-merge-commit workflow
JohnRTitor Dec 3, 2024
5ddb63f
ci/eval: use the get-merge-commit workflow
JohnRTitor Dec 3, 2024
52acf63
ci/nixpkgs-vet: use the get-merge-commit workflow
JohnRTitor Dec 3, 2024
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
46 changes: 11 additions & 35 deletions .github/workflows/eval.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,72 +16,48 @@ permissions:
contents: read

jobs:
get-merge-commit:
uses: ./.github/workflows/get-merge-commit.yml

attrs:
name: Attributes
runs-on: ubuntu-latest
needs: get-merge-commit
outputs:
mergedSha: ${{ steps.merged.outputs.mergedSha }}
mergedSha: ${{ needs.get-merge-commit.outputs.mergedSha }}
baseSha: ${{ steps.baseSha.outputs.baseSha }}
systems: ${{ steps.systems.outputs.systems }}
steps:
# Important: Because of `pull_request_target`, this doesn't check out the PR,
# but rather the base branch of the PR, which is needed so we don't run untrusted code
- name: Check out the ci directory of the base branch
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
path: base
sparse-checkout: ci
- name: Check if the PR can be merged and get the test merge commit
id: merged
env:
GH_TOKEN: ${{ github.token }}
GH_EVENT: ${{ github.event_name }}
run: |
case "$GH_EVENT" in
push)
echo "mergedSha=${{ github.sha }}" >> "$GITHUB_OUTPUT"
;;
pull_request_target)
if mergedSha=$(base/ci/get-merge-commit.sh ${{ github.repository }} ${{ github.event.number }}); then
echo "Checking the merge commit $mergedSha"
echo "mergedSha=$mergedSha" >> "$GITHUB_OUTPUT"
else
# Skipping so that no notifications are sent
echo "Skipping the rest..."
fi
;;
esac
rm -rf base
- name: Check out the PR at the test merge commit
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
# Add this to _all_ subsequent steps to skip them
if: steps.merged.outputs.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
with:
ref: ${{ steps.merged.outputs.mergedSha }}
ref: ${{ needs.get-merge-commit.outputs.mergedSha }}
fetch-depth: 2
path: nixpkgs

- name: Determine base commit
if: github.event_name == 'pull_request_target' && steps.merged.outputs.mergedSha
if: github.event_name == 'pull_request_target' && needs.get-merge-commit.outputs.mergedSha
id: baseSha
run: |
baseSha=$(git -C nixpkgs rev-parse HEAD^1)
echo "baseSha=$baseSha" >> "$GITHUB_OUTPUT"

- name: Install Nix
uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30
if: steps.merged.outputs.mergedSha
if: needs.get-merge-commit.outputs.mergedSha

- name: Evaluate the list of all attributes and get the systems matrix
id: systems
if: steps.merged.outputs.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
run: |
nix-build nixpkgs/ci -A eval.attrpathsSuperset
echo "systems=$(<result/systems.json)" >> "$GITHUB_OUTPUT"

- name: Upload the list of all attributes
uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3
if: steps.merged.outputs.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
with:
name: paths
path: result/*
Expand Down
43 changes: 43 additions & 0 deletions .github/workflows/get-merge-commit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Get merge commit

on:
workflow_call:
outputs:
mergedSha:
description: "The merge commit SHA"
value: ${{ jobs.resolve-merge-commit.outputs.mergedSha }}

# We need a token to query the API, but it doesn't need any special permissions
permissions: {}

jobs:
resolve-merge-commit:
runs-on: ubuntu-latest
outputs:
mergedSha: ${{ steps.merged.outputs.mergedSha }}
steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
path: base
sparse-checkout: ci
- name: Check if the PR can be merged and get the test merge commit
id: merged
env:
GH_TOKEN: ${{ github.token }}
Copy link
Member

@infinisil infinisil Dec 4, 2024

Choose a reason for hiding this comment

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

Security concern: There's no permissions specified here, so this might use a default token with a lot of permissions and not the one from the parent workflow with little permissions. Should definitely specify permissions here to limit it to only what's necessary (which requires figuring out which one this uses). See also https://docs.github.com/en/actions/sharing-automations/reusing-workflows#using-inputs-and-secrets-in-a-reusable-workflow

Copy link
Member Author

@JohnRTitor JohnRTitor Dec 4, 2024

Choose a reason for hiding this comment

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

Decided to go with:

# We need a token to query the API, but it doesn't need any special permissions
permissions: {}

GH_EVENT: ${{ github.event_name }}
run: |
case "$GH_EVENT" in
push)
echo "mergedSha=${{ github.sha }}" >> "$GITHUB_OUTPUT"
;;
pull_request_target)
if mergedSha=$(base/ci/get-merge-commit.sh ${{ github.repository }} ${{ github.event.number }}); then
echo "Checking the merge commit $mergedSha"
echo "mergedSha=$mergedSha" >> "$GITHUB_OUTPUT"
else
# Skipping so that no notifications are sent
echo "Skipping the rest..."
fi
;;
esac
rm -rf base
32 changes: 10 additions & 22 deletions .github/workflows/nixpkgs-vet.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,46 +19,34 @@ permissions: {}
# There is a feature request for suppressing notifications on concurrency-canceled runs: https://github.com/orgs/community/discussions/13015

jobs:
get-merge-commit:
uses: ./.github/workflows/get-merge-commit.yml
Copy link
Member

@infinisil infinisil Dec 3, 2024

Choose a reason for hiding this comment

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

Do we need a separate job for this? It seems like this can just be a separate step in the first job instead. This way we wouldn't spam the list of CI checks with many of these

Copy link
Member Author

@JohnRTitor JohnRTitor Dec 3, 2024

Choose a reason for hiding this comment

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

I just followed https://docs.github.com/en/actions/sharing-automations/reusing-workflows#calling-a-reusable-workflow

But let's try to implement this.

Copy link
Member Author

@JohnRTitor JohnRTitor Dec 3, 2024

Choose a reason for hiding this comment

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

@infinisil tried d3446ef, but seems to fail https://github.com/JohnRTitor/nixpkgs/actions/runs/12145724531/job/33868011344?pr=2

Could you help?

Copy link
Contributor

@adamcstephens adamcstephens Dec 3, 2024

Choose a reason for hiding this comment

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

You'll need to move it after the checkout, as the message suggests. Otherwise it hasn't cloned the repo.

Copy link
Member Author

@JohnRTitor JohnRTitor Dec 3, 2024

Choose a reason for hiding this comment

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

Well we are doing it in the get-merge-commit workflow. https://github.com/JohnRTitor/nixpkgs/blob/d3446ef3d7072be54facc926568a02830c484823/.github/workflows/get-merge-commit.yml#L16

It works if I just drop d3446ef

Copy link
Member

@infinisil infinisil Dec 3, 2024

Choose a reason for hiding this comment

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

On Matrix (since there were problems with GitHub) @JohnRTitor shared this:

Unlike when you are using actions within a workflow, you call reusable workflows directly within a job, and not from within job steps.

Copy link
Member

@infinisil infinisil Dec 3, 2024

Choose a reason for hiding this comment

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

I just found out about "Composite actions" though, which sound like a better fit now! https://docs.github.com/en/actions/sharing-automations/avoiding-duplication#key-differences-between-reusable-workflows-and-composite-actions

Copy link
Member

@infinisil infinisil Dec 3, 2024

Choose a reason for hiding this comment

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

Looks like that works best with a separate repository though, which we could totally do by requesting it, won't be as local anymore though.

Copy link
Contributor

@azuwis azuwis Dec 4, 2024

Choose a reason for hiding this comment

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

Composite actions does not need a separate repo, I use them in both in-repo and out-of-repo.

In-repo example https://github.com/azuwis/nix-config/blob/dd9650cce55ca08d1304fb49e850a4ddaeb42f8d/.github/workflows/package.yml#L53

We can totally put them in the ci/ dir.


check:
name: nixpkgs-vet
# This needs to be x86_64-linux, because we depend on the tooling being pre-built in the GitHub releases.
runs-on: ubuntu-latest
# This should take 1 minute at most, but let's be generous. The default of 6 hours is definitely too long.
timeout-minutes: 10
needs: get-merge-commit
steps:
# This checks out the base branch because of pull_request_target
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
path: base
sparse-checkout: ci
- name: Resolving the merge commit
env:
GH_TOKEN: ${{ github.token }}
run: |
if mergedSha=$(base/ci/get-merge-commit.sh ${{ github.repository }} ${{ github.event.number }}); then
echo "Checking the merge commit $mergedSha"
echo "mergedSha=$mergedSha" >> "$GITHUB_ENV"
else
echo "Skipping the rest..."
fi
rm -rf base
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
if: env.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
with:
# pull_request_target checks out the base branch by default
ref: ${{ env.mergedSha }}
ref: ${{ needs.get-merge-commit.outputs.mergedSha }}
# Fetches the merge commit and its parents
fetch-depth: 2
- name: Checking out base branch
if: env.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
run: |
base=$(mktemp -d)
git worktree add "$base" "$(git rev-parse HEAD^1)"
echo "base=$base" >> "$GITHUB_ENV"
- uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30
if: env.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
- name: Fetching the pinned tool
if: env.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
# Update the pinned version using ci/nixpkgs-vet/update-pinned-tool.sh
run: |
# The pinned version of the tooling to use.
Expand All @@ -71,7 +59,7 @@ jobs:
# Adds a result symlink as a GC root.
nix-store --realise "$toolPath" --add-root result
- name: Running nixpkgs-vet
if: env.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
env:
# Force terminal colors to be enabled. The library that `nixpkgs-vet` uses respects https://bixense.com/clicolors/
CLICOLOR_FORCE: 1
Expand Down
29 changes: 8 additions & 21 deletions ci/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ Exit codes:

### Usage

This script can be used in GitHub Actions workflows as follows:
This script is implemented as a reusable GitHub Actions workflow, and can be used as follows:

```yaml
on: pull_request_target
Expand All @@ -67,32 +67,19 @@ on: pull_request_target
permissions: {}

jobs:
get-merge-commit:
# use the relative path of the get-merge-commit workflow yaml here
uses: ./.github/workflows/get-merge-commit.yml

build:
name: Build
runs-on: ubuntu-latest
needs: get-merge-commit
steps:
# Important: Because of `pull_request_target`, this doesn't check out the PR,
# but rather the base branch of the PR, which is needed so we don't run untrusted code
- uses: actions/checkout@<VERSION>
with:
path: base
sparse-checkout: ci
- name: Resolving the merge commit
env:
GH_TOKEN: ${{ github.token }}
run: |
if mergedSha=$(base/ci/get-merge-commit.sh ${{ github.repository }} ${{ github.event.number }}); then
echo "Checking the merge commit $mergedSha"
echo "mergedSha=$mergedSha" >> "$GITHUB_ENV"
else
# Skipping so that no notifications are sent
echo "Skipping the rest..."
fi
rm -rf base
- uses: actions/checkout@<VERSION>
# Add this to _all_ subsequent steps to skip them
if: env.mergedSha
if: needs.get-merge-commit.outputs.mergedSha
with:
ref: ${{ env.mergedSha }}
ref: ${{ needs.get-merge-commit.outputs.mergedSha }}
- ...
```