-
Notifications
You must be signed in to change notification settings - Fork 166
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
Migrate Percy from CircleCI to Github Actions #5089
Conversation
a92ebf1
to
26ffa2c
Compare
26ffa2c
to
9e31ec9
Compare
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.
I left couple comments/questions, but overall impressive work!
@@ -0,0 +1,42 @@ | |||
name: "Upload artifacts for Percy screenshots" |
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.
Maybe just for simplicity (and potential future changes of what is done internally) we could rename this step to "Prepare Percy build" pr-percy-prepage.yml
, and the other one "Percy snapshots" pr-percy-snapshots.yml
.
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.
Makes sense, renaming now
steps: | ||
- name: Verify that triggering workflow run passed | ||
run: | | ||
if [[ "${{ github.event.workflow_run.conclusion }}" != "success" ]]; then |
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.
Is there any reason for this conditional check to be in steps, rather than on the job level?
jobs:
on-success:
runs-on: ubuntu-latest
if: ${{ github.event.workflow_run.conclusion == 'success' }}
steps:
- run: echo 'The triggering workflow passed'
on-failure:
runs-on: ubuntu-latest
if: ${{ github.event.workflow_run.conclusion == 'failure' }}
steps:
- run: echo 'The triggering workflow failed'
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.
Good call, moving that to a job level check
|
||
- name: Replace SCM files with artifact files | ||
run: | | ||
set -e |
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.
Nice touch, didn't know this before.
# This identifies the diff in the percy diffs list | ||
PERCY_BRANCH: "pull/${{ steps.get_pr_data.outputs.pr }}" | ||
#PERCY_BRANCH: "${{ github.event.workflow_run.head_repository.full_name }}/${{ github.event.workflow_run.head_branch }}" | ||
#PERCY_PULL_REQUEST: ${{ steps.get_pr_data.outputs.pr }} |
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.
Are these intentionally commented out?
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.
@bartaz Yes, I ran into a bug while I was working on #5092 where I noticed that all Percy builds were becoming the baseline because they defaulted to being "main" branch without me specifying PERCY_BRANCH
.
This sets the diff branch name to a PR identifier, making sure that builds don't become the baseline unless they are approved.
The commented out code was doing the following:
PERCY_BRANCH
- I was trying to set the branch name to<fork_repo_username>/<fork_repo_name>/<fork_pr_head_branch_name>
in an attempt to make the diff branches unique. However I simplified this to thepull/
format as that is what is currently used in the project.PERCY_PULL_REQUEST
- links the diff to a specific PR by number. I think this requires the Percy->GH integration to be enabled, but I don't have permissions to enable it in the Percy portal.
See more about Percy CLI environment vars here.
I'll remove these in a commit shortly.
run: | | ||
set -e | ||
percy_output=$(yarn percy) | ||
build_link=$(echo "$percy_output" | sed -n 's/.*Finalized build #.*: \(https:\/\/percy.io\/[^ ]*\).*/\1/p') |
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.
There is a lot of happening here in a bit cryptic way. Could be worth to add some comments to explain what data of Percy build is being extracted and why.
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.
Added some new comments for this section
* Revert "Revert "break percy diff intentionally"" This reverts commit 20db9ce. * Revert "Revert "Revert "break percy diff intentionally""" This reverts commit 4e3ef26. * test for revisions to canonical/vanilla-framework#5089 * version bump for test pr * bump for test action * dedupe * test pr test pr * intentional fail * Revert "intentional fail" This reverts commit 20a3fb6.
# https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#create-a-check-run | ||
apply_pr_check: | ||
name: Apply PR check | ||
needs: upload |
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.
This will only get triggered once the upload is completed. Could we have some kind of "in progress" status as well to see that "Percy build" is running (and not only when it succeeds)?
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.
Great work, thanks. Let's try it out.
Done
Switches Percy integration from CircleCI to Github Actions
Closes WD-11242
The problem
We are currently using CircleCI to execute Percy visual testing, because executing
percy snapshot
requires an environment variablePERCY_TOKEN
to be set. However, repo secrets are not exposed to fork contributors for security reasons. CircleCI is used as a workaround for this, as it knows the Percy secret and is triggered on PR.However, this disjoint approach causes issues with linking Percy build state to the PR. The PR CircleCI check success only means that the snapshots were uploaded to Percy; it does not necessarily mean that Percy diffs were approved.
The solution
This PR is the first step towards solving the above problem. This migrates us to using GHA to start Percy, allowing us to report Percy diff state in the PR at a later date and also run Percy more selectively (such as only when certain files change) at a later date.
The approach
Two workflows are used:
pr-percy-copy-diff-files
, andpr-percy-create-screenshots
. This follows this approach by GH security team.Copy diff files
Runs in an unprivileged context on
pull_request
tomain
. It checks out thetemplates/docs/examples/
andscss/
directories, notes the PR # & commit signature (as suggested by the GH security article above), and uploads it as an artifact to Github Actions associated with the workflow run.Create screenshots
Runs in a privileged context on
workflow_run
trigger. Runs after completion of the copy diff files workflow. It downloads the artifacts from the previous workflow and replaces the matching files inmain
with the files from the artifacts. It then starts a local Flask server and uploads screenshots to Percy.Note that for this workflow to run,
pr-percy-create-screenshots.yml
must be in the default branch of the target repo (canonical/vanilla-framework
) at the time of the first workflow's completion. Thus, it will not run on this repo until this PR is merged and a second PR is raised to test its functionality.QA
As mentioned above, screenshots will not be created by this PR as
pr-percy-create-snapshots.yml
is not yet in our main branch. However, the PR workflow will run and upload diff artifacts based on this PR commit. The following should occur after this PR is raised:web-artifact-{workflow_run_id}
. Opening this artifact will reveal abuild.tar.gz
file containing:templates/docs/examples/
folder from the PR code)scss/
folder from the PR code)https://github.com/canonical/vanila-framework/pull/{pr_number}
)To fully test this workflow in a fork PR context, a separate repository was created. An example workflow is shown:
Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots
Upload & Snapshot status checks on PR:
Percy diff:
Upload completed status check on PR with Percy build details: