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

Percy workflow selectivity improvements #5108

Merged

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Jun 4, 2024

Done

  • Percy tests no longer run against draft PRs.
  • Percy tests no longer run against PRs that don't modify examples files or scss files.
  • The above can be circumvented by applying the Review: Percy needed label. All commits to PRs with the Percy review needed label will be tested.
    • Note that name of this label is not stored in code; rather we will use a Github Repo variable to define it in one place. It must match the name of a label on our repo that we want to use for requesting Percy testing. I have defined it as "Review: Percy needed".
  • Some minor tweaks to the data passing between privileged/nonprivileged workflows to stop unnecessarily compressing the artifact twice.
  • Some preparation work for WD-11227: we are now passing PR numbers into Percy builds, which should enable the Percy integration to create status checks on our PRs once that is set up by IS.
  • The "Percy Prepare" workflow was made into a reusable workflow so that it can be called by both the labeling workflow and the on-push workflow without reusing code.

Closes WD-11405

QA

  • Review comments & workflows on testing PR.
    • Note that in my fork, the Percy label is "Percy test requested" rather than "Review: Percy needed". Do not worry about this; the name of the label is fetched from a repo variable and in the Canonical repo it will listen for "Review: Percy needed".

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:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

@jmuzina jmuzina self-assigned this Jun 4, 2024
@webteam-app
Copy link

@jmuzina jmuzina force-pushed the percy-workflow-selectivity-improvements branch 2 times, most recently from b89b742 to 522e001 Compare June 5, 2024 14:51
@jmuzina jmuzina marked this pull request as ready for review June 5, 2024 15:05
@jmuzina jmuzina changed the title WIP: Percy workflow selectivity improvements Percy workflow selectivity improvements Jun 5, 2024
@bartaz
Copy link
Member

bartaz commented Jun 11, 2024

The name of the workflow and job show up in the listing, it would be nice to make them a bit less detailed. We can keep details in comments in the files if needed.

image

@jmuzina jmuzina force-pushed the percy-workflow-selectivity-improvements branch from 522e001 to 9f7070b Compare June 11, 2024 13:56
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

Small naming adjustments please.

@@ -0,0 +1,22 @@
# This workflow ensures Percy is executed against PRs with the Percy required label, regardless of which paths were changed
name: "Percy [labeled] / Prepare build"
Copy link
Member

Choose a reason for hiding this comment

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

That's weird, I thought I commented on this already.

I meant for the workflow to be Percy [labeled], and for the job to be Prepare build, as now we still have a long and unnecessarily detailed "Copy changed files to GHA artifact" as status name on PR.

Suggested change
name: "Percy [labeled] / Prepare build"
name: "Percy [labeled]"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I misunderstood your original comment and named the entire workflow that; this has been revised!

.github/workflows/pr-percy-prepare-label.yml Show resolved Hide resolved
Copy link
Contributor

@pastelcyborg pastelcyborg left a comment

Choose a reason for hiding this comment

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

High-level comment: I tend to prefer when workflows/actions are divided into their own directories, as done here:

https://github.com/pastelcyborg/vanilla-web-scanner/tree/main/.github

The primary reason for this (atop the structure being a bit clearer) is that it prevents us from having multiple files in the repo called "action.yml," assuming we add other Actions in the future. Every file would then go in one of these two parent directories. Open to other thoughts, though.

@jmuzina jmuzina force-pushed the percy-workflow-selectivity-improvements branch from 8a256e5 to 257ae7e Compare June 17, 2024 13:03
…tifact compression; Note manual status check application as to-be-removed; run percy only when certain files are changed
@jmuzina jmuzina force-pushed the percy-workflow-selectivity-improvements branch from 257ae7e to 131845f Compare June 17, 2024 13:06
@jmuzina
Copy link
Member Author

jmuzina commented Jun 17, 2024

Making that naming change to the workflows seems to have upset the workflow run trigger... investigating, standby

…tifact compression; Note manual status check application as to-be-removed; run percy only when certain files are changed
@jmuzina
Copy link
Member Author

jmuzina commented Jun 17, 2024

High-level comment: I tend to prefer when workflows/actions are divided into their own directories, as done here:

https://github.com/pastelcyborg/vanilla-web-scanner/tree/main/.github

The primary reason for this (atop the structure being a bit clearer) is that it prevents us from having multiple files in the repo called "action.yml," assuming we add other Actions in the future. Every file would then go in one of these two parent directories. Open to other thoughts, though.

@pastelcyborg
I like this approach. If I understood it correctly, I've made the change to this PR to implement it (moved percy-snapshot action into .github/actions)

@@ -0,0 +1,48 @@
name: "Prepare Percy build"
Copy link
Member

Choose a reason for hiding this comment

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

Is it a workflow, or an action?

It doesn't seem to be triggered via PR events, but reused in workflows as if it was an action, right?

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have a comment on top what the workflow does, and when it's supposed to be triggered, to have a better understanding of the flow.

As now it's getting complicated with multiple conditional workflows.

Copy link
Member Author

@jmuzina jmuzina Jun 18, 2024

Choose a reason for hiding this comment

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

@bartaz

Is it a workflow, or an action?

It is a reusable workflow.

It doesn't seem to be triggered via PR events, but reused in workflows as if it was an action, right?

Yes, it's triggered by the snapshot & label workflows - the workflow runs at job level whereas the action runs at the step level

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be good to have a comment on top what the workflow does, and when it's supposed to be triggered, to have a better understanding of the flow.

As now it's getting complicated with multiple conditional workflows.

Agreed, I'll add a note to the workflow momentarily

@jmuzina jmuzina force-pushed the percy-workflow-selectivity-improvements branch from e05e317 to 9e60c58 Compare June 18, 2024 14:19
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, let's see how that works in practice!

@bartaz bartaz merged commit d1a0dbc into canonical:main Jun 18, 2024
7 checks passed
@jmuzina jmuzina deleted the percy-workflow-selectivity-improvements branch June 26, 2024 17:50
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