Make WordPress Core

Opened 2 years ago

Closed 2 months ago

Last modified 2 months ago

#60370 closed enhancement (wontfix)

Test Tools: Clarifying reusable workflows in Github CI

Reported by: youknowriad's profile youknowriad Owned by:
Milestone: Priority: normal
Severity: minor Version:
Component: Build/Test Tools Keywords: close
Focuses: Cc:

Description

While working on https://github.com/WordPress/wordpress-develop/pull/5922 we had to increase the timeout limit for one of the reusable workflows of the repository: phpunit-tests-run.yml

The problem is that the update were not being taken into consideration in the PR because we explicitly refer to the "trunk" version of that workflow in phpunit-tests.yml

Github does offer a way to use relative links to other workflows from the same repository (instead of URLs), ti would have solved the issue there but it comes with a set of tradeoffs that need to be considered.

For example, updating Node.JS version in the reusable workflow would need to be done for all the previous branches that WP still supports while forcing the use of the "trunk" workflow means we can update in a single place.

The initial discussion about this happened on the following Slack Thread https://wordpress.slack.com/archives/C02RQBWTW/p1706523051675219

I'm opening this ticket so we can track this discussion and potentially address it (or not).

Change History (6)

This ticket was mentioned in Slack in #core by youknowriad. View the logs.


2 years ago

#2 @desrosj
2 months ago

  • Keywords close added

I believe that this is addressed in [59673]. Can you confirm @youknowriad?

The idea here is:

  • Use relative references in trunk
  • Switch to remote references after branching (so that the version within trunk is used, which makes it much easier to maintain).

Is there anywhere specific we could add documentation that would help with this? @johnbillion and I are working on a draft of a "GitHub Actions Best Practices" page for the Core Handbook. I think this goes nicely in there.

#3 @youknowriad
2 months ago

Ah I missed that, great improvement

I still wonder why we're switching to the trunk version when branching to be honest. That feels like it just creates instability and and implicit dependency. I'd rather always use relative references tbh.

#4 @johnbillion
2 months ago

It's mainly a balance between maintainability and stability.

The commit message in [59673] covers the two main somewhat opposing scenarios: we want a local reference to avoid the exact problem you experienced, but we don't want to endlessly have to maintain and backport workflow changes to 23 old branches.

Using a local reference for trunk and switching it back to a fully qualified remote reference in prior branches gives us the best of both. In particular, this means we no longer need to backport updates to deprecated actions and deprecated runner references, both of which were problematic in the past when someone started work on a backport and for example ubuntu-latest had switched from 22.04 to 24.04 since the previous workflow run.

#5 follow-up: @youknowriad
2 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

but we don't want to endlessly have to maintain and backport workflow changes to 23 old branches.

I understand and I agree that it's tedious but IMO we're shooting ourselves in the foot by doing so. For instance no one guarantees that an update to Node in trunk would work with the npm packages and scripts that were existing in the previous versions.

In other words, I wonder if we're overestimating the maintainability benefits here.

Anyway, I'm going to close this ticket. Personally I have a preference for full local imports but I can live with the current approach.

#6 in reply to: ↑ 5 @johnbillion
2 months ago

Replying to youknowriad:

but we don't want to endlessly have to maintain and backport workflow changes to 23 old branches.

I understand and I agree that it's tedious but IMO we're shooting ourselves in the foot by doing so.

Shooting ourselves in the foot by doing what? Maintaining 23 old branches? I fully agree, tell your boss.

no one guarantees that an update to Node in trunk would work with the npm packages and scripts that were existing in the previous versions.

This approach is about maintaining the workflow files and keeping them up to date so the workflows keep running reliably. The Node version that runs scripts is controlled via .nvmrc which is present in all branches. Updating Node in trunk doesn't affect (or rather, shouldn't affect) old branches. Same for Composer dependencies, PHP versions, MySQL versions, etc. Some older branches use different workflows, hence the versions in some workflow file names (reusable-phpunit-tests-v3.yml etc).

I wonder if we're overestimating the maintainability benefits here.

There were at least 27 commits to GitHub Actions during the 6.9 cycle alone: #63170. The team who work on these changes through each release cycle have decided this is the best approach for maintainability.

Note: See TracTickets for help on using tickets.