Make WordPress Core

Opened 9 months ago

Closed 5 months ago

Last modified 2 weeks ago

#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:

Description

This ticket is for various updates and improvements for Core's GitHub Actions workflows.

Previously:

Change History (44)

This ticket was mentioned in PR #10511 on WordPress/wordpress-develop by @desrosj.


9 months ago
#1

  • Keywords has-patch added

This new post-branching task 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-remote task, a new clean:workflows task is being added that deletes the workflow files that are only intended for trunk.

Trac ticket: Core-64227

@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.

#3 @desrosj
9 months ago

In 61286:

Build/Test Tools: Introduce post-branching Grunt task.

In [59673], a Grunt subtask was introduced to convert workflow file references in GitHub Action workflows from local ones to remote ones that target the trunk branch.

The workflow-references-local-to-remote Grunt task should be run after creating a new numbered branch to take advantage of the work started in [58165].

This commit introduces a new target for the clean Grunt task: workflows. Running grunt clean:workflows will delete all workflow files that are not intended to exist outside of trunk.

Another Grunt task (post-branching) has also been added as a way to group all tasks taht should run within a newly created branch. For now, this contains clean:workflows and replace:workflow-references-local-to-remote. But more can be added in the future as more aspects of the branching process are automated.

Props johnbillion.
See #64227.

@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.

#6 @desrosj
6 months ago

In 61663:

Build/Test Tools: Upgrade third-party GitHub Actions.

This updates the following GitHub Actions to their latest versions:

  • actions/cache from 4.3.0 to 5.0.3
  • actions/checkout from 5.0.0 to 6.0.2
  • actions/download-artifact from 6.0.0 to 7.0.0
  • actions/setup-node from 6.0.0 to 6.2.0
  • actions/upload-artifact from 5.0.0 to 6.0.0
  • codecov/codecov-action from 5.5.1 to 5.5.2
  • shivammathur/setup-php from 2.35.5 to 2.36.0

See #64227.

@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.

#9 @johnbillion
5 months ago

In 61733:

Build/Test Tools: Explicitly enable pretty permalinks on the local development environment.

The wp_install_maybe_enable_pretty_permalinks() function usually enables pretty permalinks during installation, but it relies on a loopback request that may not work on all development environments.

In temporary lieu of fixing the underlying loopback request on all the container images, this switches to explictly enabling permalinks and removes redundant permalink configuration from GitHub Actions workflow files.

See #64227

@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.

#12 @desrosj
5 months ago

In 61836:

Build/Test Tools: Increase timeout-minutes for PHPUnit workflow.

The GitHub Actions workflow responsible for running the PHPUnit test suite is frequently encountering the default timeout-minutes value of 20 since the changes in [61438]. The result is that the workflow is consistently unable to finish running and ends up being cancelled.

This bumps the default value to 40 until the overall speed of the build script can be improved.

See #64225, #64227, #64393.

#13 @desrosj
5 months ago

In 61871:

Build/Test Tools: Explicitly enable pretty permalinks on the local development environment.

The wp_install_maybe_enable_pretty_permalinks() function usually enables pretty permalinks during installation, but it relies on a loopback request that may not work on all development environments.

In temporary lieu of fixing the underlying loopback request on all the container images, this switches to explictly enabling permalinks and removes redundant permalink configuration from GitHub Actions workflow files.

Merges [61733] to the 6.9 branch.

Props johnbillion, jorbin.
See #64227.

#14 @desrosj
5 months ago

In 61872:

Build/Test Tools: Explicitly enable pretty permalinks on the local development environment.

The wp_install_maybe_enable_pretty_permalinks() function usually enables pretty permalinks during installation, but it relies on a loopback request that may not work on all development environments.

In temporary lieu of fixing the underlying loopback request on all the container images, this switches to explictly enabling permalinks and removes redundant permalink configuration from GitHub Actions workflow files.

Merges [61733] to the 6.8 branch.

Props johnbillion, jorbin.
See #64227.

#15 @desrosj
5 months ago

In 61875:

Build/Test Tools: Test against MySQL 9.6 & MariaDB 12.1.

These are now the latest Innovation and Rolling releases, respectively.

See #64225, #64227.

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.

#18 @desrosj
5 months ago

In 62022:

Build/Test Tools: Fix installation tests for PHP <= 7.3.

In MySQL 8.0, the default authentication plugin changed from mysql_native_password to caching_sha2_password, which is not available in PHP 7.3 and earlier. The installation testing workflow currently accounts for this, but only for PHP 7.2 & 7.3.

It also does not account for a change in MySQL 8.4 which changed the related flag for controlling this from --default-authentication-plugin to --authentication-policy.

This adjusts the flags and surrounding conditions to ensure the correct ones are passed for the right combinations.

Props johnbillion.
See #64225, #64227.

#19 @desrosj
5 months ago

In 62023:

Build/Test Tools: Add additional innovation release excludes.

This excludes MySQL 9.5 jobs for the installation testing workflow. Only the most recent innovation release of MySQL should be tested.

Follow up to [61875].

See #64225, #64227.

@desrosj commented on PR #11250:


5 months ago
#20

Merged in r62022.

#21 @desrosj
5 months ago

In 62033:

Build/Test Tools: Update third-party GitHub Actions.

This updates the following GitHub Actions:

  • actions/cache from 4.3.0 to 5.0.3.
  • actions/checkout from 5.0.0 to 6.0.2.
  • actions/download-artifact from 7.0.0 to 8.0.1.
  • actions/setup-node from 6.0.0 to 6.3.0.
  • actions/upload-artifact from 6.0.0 to 7.0.0.
  • ramsey/composer-install from 3.1.1 to 4.0.0.
  • shivammathur/setup-php from 2.35.3 to 2.37.0.
  • slackapi/slack-github-action from 2.1.1 to 3.0.1.

See #64227.

@desrosj commented on PR #11268:


5 months ago
#22

Merged in r62033.

#23 @desrosj
5 months ago

In 62035:

Build/Test Tools: Trim performance workflow from 6.7-6.4.

[61736], [61801], [61082], and [61083] removed the performance.yml workflow from the 6.7-6.4 branches as part of #64083.

The workflow also needs to be removed from the workflow responsible for testing old branches to avoid failures when attempting to trigger a workflow_dispatch event on a non-existent workflow file.

See #64227, #64083.

This ticket was mentioned in PR #10583 on WordPress/wordpress-develop by @jorbin.


5 months ago
#24

Trac ticket: Core-64227

#25 @desrosj
5 months ago

In 62042:

Build/Test Tools: Test 6.9 in upgrade testing workflows.

This sets 6.9 as the most recent branch of WordPress in the upgrade testing workflows.

Props jorbin.
See #64227, #64235.

@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.

#29 @desrosj
5 months ago

In 62053:

Build/Test Tools: Update some third-party GitHub Actions.

This updates the following GitHub Actions:

  • actions/cache from 5.0.3 to 5.0.4
  • codecov/codecov-action from 5.5.2 to 5.5.3

This resolves the final node20-related deprecation notices being triggered by GitHub Actions.

See #64227.

#30 @desrosj
5 months ago

  • Resolutionfixed
  • Status newclosed

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.

Last edited 5 months ago by johnbillion (previous) (diff)

#31 @desrosj
5 months ago

In 62083:

Build/Test Tools: Exclude MySQL 9.5 in Local Docker test workflow.

Only the most recent innovation release should be tested.

Follow up to [61875].

See #64227.

#32 @desrosj
5 months ago

In 62095:

Build/Test Tools: Improve GitHub Actions path filtering rules.

This adjusts the path filtering rules for several GitHub Actions workflows to account for new and missed file paths.

Props westonruter.
See #64227.

@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:

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 trunk when this gets committed. I suspect not because the PR won't get merged on GitHub. I'll handle it post merge if necessary.

## Todo

## 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_target trigger.
    • We're aware that this trigger is fundamentally risky. Need to document it inline and then close the alerts.
  • ❌ 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_ENV and $GITHUB_OUTPUT in 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 of true. No security concern but needs an audit.

#35 @johnbillion
4 months ago

In 62250:

Build/Test Tools: Add more workflow file linting with Zizmor.

This change introduces Zizmor, which is a tool for linting GitHub Actions workflow files for security weaknesses. This compliments the existing Actionlint scanning.

For more information about Actionlint and Zizmor, see the GitHub Actions Workflow Standards page in the developer handbook: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/github-actions/

Some issues in workflow files that are reported by Zizmor will be addressed in follow-up commits.

Props johnbillion, desrosj.

See #64227

#36 @johnbillion
4 months ago

In 62251:

Build/Test Tools: Address some issues in GitHub Actions workflow files as reported by Zizmor.

This removes unnecessarily broad inheritance of secrets, replaces some GitHub Actions expressions with environment variables, removes git credential persistence, and adds documentation to the readme.

See #64227

#37 @johnbillion
4 months ago

In 62252:

Build/Test Tools: Update Actionlint to the latest version.

See #64227

#39 @johnbillion
4 months ago

In 62253:

Build/Test Tools: Remove unnecessary use of GitHub Actions expressions for values that resolve to "true" or "false" strings.

See #64227

#40 @johnbillion
4 months ago

In 62254:

Build/Test Tools: Use the exact tag name in version number comments that trail pinned actions.

None of these actions use v-prefixed tag names.

See #64227

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

@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?

Note: See TracTickets for help on using tickets.