-
Notifications
You must be signed in to change notification settings - Fork 3k
#63170 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?
Changes from all commits
b2b9589
c0548be
142f1a2
f2a751b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,12 @@ on: | |
- '3.[7-9]' | ||
- '[4-9].[0-9]' | ||
paths: | ||
# Any change to a PHP, CSS, JavaScript, JSON, HTML, or otherwise tested file should run checks. | ||
- '**.css' | ||
- '**.html' | ||
- '**.js' | ||
- '**.json' | ||
- '**.php' | ||
# Any change to a source PHP, CSS, JavaScript, JSON, HTML, or otherwise tested file should run checks. | ||
- 'src/**.css' | ||
- 'src/**.html' | ||
- 'src/**.js' | ||
- 'src/**.json' | ||
- 'src/**.php' | ||
- 'src/license.txt' | ||
- 'src/SECURITY.md' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Including |
||
# These files configure npm and the task runner. Changes could affect the outcome. | ||
|
@@ -30,7 +30,8 @@ on: | |
- 'Gruntfile.js' | ||
# These files configure Composer. Changes could affect the outcome. | ||
- 'composer.*' | ||
# This files affect the phpunit tests. Changes could affect the outcome. | ||
# These files affect the phpunit tests. Changes could affect the outcome. | ||
- 'phpunit.xml.dist' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good catch! |
||
- 'tests/phpunit/**' | ||
# Confirm any changes to relevant workflow files. | ||
- '.github/workflows/phpunit-tests.yml' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,13 @@ on: | |
- '3.[7-9]' | ||
- '[4-9].[0-9]' | ||
paths: | ||
# Any change to a PHP, CSS, JavaScript, or JSON file should run checks. | ||
- '**.css' | ||
- '**.js' | ||
- '**.json' | ||
- '**.php' | ||
# Any change to a source PHP, CSS, JavaScript, JSON, or certificate file should run checks. | ||
- 'src/**.css' | ||
- 'src/**.js' | ||
- 'src/**.json' | ||
- 'src/**.php' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @johnbillion What do you think about removing the Otherwise, the only files related to certificates that would affect the build process are the |
||
- 'src/**.pem' | ||
- 'src/**.crt' | ||
# These files configure npm and the task runner. Changes could affect the outcome. | ||
- 'package*.json' | ||
- '.npmrc' | ||
|
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.
It looks like
Gruntfile.js
specifies the exclusion oftests/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 thetests/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.
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.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.