Skip to content
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

ci: fix fetch-depth and split workflows #1263

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

0xB10C
Copy link
Contributor

@0xB10C 0xB10C commented Jun 17, 2024

This fixes the fetch-depth issue (#1207 (comment)) and splits the CI into two workflows to work around the missing comment permissions from external forks:

  1. For PRs against main, generate the SHASUM summary comment and upload it as a "build-artifact" (GitHub forbids the usage of GITHUB_TOKEN to e.g. comment on the PR from external forks)
  2. When 1. succeeds, run a workflow that downloads the "build-artifact" and comments on the PR

For more context, see the discussion starting here: #1207 (comment)

With `${{ github.event.pull_request.commits }}` we don't fetch enough
commits to diff against the merge base. We require `pull_request.commits + 1`
commits to be able do that.

An alternative is to just fetch the complete git history as we don't
expect this repository to grow to a point where this has a
significant impact on CI performance.
commenting on a PR requires permissions which don't work from forks of
public repos. Circumvent this by using a second workflow which runs via
`on_workflow`, where the correct permissions are obtained.

Previously, the workflow would run the shasum-summary tool and store the
comment in a file. The file contents would then be used as the comment
body. Commenting however fails due to external forks not having the
permissions to comment.

Now, the workflow is split into two parts. First, the comment (and PR
number) is stored in file which is uploaded as build-artifact. Upon
completion, a second workflow starts, downloads the build-artifact, and
comments on the PR.
@0xB10C
Copy link
Contributor Author

0xB10C commented Jun 17, 2024

We've tested this here 0xB10C#14 and here 0xB10C#13. Note that the permission issue wasn't catched before as I've opened a PR against my own fork, which has different permissions from an external fork..

@0xB10C
Copy link
Contributor Author

0xB10C commented Jun 17, 2024

I got a notification for https://github.com/bitcoin-core/guix.sigs/actions/runs/9548877855 which states the following (but the action run does not show up anywhere on this PR...):

actions/upload-artifact@v4 is not allowed to be used in bitcoin-core/guix.sigs.
Actions in this workflow must be: within a repository owned by bitcoin-core or matching the following:
actions/checkout@v4, actions/github-script@v7.

Allowing select actions and reusable workflows to run explains how to enable this action.

Based on my understanding of the docs, going to https://github.com/organizations/bitcoin-core/settings/actions and adding the actions/upload-artifact@v4 should do the trick. I hope this doesn't disallow the actions/checkout@v4 and actions/github-script@v7 actions.

image

It makes sense to do this only when there is general agreement that we want to merge this PR.

@achow101
Copy link
Member

achow101 commented Jun 17, 2024

Concept ACK

Added actions/upload-artifact@v4 to the allowed workflows

@willcl-ark
Copy link
Member

ACK 16e1893 from me.

I know it doesn't seem like the most elegant solution process-wise, but it's kind of difficult to get GitHub to comment on PRs from forks without such a workaround (or other, even more ugly ones like it).

Hopefully this will reduce the frequency of us introducing bad sigs into this repo

@fanquake fanquake merged commit b0ed210 into bitcoin-core:main Jun 18, 2024
1 check passed
@achow101
Copy link
Member

@0xB10C Looks like it still has the permissions error: https://github.com/bitcoin-core/guix.sigs/actions/runs/9568585483

@0xB10C 0xB10C deleted the 2024-06-split-ci-workflows branch June 18, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants