-
Notifications
You must be signed in to change notification settings - Fork 3.3k
#64227 Increase the specificity of workflow path filtering #9457
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
base: trunk
Are you sure you want to change the base?
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This comment was marked as off-topic.
This comment was marked as off-topic.
peterwilsoncc
left a comment
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 a couple of notes inline.
| - 'src/**.php' | ||
| - 'tests/**.php' |
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.
Misses files in the root directory, currently the PHP sample configs, webpack.
Which files in particular are you trying to ignore here?
🔢 This applies to other changes too, so I won't repeat myself.
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.
For example, with this change the workflow wouldn't unnecessarily run when an e2e, performance, or QUnit test file is changed (which are all .js files).
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 looks like we do currently include QUnit test files in the JSHint scans.
| - 'src/**.php' | |
| - 'tests/**.php' | |
| - 'src/**.php' | |
| - 'tests/**.php' | |
| - 'tests/qunit/**/*.js |
It looks like Gruntfile.js specifies the exclusion of tests/qunit/(vendor|editor)/** as well. But unless I'm missing something, those directories no longer exist, so there's no need to include these exclusions here.
It also looks like grunt jshint:tests uses the tests/qunit/.jshintrc file for configuration. I haven't looked into why there are different configurations, but for the time being, below should also be changed to include that file in case it's updated.
TIL that you can't add comments or suggested changes to unmodified lines in a PR. So instead here's what I suggest for below.
# This file configures JSHint. Changes could affect the outcome.
- '**.jshintrc'
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 also wish that there were an easy way to prevent a specific job from running within a workflow when certain path filters do not match (don't run PHPCS when only .js files are touched). However, it seems that can only be accomplished by having a job that detects this (without relying on a third-party action). ChatGPT came up with the following rough example.
check-files:
runs-on: ubuntu-latest
outputs:
changed: ${{ steps.filter.outputs.changed }}
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
- id: filter
run: |
if git diff --name-only ${{ github.base_ref || github.event.before }} ${{ github.sha }} | grep -qE '\.php$'; then
echo "changed=true" >> "$GITHUB_OUTPUT"
else
echo "changed=false" >> "$GITHUB_OUTPUT"
fi
I don't think there's much benefit in this specific workflow because these jobs complete in under 30 seconds. But if there are any longer-running ones, it may be worthwhile to try.
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.
Misses files in the root directory, currently the PHP sample configs, webpack.
Good catch. PHP files in the root are indeed covered by phpcs so these need re-adding to the path filtering.
The webpack files aren't covered by eslint so they can remain excluded:
wordpress-develop/.eslintignore
Line 6 in cfcb4e9
| /tools |
| - 'src/**.php' | ||
| - 'tests/**.php' |
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 looks like we do currently include QUnit test files in the JSHint scans.
| - 'src/**.php' | |
| - 'tests/**.php' | |
| - 'src/**.php' | |
| - 'tests/**.php' | |
| - 'tests/qunit/**/*.js |
It looks like Gruntfile.js specifies the exclusion of tests/qunit/(vendor|editor)/** as well. But unless I'm missing something, those directories no longer exist, so there's no need to include these exclusions here.
It also looks like grunt jshint:tests uses the tests/qunit/.jshintrc file for configuration. I haven't looked into why there are different configurations, but for the time being, below should also be changed to include that file in case it's updated.
TIL that you can't add comments or suggested changes to unmodified lines in a PR. So instead here's what I suggest for below.
# This file configures JSHint. Changes could affect the outcome.
- '**.jshintrc'
| - 'src/**.php' | ||
| - 'tests/**.php' |
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 also wish that there were an easy way to prevent a specific job from running within a workflow when certain path filters do not match (don't run PHPCS when only .js files are touched). However, it seems that can only be accomplished by having a job that detects this (without relying on a third-party action). ChatGPT came up with the following rough example.
check-files:
runs-on: ubuntu-latest
outputs:
changed: ${{ steps.filter.outputs.changed }}
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
- id: filter
run: |
if git diff --name-only ${{ github.base_ref || github.event.before }} ${{ github.sha }} | grep -qE '\.php$'; then
echo "changed=true" >> "$GITHUB_OUTPUT"
else
echo "changed=false" >> "$GITHUB_OUTPUT"
fi
I don't think there's much benefit in this specific workflow because these jobs complete in under 30 seconds. But if there are any longer-running ones, it may be worthwhile to try.
| - 'src/**.css' | ||
| - 'src/**.js' | ||
| - 'src/**.json' | ||
| - 'src/**.php' |
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.
@johnbillion What do you think about removing the composer.* pattern here? The only dependency managed in Composer that is used in the build process is composer/ca-bundle. But the build process will never be affected by this without first manually updating the version (it's pinned to an exact version vs. using a semantic range) AND running grunt certificates:upgrade.
Otherwise, the only files related to certificates that would affect the build process are the .pem and .crt files that are in src/wp-includes/certificates. It may be worth adding **/*.pem and **/*.crt to this list of path filters for those scenarios.
Co-authored-by: Jonathan Desrosiers <359867+desrosj@users.noreply.github.com>
The main aim here is to reduce the chance of unnecessary workflows running when changes are not made to
srcfiles. As an example, a change to a phpunit test file currently triggers the e2e tests, the performance tests, and the build process testing, all of which are unnecessary.I would appreciate the proposed path changes being double checked in case I've missed something.
Ironically, changing these workflow files triggers the corresponding workflows in this PR.
Trac ticket: https://core.trac.wordpress.org/ticket/64227