-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Run PHPCS in CI for FSE plugin #43458
Changes from all commits
2ecbec3
80bdb45
f1a512f
6c9c0eb
09bdbc7
d560633
ec2bb59
1d8b816
d6d196a
28ecfff
9c9d563
908809d
24fcb81
a679a24
03c5faa
df2bdb2
5271dda
f838d3e
0998920
de372ca
634568d
d4d52bb
bf74d5c
65261ce
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 |
---|---|---|
@@ -1,8 +1,9 @@ | ||
on: | ||
pull_request: | ||
# only trigger this workflow if FSE plugin files have been modified | ||
# only trigger this workflow if FSE plugin files have been modified, or if packages have been updated. | ||
paths: | ||
- 'apps/full-site-editing/full-site-editing-plugin/**' | ||
- 'apps/full-site-editing/**' | ||
- 'yarn.lock' | ||
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. It turns out that this match is too broad and it will trigger on PRs that are not related to FSE plugin. For example see D45192-code. 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. I'll file a PR to remove it 👍 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. |
||
|
||
name: FSE plugin | ||
|
||
|
@@ -109,3 +110,32 @@ jobs: | |
- name: Run phpunit command | ||
run: yarn test:php | ||
working-directory: apps/full-site-editing | ||
|
||
phpcs: | ||
name: Run phpcs | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@master | ||
|
||
- name: Run composer | ||
uses: nick-zh/composer-php@master | ||
with: | ||
action: 'install' | ||
|
||
- name: Get changed files | ||
id: changes | ||
uses: lots0logs/gh-action-get-changed-files@2.1.4 | ||
with: | ||
token: ${{ secrets.GITHUB_TOKEN }} | ||
|
||
- name: Displaying changed files | ||
run: echo ${{ join( fromJson( steps.changes.outputs.all ) ) }} | ||
|
||
- name: Execute phpcs on changed files | ||
run: ./vendor/bin/phpcs --standard=apps/phpcs.xml ${{ join( fromJson( steps.changes.outputs.all ), ' ' ) }} | ||
if: ${{ steps.changes.outputs.all != '' }} | ||
|
||
- name: No changes found | ||
run: echo "No changes found to check!" | ||
if: ${{ steps.changes.outputs.all == '' }} |
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.
Not sure if we should build and sync diff creation for things outside of actual plugin folder. 🤔 It probably won't cause issues since we only merge version bump PRs anyway. Another option would be to isolate the changes here in a separate workflow file with these broader matchers.
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 might be possible. IMO, we should be running:
yarn build
phpunit
phpcs
to any change in apps/full-site-editing (e.g. to pick up on config changes). And on changes to yarn-lock.
IMO the sync diff is the exception 🤔 I wonder if we can restrict a job based on the path itself.
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.
Yeah, and there's currently not a lot of files in
apps/full-site-editing/
(outside ofapps/full-site-editing/full-site-editing-plugin
), so the overhead shouldn't be too bad.That might make sense in the long run.
Personally, I wouldn't block merging this PR on that -- I think it's important to get PHPCS into CI, and we can tweak and iterate how we do it later.
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.
Afaik that hasn't been introduced in GH actions proper yet, although I've seen some 3rd party actions for it (I would rather avoid that dependency).
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 don't consider this a blocker either, just a possible future improvement.