-
Notifications
You must be signed in to change notification settings - Fork 171
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
Percy workflow selectivity improvements #5108
Conversation
b89b742
to
522e001
Compare
522e001
to
9f7070b
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.
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" |
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.
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.
name: "Percy [labeled] / Prepare build" | |
name: "Percy [labeled]" |
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.
Ah I misunderstood your original comment and named the entire workflow that; this has been revised!
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.
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.
8a256e5
to
257ae7e
Compare
…tifact compression; Note manual status check application as to-be-removed; run percy only when certain files are changed
257ae7e
to
131845f
Compare
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
@pastelcyborg |
@@ -0,0 +1,48 @@ | |||
name: "Prepare Percy build" |
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 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?
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.
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.
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 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
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.
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
e05e317
to
9e60c58
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.
LGTM, let's see how that works in practice!
Done
Review: Percy needed
label. All commits to PRs with the Percy review needed label will be tested.Closes WD-11405
QA
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: