Skip to content

Add support for linting annotations and other static analysis workflow improvements #76120

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.

Sign up for GitHub

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

Merged
desrosj merged 17 commits into from
Mar 5, 2026
Merged

Add support for linting annotations and other static analysis workflow improvements #76120

Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3cccf7e
Add support for linting annotations.
desrosj Mar 4, 2026
3cf00c0
Switch off deprecated formatter.
desrosj Mar 4, 2026
59fb444
Configure caching for `actions/setup-node`.
desrosj Mar 4, 2026
7f45020
Introduce some coding standards issues.
desrosj Mar 4, 2026
e81a47a
Only generate annotations on one job.
desrosj Mar 4, 2026
44b76f4
Remove unused `event` matrix values.
desrosj Mar 4, 2026
016c76c
Remove JS errors and add CSS ones.
desrosj Mar 4, 2026
464f02e
Fix typo in matrix value name.
desrosj Mar 4, 2026
900b6ca
`include` and `exclude` values must be scalar.
desrosj Mar 4, 2026
eed1253
Revert "Remove JS errors and add CSS ones."
desrosj Mar 4, 2026
d30e5a0
Revert "Introduce some coding standards issues."
desrosj Mar 4, 2026
0913877
Only run JS/CSS linting once.
desrosj Mar 4, 2026
802e921
Change condition for running JS/CSS linting.
desrosj Mar 4, 2026
8ff0b5e
Avoid a unique job just for annotations.
desrosj Mar 4, 2026
65504ba
Remove stray character.
desrosj Mar 4, 2026
b9b91da
Merge branch 'trunk' into add/annotations-for-style-and-script-linting
desrosj Mar 4, 2026
20f0ebd
Re-add the `pull_request` restrictions.
desrosj Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions .github/workflows/static-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ jobs:
event: ['${{ github.event_name }}']
node: ['20', '22', '24']
os: ['macos-15', 'ubuntu-24.04', 'windows-2025']

include:
# Only generate annotations once.
- node: '24'
os: 'ubuntu-24.04'
annotations: true
exclude:
# On PRs: Only test Node 20 on Ubuntu and Windows
- event: 'pull_request'
Expand All @@ -50,11 +56,12 @@ jobs:
uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0
with:
node-version: ${{ matrix.node }}
cache: npm
Copy link
Member

@aduth aduth Mar 4, 2026

Choose a reason for hiding this comment

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

This seems like it'll be a nice win as a standalone change 👍 Surprised we didn't have this or that it's not a default.

Though reading the relevant documentation, it does have me thinking whether we should have packageManager specified in package.json as a more standards-based way of activating this across all of the Node workflows.

Copy link
Member Author

@desrosj desrosj Mar 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Though reading the relevant documentation, it does have me thinking whether we should have packageManager specified in package.json as a more standards-based way of activating this across all of the Node workflows.

Ah, good catch. That is a relatively new addition to actions/seup-node.

I'm torn for this one. On one hand it's good to have it on by default for the sake of better performance. But on the other, making it an intentional addition makes it a conscious choice and could help guard against cache poisoning attacks when production assets are being built or commits are being pushed to the repository.

It's also possible that someone could submit a PR to add packageManager to package.json because it's just a good thing to do. In that case, it may unintentionally enable caching without awareness that caching ends up enabled within Actions.

I am leaning towards adding it now and auditing where actions/setup-node is being used to explicitly disable caching wherever necessary at the same time just to prevent that last scenario. I've made myself a note to create an issue to collect feedback once the dust settles from 7.0.


- name: Pin npm version for consistency
run: npm install -g npm@10

- name: Npm install
- name: npm install
# A "full" install is executed, since `npm ci` does not always exit
# with an error status code if the lock file is inaccurate. This also
# helps to catch dependencies not being installed with exact version.
Expand All @@ -63,8 +70,22 @@ jobs:
# See: https://github.com/WordPress/gutenberg/pull/39865
run: npm install

- name: Lint JavaScript and Styles
run: npm run lint
Copy link
Member

@aduth aduth Mar 4, 2026

Choose a reason for hiding this comment

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

Noting a couple things about this change to use granular scripts:

  • It duplicates the source of truth for "linting everything", i.e. I could foresee that we add new scripts to npm run lint and forget to include them here, or vice versa.
  • npm run lint uses concurrently to run these in parallel, so this could have a performance impact. Although (a) linting generally isn't our biggest problem in CI as far as time spent, and (b) we might be overdoing the parallelization that this could end up being a good thing anyways.

Copy link
Member Author

@desrosj desrosj Mar 5, 2026

Choose a reason for hiding this comment

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

It duplicates the source of truth for "linting everything", i.e. I could foresee that we add new scripts to npm run lint and forget to include them here, or vice versa.

Yes, that is possible. One approach to make sure this scenario is caught could be to run npm run lint anytime the scripts package is updated. But that would only ensure the PR introducing the new script runs it.

We likely need to just have better documentation and guidance around this within packages. Each package should have a section detailing where any automation or CI/CD testing should be added when updating the package as a reminder. Maybe that could be a reason to introduce a CONTRIBUTING.md file to individual packages.

- name: Lint package.json files
run: npm run lint:pkg-json

- name: Lint package-lock.json file
run: npm run lint:lockfile

- name: Validate TypeScript config
run: npm run lint:tsconfig

- name: Lint JavaScript
if: ${{ matrix.annotations }}
run: npm run lint:js -- --format compact

- name: Lint Styles
if: ${{ matrix.annotations }}
run: npm run lint:css -- --formatter compact

- name: Type checking
run: npm run build
Expand Down
Loading