#64227 closed task (blessed) (fixed)
GitHub Actions updates and improvements for 7.0
| Reported by: | desrosj | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.0 |
| Component: | Build/Test Tools | Version: | |
| Severity: | normal | Keywords: | has-patch |
| Cc: | Focuses: |
Change History (44)
This ticket was mentioned in PR #10511 on WordPress/wordpress-develop by @desrosj.
9 months ago
#1
- Keywords has-patch added
@desrosj commented on PR #10511:
9 months ago
#2
Waiting to merge this until the discussions on #10512 are resolved to ensure this task deletes all of the correct files.
@desrosj commented on PR #10511:
9 months ago
#4
Fixed in r61286.
This ticket was mentioned in PR #9457 on WordPress/wordpress-develop by @johnbillion.
8 months ago
#5
The main aim here is to reduce the chance of unnecessary workflows running when changes are not made to src files. 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.
@desrosj commented on PR #10959:
6 months ago
#7
Merged in r61663.
This ticket was mentioned in PR #11031 on WordPress/wordpress-develop by @johnbillion.
6 months ago
#8
Trac ticket: https://core.trac.wordpress.org/ticket/64227
This addresses two issues with the e2e test workflow.
## Tricky Trixie
Last night at 01:11 UTC the latest tag for the PHP-based wordpressdevelop Docker images was changed from 8.2 to 8.3. Merging this change causes all the images to be rebuilt in the same way as we saw for Core-63876.
The e2e tests workflow doesn't specify a PHP version, so runs after that point switched from PHP 8.2 to 8.3.
Switching from PHP 8.2 to 8.3 also meant switching from Debian Bullseye using cURL 7.74.0 to Debian Trixie using cURL 8.14.1. In cURL 7.77.0 a change was made that means localhost always resolves to either 127.0.0.1 or ::1 and never consults /etc/hosts.
In the Docker config for the local dev environment, localhost points to the host machine, not to 127.0.0.1. We're not the only ones affected by this.
During installation, WordPress calls wp_install_maybe_enable_pretty_permalinks() which performs a loopback request to check that pretty permalinks work. If they do, they get enabled by default.
Then we finally get to the e2e tests. The e2e tests assume that pretty permalinks are enabled. They don't get enabled explicitly. When a test requests /this-does-not-exist it ironically receives an HTTP 200 response rather than the expected 404 because pretty permalinks are not enabled, causing URL path info to be ignored and the home page to be served.
This is why the the e2e tests that assume pretty permalinks are in place were passing on PHP 8.2 and now fail on the new PHP 8.3 image with cURL 8.
## Awkward apt-get
The e2e tests only run on Chrome.
The e2e workflow installs browsers for Playwright using npx playwright install --with-deps in accordance with to the Playwright documentation for CI. This installs dependencies for running headless Chromium which aren't otherwise included by default on the container images.
By default, Playwright installs Chromium, Firefox, and Webkit, but we only need Chromium. By excluding Firefox and Webkit, a massively reduced set of dependencies are pulled in by apt-get. The affected step goes from ~15 minutes to ~30 seconds.
This fix isn't technically needed to get the e2e tests passing again, but it does mean the workflow run doesn't bump right up against the 20 minute timeout for the workflow.
@johnbillion commented on PR #11031:
5 months ago
#10
I agree that perhaps switching to a different host name in the container is worth investigating, but as per the comments in https://github.com/curl/curl/issues/11104 the reason localhost is used is so there's no need to mess with /etc/hosts on the host machine. Needs some research, maybe bite the bullet and go with dnsmasq.
@johnbillion commented on PR #11031:
5 months ago
#11
Committed in https://core.trac.wordpress.org/changeset/61733
This ticket was mentioned in PR #11250 on WordPress/wordpress-develop by @desrosj.
5 months ago
#16
The installation tests currently fail for older branches
Trac ticket: Core-64227.
## Use of AI Tools
@desrosj commented on PR #11250:
5 months ago
#17
This is ready to go. Since you can't run a workflow_dispatch event from a PR branch, I've pushed the changes to my fork and done so. All looks good.
@desrosj commented on PR #11250:
5 months ago
#20
Merged in r62022.
@desrosj commented on PR #11268:
5 months ago
#22
Merged in r62033.
This ticket was mentioned in PR #10583 on WordPress/wordpress-develop by @jorbin.
5 months ago
#24
Trac ticket: Core-64227
@desrosj commented on PR #10583:
5 months ago
#26
Merged in r62042.
This ticket was mentioned in PR #11278 on WordPress/wordpress-develop by @desrosj.
5 months ago
#27
This makes several improvements to path filtering within GitHub Actions workflows to account for recent changes to the build script and a few other minor improvements.
Trac ticket: Core-64227.
## Use of AI Tools
None.
This ticket was mentioned in PR #11280 on WordPress/wordpress-develop by @desrosj.
5 months ago
#28
This simplifies Composer-related caching by eliminating the need for an extra step to create a date-based value to use in cache keys.
Trac ticket: Core-64227.
## Use of AI Tools
Used Claude to create the initial draft of these changes.
#30
@
5 months ago
- Resolution → fixed
- Status new → closed
With RC1 later today, going to close this out. This can be reopened or referenced if more changes are required for 7.0.
Created #64893 for the 7.1 release cycle.
@desrosj commented on PR #11278:
5 months ago
#33
Merged in r62095.
This ticket was mentioned in PR #9767 on WordPress/wordpress-develop by @johnbillion.
4 months ago
#34
Trac ticket: https://core.trac.wordpress.org/ticket/64227
Previously:
- https://github.com/WordPress/wordpress-develop/pull/8007
- https://github.com/WordPress/gutenberg/pull/69126
This change:
- Introduces Zizmor which is a tool for linting workflow files for security weaknesses.
- Fixes some issues in workflow files that were identified by Zizmor.
## Code scanning alerts
Like the existing Actionlint job, Zizmor reports to GitHub Code Scanning by uploading a SARIF report via the github/codeql-action/upload-sarif action (none of these tools use CodeQL but GitHub groups its code scanning actions under the CodeQL name).
- This makes the results available on the Security → Code Scanning screen for users with write permission on the repo.
- Code scanning errors and warnings that are newly introduced in a PR will cause the code scanning to fail. The code scanning job itself won't fail, it will produce a separate "Code scanning results" workflow on the Checks screen which shows the failures, accompanied with an inline annotation on the affected file and line.
- Existing issues (documented below) will remain in place until fixed or dismissed. They won't affect new PRs, so they behave like a baseline.
- Note: It's unclear whether the dismissed issues in this branch will carry across to
trunkwhen this gets committed. I suspect not because the PR won't get merged on GitHub. I'll handle it post merge if necessary.
- Note: It's unclear whether the dismissed issues in this branch will carry across to
## Todo
- [x] Implement scanning
- [x] Add docs to the readme
- [x] Address reported errors
- [x] Draft new page for the coding standards handbook
## We already have Actionlint in place. Why do we need Zizmor?
The tools are complementary. Actionlint implements 38 checks, Zizmor implements 34 in its strictest mode, but only around 9 of the checks fully overlap. Zizmor gives us a net of about 20 new checks in its default mode, including detection of archived actions, spoofable bot conditions, problematic use of GitHub Actions features and patterns, overprovisioned secrets, git ref confusion, and unpinned actions.
Actionlint primarily focuses on correctness, with some security related audits included. Zizmor primarily focues on security.
## Results
Some code scanning alerts remain after I dismissed the false positives and closed some non-critical warnings.
### Errors
- ❌ use of fundamentally insecure workflow trigger
- Refers to the
pull_request_targettrigger. - We're aware that this trigger is fundamentally risky. Need to document it inline and then close the alerts.
- Refers to the
- ❌ Use of "downloadArtifact" in "actions/github-script" action.
- Need to audit these to ensure the artifact contents are not treated as trusted.
- ❌ Use of 'actions/checkout' with a custom ref
- Need to audit these so we're clear on where the refs originate.
- ❌ Write to
$GITHUB_ENVand$GITHUB_OUTPUTin a bash script- These are only dangerous if the input is user-controlled. I've closed off all the false positives, the remaining needs to be reviewed.
### Warnings
- ⚠️ Obfuscated usage of GitHub Actions features
- Refers to incorrect usage of
${{ true }}instead oftrue. No security concern but needs an audit.
- Refers to incorrect usage of
@johnbillion commented on PR #9767:
4 months ago
#38
This ticket was mentioned in PR #11808 on WordPress/wordpress-develop by @johnbillion.
3 months ago
#41
Trac ticket: https://core.trac.wordpress.org/ticket/64227 (7.0)
Trac ticket: https://core.trac.wordpress.org/ticket/64893 (7.1)
## Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 4.7
Used for: Research and collab
@johnbillion commented on PR #11808:
3 months ago
#42
@desrosj commented on PR #11280:
2 months ago
#43
Merged in r62443.
@lancewillett commented on PR #9457:
2 weeks ago
#44
To test this PR, I implemented GitHub's filter-glob semantics and replayed the last 12 months of trunk commits (1,800 of them) through both the old and new filter sets. The numbers below are measured rather than estimated.
_Transparency in AI usage: I used Claude Code Opus 5 to review, test, and craft this reply._
The approach holds up well: this change avoids roughly 790 workflow runs per year. The phpunit-test-file example accounts for 512 of those on its own, across the e2e, performance, and build-process workflows.
---
Four things worth changing before you merge.
### 1. Rebase first, and keep the entries trunk has since added
The PR currently shows as conflicting, and the branch is 1,691 commits behind trunk. ece2d364b4 ("Improve GitHub Actions path filtering rules", See #64227) has since added tools/gutenberg/** and tools/vendors/** to seven of these workflows. Those entries do not exist on this branch, so a conflict resolution that takes the branch side will silently drop them:
check-built-files .github/workflows/reusable-check-built-files.yml
tools/gutenberg/**, tools/vendors/**
end-to-end-tests tools/gutenberg/**, tools/vendors/**
javascript-tests tools/gutenberg/**, tools/vendors/**
local-docker-environment tools/gutenberg/**, tools/vendors/** (push)
performance tools/gutenberg/**, tools/vendors/**
test-build-processes tools/gutenberg/**, tools/vendors/**
phpunit-tests tests/phpunit/multisite.xml
Separately, becfe55789 already landed the phpunit.xml.dist entry and the "This files affect"→"These files affect" fix, so those two hunks become no-ops after a rebase.
### 2. phpunit-tests.yml, end-to-end-tests.yml, and performance.yml need tools/local-env/**
reusable-phpunit-tests-v3.yml runs every PHPUnit invocation through node ./tools/local-env/scripts/docker.js, plus npm run env:start and npm run env:install. The e2e and performance workflows reach the same scripts through their own reusable workflows. **.js matched them; src/**.js does not.
Only 7 commits touched tools/local-env/ in the last 12 months, but 6 of those touched nothing under src/, so they would now run no PHPUnit at all. Two examples: 4b4e950756 ("Ensure all db WP-CLI commands work", docker.js) and 4ac482fd97 ("Explicitly enable pretty permalinks on the local development environment", install.js). Both change the harness the tests run in.
# The test runner executes through these scripts. Changes could affect the outcome.
- 'tools/local-env/**'
Worth noting the e2e and performance lists keep tools/webpack/**, and local-docker-environment.yml already uses tools/local-env/**, so the pattern is established.
### 3. coding-standards.yml needs Gruntfile.js
The JavaScript job runs grunt jshint, and the Gruntfile defines jshint.grunt = { src: ['Gruntfile.js'] }. The other four JSHint targets (tests, themes, media, core) all resolve to tests/qunit/** or paths under src/, so Gruntfile.js is the only JSHint input this drops.
It changed 46 times in the last 12 months, and accounts for 18 of the 33 commits this workflow would newly skip. Every other workflow touched here already lists it.
### 4. test-build-processes.yml is missing the root *.php entry
Five siblings received - '*.php'; this one narrowed to src/**.php alone. It matters most here: the Gruntfile's copy:files task copies root wp-config-sample.php into the build directory, and this workflow's job is verifying the src-to-build copy across a directory: ['src','build'] matrix.
### Nit/typo
"These files configures JSHint" should be "configure" in coding-standards.yml.
### Questions
Adding src/**.json to end-to-end-tests.yml and performance.yml widens them rather than narrowing: 189 files now trigger those two workflows that previously did not, being 119 block.json files under src/wp-includes/ and 69 theme.json files under src/wp-content/. Defensible on the merits, since both affect rendering and performance, but it is the opposite of the stated goal and is not mentioned in the description. Intentional?
src/SECURITY.md in phpunit-tests.yml matches nothing, since the file lives at the repository root, unlike src/license.txt on the line above. Pre-existing rather than something this PR introduces, but you are already adding root-file patterns a couple of lines below.
Lastly, are any of the previous code comments from other reviewers still valid?
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
This new
post-branchingtask is meant for any tasks that should be run immediately after creating a new numbered branch. Currently, this only consists of GitHub Actions-related tasks.In addition to the pre-existing
replace:workflow-references-local-to-remotetask, a newclean:workflowstask is being added that deletes the workflow files that are only intended fortrunk.Trac ticket: Core-64227