-
-
Notifications
You must be signed in to change notification settings - Fork 19.8k
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 }} | ||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On Matrix (since there were problems with GitHub) @JohnRTitor shared this:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| 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. | ||
|
|
@@ -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 | ||
|
|
||
There was a problem hiding this comment.
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
permissionsspecified 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 specifypermissionshere 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-workflowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to go with: