ci(GHA): Improve handling of PRs from forks #3228
Conversation
jpveooys
changed the title
jpveooys
force-pushed
the
ci/handle-forks
branch
4 times, most recently
from
d24e28b to
ac7e8b8
Compare
jpveooys
added
Type: Devops
| with: | ||
| path: '**/node_modules' | ||
| key: ${{ runner.os }}-modules-${{ secrets.CACHE_VERSION }}-${{ hashFiles('**/yarn.lock') }} | ||
| key: ${{ runner.os }}-node-modules-${{ hashFiles('**/yarn.lock') }} |
There was a problem hiding this comment.
What was the reason for that CACHE_VERSION secret?
Was it there as a bit of a hack to manually invalidate the cache?
There was a problem hiding this comment.
Yes that's probably the main reason. The other possibility is changing it after e.g. a Node.js upgrade.
We could keep it in but it won't work for forks or Dependabot. We could also just append e.g. v1 to the cache key and change it in the workflow if a problem came up.
It seems like some APIs to manage the cache may be coming later in the year as well: actions/cache#2 (comment)
| uses: actions/github-script@v6 | ||
| with: | ||
| script: | | ||
| const allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({ |
There was a problem hiding this comment.
Is it worth putting this in an external seperate script and referencing it?
There was a problem hiding this comment.
Hmm, yes, it was a bit annoying editing that here. It looks like it's possible: https://github.com/actions/github-script#run-a-separate-file
Will give it a go 👍
There was a problem hiding this comment.
Got this working – have pushed a fixup.
These aren't needed and cause inconsistent behaviour for PRs from forks and Dependabot PRs.
Running a SonarCloud scan fails on PRs from forks as it requires secrets. Therefore this skips it for forks (in addition to Dependabot updates). Running a SonarCloud scan requires secrets to be available and the code being scanned to be checked out, which is not generally recommended for untrusted code. Therefore there is no nice way to make it work. See also https://portal.productboard.com/sonarsource/1-sonarcloud/c/50-sonarcloud-analyzes-external-pull-request
This adds an additional security using GitHub's CodeQL code scanning feaure. This is useful as SonarCloud doesn't work for forks while code scanning does.
This changes how Chromatic is invoked in order to make it compatible with forks. It is now triggered using the workflow_run event using a pre-built Storybook, so that it can be run in a trusted environment without exposing secrets to untrusted code. This also fixes the reporting of Chromatic status for normal PRs. Finally, this change also fixes running of Chromatic for Dependabot PRs as Chromatic was being run against master in this case.
jpveooys
force-pushed
the
ci/handle-forks
branch
from
f698f7d to
6bd055c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Related issue
Resolves #2737
Overview
This:
Reason
To make sure PRs can be made from forks without friction.
Work carried out
Developer notes
See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ for some detail on the relevant considerations here.
This has been tested on a fork:
Chromatic has been refactored to follow a similar pattern as in the blog post linked above (for all builds). This means Storybook is built in the main PR build and uploaded as an artefact, and then this is downloaded in an
on: workflow_runworkflow where master is checked out instead of the PR branch and secrets are available.This also brings back the Chromatic status on PRs:
These won't appear until the PR is merged (as
.github/workflows/post_build_and_test.ymlneeds to be on master for it to be used).Branch protection rules will need to be updated after this has been merged to require UI Tests instead of Test_visual_regression.
There's no nice way to make SonarCloud work with forks, because you have to have the secret available and the code checked out at the same time (this could be done, but it's not generally recommended). They have support for forks on their product board: https://portal.productboard.com/sonarsource/1-sonarcloud/c/50-sonarcloud-analyzes-external-pull-request
I added GitHub code scanning to make sure PRs from forks do go through a code security scan.