[2.x] ci(core): overhaul reusable backend workflow#4693
Conversation
|
I added this to the |
|
Documentation PR: flarum/docs#534 |
|
Thanks for taking this on — the matrix is genuinely hard to reason about today, and dropping the include/exclude sprawl for a clean cartesian is the right direction. The action SHA-pinning (and fixing the unpinned I'd like to discuss a couple of points before merging — let me know your thoughts on these: 1. Keep the floor — test minimum and latest The new defaults move to latest-only DBs ( So I'd test both ends of the supported window — the minimum we claim to support and the current latest — and treat both as gating. We don't need the floor on every PHP though: someone still on MySQL 5.7 is realistically on older PHP too, and the SQL we generate doesn't branch on PHP version, so floor-DB on the oldest PHP only is enough: include:
- php: ${{ fromJSON(inputs.php_versions)[0] }}
db: mysql:5.7
prefix: ''
- php: ${{ fromJSON(inputs.php_versions)[0] }}
db: mariadb:10.11
prefix: ''
- php: ${{ fromJSON(inputs.php_versions)[0] }}
db: postgres:10
prefix: ''(Latest of each engine stays in the main cartesian; SQLite is 3 throughout — no separate floor.) Worth aligning the exact minimums with the supported versions listed in 2. Prefix testing doesn't need to be a full matrix dimension Right now include:
- php: ${{ fromJSON(inputs.php_versions)[0] }}
db: mariadb:12.3
prefix: flarum_MySQL/MariaDB specifically, since the 64-char limit is tightest there (SQLite has no real limit, Postgres is 63 and rarely the issue). That lets us drop the Net of both: roughly 15–16 jobs instead of 24, with minimum + latest DB coverage and prefix coverage all represented. Minor: the old matrix used None of this is far off — happy to look again once the floor/prefix shape is settled. You mentioned being open to adjusting DB versions, so hopefully this lands as expected. |
|
TL;DR AI Summary:
After our discussion I agree that we should also test the framework against older database versions to keep the confidence that the framework is compatible with the widley used versions, however expanding the matrix thru the One additional thing I think we should split apart here is:
Would you agree that it makes sense to have broader DB version coverage just in the framework but leave the latest versions of MySQL, MariaDB, PGSQL and Sqlite as the default for calling workflows? I did a small POC where I input What I would circle back here is what we also discussed in potentially reviving the pre release workflow or setting up a new one which can be called manually at any time (though usually before a new release) which runs the workflows for a greater set of DB versions. In such a scenarion job concurrency and with that time until all jobs are done is less of an issue while still having the confidence that the framework doesn't just support the newest, but also the oldest "floor" versions and potentially also some versions in between. In such a scenario it also wouldn't matter as much DB Prefix tests are run against the same DB version for multiple PHP versions. About the feedback for the Here is my middle way proposal: We leave the default to true (all jobs are canceled if one fails) but actually set it to false in backend.yml which calls the reusable backend workflow in the framework?
I agree that practically there are no cases where a prefix test would break on one PHP version but not the other. Having said that, I'm uncertain if it's worth it to keep some of the unexpected behaviour to decrease the job count as is currently by ~30% (though the impact would become more exponential if e.g. new PHP versions are introduced and we wanna keep the default PHP versions (i.e. not remove 8.3 even after PHP 9 was released). For the vast majority of Flarum Extensions which are open source this isn't an issue and has no implications and costs as action minutes are free for public repositories. The concurrency of the default github hosted runner is 60 meaning there is still room for growth with adding more PHP versions until the concurrency is reached and jobs are stuck in the queue until they are picked up. For high pace vendors where multiple developers are creating pull requests at the same vendors can use the runner_type input and use a custom github hosted runner with a max concurrency of 1000. This if course would introduce extra costs for both public and private repos but from experience isn't really an issue. |
Fixes #0000
Changes proposed in this pull request:
db_prefixesinput:Allows consuming workflows to pass in custom table prefix names or disable prefix testing altogether by passing
'[""]'Calling workflows can now predictably customize which permutations across DB Versions, PHP Versions and now as well DB Prefixes are run. The previous implementation did intend to make it possible for DB versions and PHP Versions already, but it didn't actually work consistently and was hard to understand. E.g. Sqlite tests were still run even when overriding
db_versionsin a calling workflow.While right now probably just a tiny fraction of Flarum Communities are running on those DB versions, it future proofs the workflows and is a tradeoff between covering a wide blast radius whilst not exploding the amount of Jobs being run per push. I'm open though to add additional DB versions if requested
actions/checkoutfromv4tov6actions/checkoutandshivammathur/setup-phpto immutable commit hashes):This allows consumer extensions to keep targeting 2.x for the reusable backend workflow whilst increasing protection against upstream supply chain attacks. Dependabot creates PR's to bump actions. This is the same approach Laravel does, see Bump the github-actions group with 4 updates laravel/framework#60364 for an example.
fail-fastto false to continue run other jobs when a single fails:This allows to get a full picture of the backend flows in one go.
Reviewers should focus on:
Screenshot
Necessity
Confirmed
composer test).Required changes: