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

feat: Migrate Percy from CircleCI to Github Actions #1097

Closed
wants to merge 1 commit into from

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Jun 14, 2024

Done

QA

QA steps

  • Review test PR from my fork. Verify that it causes a Percy prepare workflow to trigger a Percy snapshot workflow, which creates a status check on the PR with a link to a Percy build showing changes from the PR.
  • Note that this PR deletes the CircleCI config so it will fail the CircleCI status check. Also, this PR will not cause a Percy build, as the CircleCI config has been removed and the Percy snapshots workflow (introduced by this PR) is not yet in the main branch. Review the changed files list to verify no visual changes are introduced. Once this PR is merged, we can disable the CircleCI integration.

Percy steps

  • No visual changes expected. This does not need to create a new react-components version as no components were updated.

Fixes

Fixes: WD-12705

@webteam-app
Copy link

@jmuzina jmuzina marked this pull request as ready for review June 14, 2024 18:55
.github/workflows/percy-snapshot/action.yml Show resolved Hide resolved

jobs:
snapshot:
name: Take Percy snapshots

Choose a reason for hiding this comment

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

Nit: since name is often used essentially as a variable, could we name them like variables? If an English description of the job is needed, the description key should probably be used

Copy link
Member Author

Choose a reason for hiding this comment

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

@pastelcyborg I'm not aware of a "description" key in the official GHA workflow syntax:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions

Would you still prefer names be changed?

Choose a reason for hiding this comment

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

Hmmmm, description is mentioned a number of times in the docs, but it's entirely possible I just don't know Actions/Workflows well enough to understand when it is and isn't available. I trust your judgment - if it's not available, the current names are fine

Copy link
Member Author

@jmuzina jmuzina Jun 27, 2024

Choose a reason for hiding this comment

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

As far as I'm aware, often the job key itself (from the YAML structure) is used as an identifier for use in other jobs, such as here:

https://github.com/canonical/vanilla-framework/blob/f82114a926a3a43dcae059a034e8e67a6eab962a/.github/workflows/pr-percy-snapshots.yml#L68

Then the job name is used for displaying in a "pretty" way on GH actions workflow pages, etc

@bartaz
Copy link
Member

bartaz commented Jun 28, 2024

I'm afraid this won't work as needed. In Vanilla copying the static SCSS and HTML was enough to get good approximation of the changes from PR (but we already seen in combined examples PR, that changes to example layout, or percy script were not taken into account, resulting in snapshots being different than after merging).

Here, it's even worse. While obviously src contains all our code of components, but the actual look of them depends on much more, for example all the dependencies: Vanilla itself, React, Storybook, etc.

Even in the simples and very common case of us updating Vanilla to new version, we will not be able to see relevant differences in Percy (because they are not part of the src).

While it would be tempting to copy over the package.json and build with all the new dependencies… this is exactly what GH is trying to secure us from, because this would allow someone to create a PR with unsafe scripts or dependencies in package.json that we would use.

I don't know if there is a good solution for that at this point. It seem to again boil down to what we can and can't do with Percy. We would either need a way to run Percy without secret (and have them use some other way to identifying the build is coming from valid source) or we would need to be able to snapshot to file system, and upload to Percy separately. But to my understanding from previous discussions with their support, neither is possible.

@jmuzina
Copy link
Member Author

jmuzina commented Jul 2, 2024

Blocked, see Jira comment:

For the time being, we will keep React-Components using CircleCI. GHA migration approach requires too many security compromises to work properly (we would need to allow the untrusted runner to commit arbitrary code to the trusted runner).

If we want this migration to work for React-Components, we may need another approach, such as taking screenshots with a tool like Selenium and uploading them to Percy with percy upload

An issue has been made on Percy’s CLI repo to add necessary functionality for taking Percy snapshots without exporting them. If this issue is implemented we can make a workflow where the untrusted runner takes snapshots and uploads them for the second runner to upload.

@jmuzina
Copy link
Member Author

jmuzina commented Sep 9, 2024

Closing this as we cannot migrate react components to use GHA for Percy without security concessions, and future architectural changes may make this effort moot anyway.

@jmuzina jmuzina closed this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants