Repository navigation
Add manual publish workflow with PHP version selection - #489
Conversation
mattgoud
left a comment
mattgoud
left a comment
There was a problem hiding this comment.
Review 馃煝 no blocker
Checks
.ai conventions: 馃煝
| Source | Result |
|---|---|
| Contexts read | none, PrestaShop/docker has no .ai/ tree (core PHP conventions do not apply to this Python and workflow diff) |
pr-hygiene.py |
馃煝 clean |
code-standards.py |
not applicable (no PHP) |
Tested on: master @ a89de43 (the PR base), PR head 732a24f, Python 3.12, no docker
Run: tests/test_get_json_versions.py 12 passed, flake8 clean (also green in the nose and flake8 jobs, which do collect the new test module). Parity check: for each of the 339 PrestaShop/PHP pairs of versions.py, VersionManager.parse_version("<ps>-<php>") returns exactly the images parse_version("<ps>") gives for that PHP, so tag push "$PS_VERSION-$php_version" in the loop builds the same tags and aliases as publish.yml, branches, nightly and classic distributions included. CLI probed with all, ALL, 8, 8.1-apache, 8.1$(id), blank and comma-only selections.
Not run: the workflow itself (dispatch needs it on the default branch), actionlint (not installed locally). The job conditions were read by hand: guard failure skips everything, build_base off lets publish_prestashop run on a skipped publish_base, a base failure stops the PrestaShop builds.
Findings
- 馃煚 Should fix:
php_versionsdefaults toallwith both booleans on, so dispatching without touching the form force rebuilds and overwrites every tag on Docker Hub. Drop the default and let the operator typeall. - 馃煛 Nit: a blank selection (
" "," , ") passesrequired, filters to[]and the run goes green with nothing built. Exit inmain()on an empty list. - 馃挕 Suggestion: the
publish-images-manualconcurrency group only serialises manual runs; a manual run and a push triggeredpublish.ymlcan still force push the same tag at the same time. Sharing a group means touchingpublish.yml, so a follow-up at most.
| php_versions: | ||
| description: 'PHP versions to rebuild, comma separated (e.g. 8.1,8.4), or "all"' | ||
| required: true | ||
| default: 'all' |
There was a problem hiding this comment.
馃煚 Should fix: with this default and both booleans on, one click on Run workflow without touching the form force rebuilds and overwrites every tag: 20 base images and 339 PrestaShop/PHP pairs (2 containers each, 3 platforms) at max-parallel: 2, the old 1.7.7.x tags included. Dropping the default (all stays a value the operator types) makes a full republish a deliberate choice.
There was a problem hiding this comment.
Seems fair for the default behaviour to me, sure we have to be careful, but we're professionals 馃槈
| def parse_php_versions(value): | ||
| if value is None or value.strip() == 'all': | ||
| return None | ||
| return [php.strip() for php in value.split(',') if php.strip()] |
There was a problem hiding this comment.
馃煛 Nit: a blank selection like " " or " , " passes the required check and comes out as [], so the run goes green with nothing built (locally, --php " , " --base prints []). Exiting in main() when the list is empty would match the "fails early" of the description.
There was a problem hiding this comment.
Really nit, if it's small why not add a a check
The concurrency suggestion however seems really important
Republishing images required a fake commit since publish.yml only runs on push and only rebuilds changed images. The new workflow_dispatch workflow, restricted to prestashop-sa, force pushes base and PrestaShop images for selected PHP versions (or all). Both publish workflows share a concurrency group so they never force push the same tags at the same time. get_json_versions.py gains --php, --base and --php-for options; its default output, used by publish.yml, is unchanged.
732a24f to
3008f59
Compare
4 participants
Why
publish.ymlonly runs on push tomasterand only rebuilds images whose files changed (any change inbase/rebuilds everything). Forcing a republish required a fake commit.What
New
Publish images manuallyworkflow (workflow_dispatch), restricted to theprestashop-sateam via the sharedteam-guardaction (same assync-releases.yml).Inputs:
php_versions:all(default) or a comma separated list, e.g.8.1,8.4build_base: rebuild and push the base images of these PHP versions (default true)build_prestashop: rebuild and push every PrestaShop image using these PHP versions (default true)Selected images are force pushed even if the tag already exists on Docker Hub. PrestaShop images build after the base images, or alone when
build_baseis off. An unknown PHP version fails the run early.get_json_versions.pygains--php,--baseand--php-foroptions to compute the matrices. Its default output (used bypublish.yml) is unchanged, covered by a test.publish.ymlis untouched.Docs added in
HOW-TO-USE.md.How to test
nosetestsandflake8passphp_versions=8.5andbuild_prestashopoff as a smoke test, and check a nonprestashop-sauser is rejected