ci: prevent stale jobs from rolling back Pages - #42
Conversation
There was a problem hiding this comment.
Review: prevent stale Pages assets
The change is sound and correctly implemented. Switching the asset copy in the three result publish.sh scripts from an unconditional cp to a guarded if [[ ! -e "$pages_dir/$file" ]] seeds a brand-new (orphan) pages branch while leaving asset refresh to the dedicated Pages workflow — so a late-finishing result job can no longer roll the UI back. The -e test and comment accurately reflect that intent, and adding the publish.sh paths to the Pages workflow paths trigger closes the loop by re-running publish-site.sh (which still overwrites unconditionally) when publish logic changes.
Findings below are non-blocking consistency/maintainability notes.
Maintainability — duplication: The 6-line comment + guarded-copy loop is now copy-pasted verbatim into all three publish.sh scripts, and the asset list (index.html app.js performance.html performance.js compatibility.html compatibility.js style.css _config.yml) is hardcoded in four places (the three publish.sh plus publish-site.sh). Adding a new asset requires editing all four in sync. Consider extracting a shared seed-site-assets.sh PAGES_DIR SITE_DIR helper with the list defined once. (Not required for this PR.)
Minor — git add inconsistency: ci/llgo-size/publish.sh uses git add . while the performance/compatibility publish.sh and publish-site.sh use git add -A. Pre-existing, but worth aligning while these three scripts are touched together.
| - main | ||
| paths-ignore: | ||
| - ci/llgo-size/site/** | ||
| - ci/llgo-size/publish.sh |
There was a problem hiding this comment.
[P3] publish.sh added to push paths-ignore but not pull_request
This PR adds ci/llgo-size/publish.sh to the push paths-ignore (line 9), but the pull_request paths-ignore block (lines 25-38) omits it — even though the sibling publish-site.sh and prepare-pages-branch.sh are ignored in both blocks. Effect: editing only ci/llgo-size/publish.sh skips the binary-size build on main pushes but still triggers the full build on PRs. (The perf/compat publish scripts are covered on both sides via the ci/llgo-performance/** / ci/llgo-compatibility/** globs, so only the size script is asymmetric.) If running the size job on PRs to validate publish logic is intended, consider adding a short comment; otherwise mirror this entry into the pull_request block for consistency.
| paths: | ||
| - ci/llgo-size/site/** | ||
| - ci/llgo-size/publish.sh | ||
| - ci/llgo-performance/publish.sh |
There was a problem hiding this comment.
[P3] Pages trigger lists perf/compat publish.sh it never runs
The Pages workflow adds ci/llgo-performance/publish.sh and ci/llgo-compatibility/publish.sh to its push paths trigger, but the publish-site job only runs prepare-pages-branch.sh and ci/llgo-size/publish-site.sh — never the performance/compatibility publish.sh. Editing only one of those (without touching ci/llgo-size/site/**) will launch a Pages redeploy that re-pushes identical assets. The rationale (all three share the ci/llgo-size/site/ assets, so a change to their copy logic should refresh the site) is reasonable but non-obvious; a one-line comment here would prevent future confusion.
Summary
Validation
bash -non all four publication scriptspython3 -m unittest ci/llgo-size/test_enrich_pull_requests.pyapp.jshash remained unchanged after all three pushes