refactor(ci): extract composite actions and improve workflow hygiene#314
refactor(ci): extract composite actions and improve workflow hygiene#314james-elicx merged 3 commits intomainfrom
Conversation
- Add composite actions: setup-node-pnpm, build-vinext, deploy-example, comment-preview-urls - Cache Playwright browsers in CI to avoid ~150MB download per E2E run - Add concurrency group to deploy-examples.yml to prevent deploy pileup - Replace inline CJS node script in create-next-app job with portable bash - Convert ecosystem-run.yml inline Node scripts from CJS require() to ESM - Apply composite actions across all workflows to eliminate repeated boilerplate
…site action Remove the build-vinext, deploy-example, and comment-preview-urls composite actions and inline their steps back into each workflow. Keep setup-node-pnpm as the one reusable action (pnpm install + node setup). Retain all other improvements from the previous commit: concurrency groups, Playwright browser cache, bash-based dev-server check, ESM node scripts in ecosystem-run.
commit: |
|
Every workflow that does pnpm/node setup + install now uses the setup-node-pnpm composite action instead of the inline 3-step triplet. Added optional registry-url input to support publish.yml's npm auth. Exceptions (intentionally kept inline): - bonk.yml / bigbonk.yml: issue_comment trigger, GHAS constraint - tip.yml notify-on-failure: node-only, no pnpm install needed
|
/bigbonk review |
There was a problem hiding this comment.
Clean refactor — the dedup is well done and all the conversions are functionally equivalent. Two things worth noting:
GHAS alerts on setup-node-pnpm: The rationale for keeping bonk.yml/bigbonk.yml inline is sound. Those are issue_comment-triggered with contents: write, so referencing local composite actions creates a TOCTOU vector where a malicious PR could modify the action before checkout. The existing GHAS alerts on build-vinext and deploy-example are pre-existing (from deploy-preview-command.yml, also issue_comment-triggered) — this PR doesn't make that worse.
registry-url default of "": The actions/setup-node action treats an empty string for registry-url as "don't configure a registry", so passing through the empty default works correctly. Only publish.yml sets it to the npm registry, which is the right behavior.
Specific observations on each change:
-
Composite action — Correct 1:1 replacement. Every inline triplet used
node-version: 24,cache: pnpm, and--frozen-lockfile, which is exactly what the action defaults to. -
publish.ymlworking-directoryremoval — The old workflow ranpnpm run buildwithworking-directory: packages/vinext. The new version runspnpm run buildat the repo root, which delegates viapnpm --filter vinext run buildin the rootpackage.json. Functionally equivalent. -
create-next-appCJS→bash — The bash version usingcurl+killis simpler and more readable. One behavioral difference: the old Node version usedtaskkillon Windows for process cleanup. Since bothubuntu-latestandwindows-latestrunners now useshell: bash, andkillworks on Windows bash (Git Bash / MSYS2), this is fine. Thekill "$SERVER_PID" 2>/dev/null || truepattern handles the case where the process already exited. -
ecosystem-run.ymlCJS→ESM —node --input-type=modulewith heredoc is the correct way to run ESM from stdin. The oldnode -approach used CJSrequire()which works but is deprecated-path. Therequire('./packages/vinext/package.json')→readFileSync+JSON.parseis necessary because JSON imports need assertions in ESM. Functionally equivalent. -
Playwright cache — Cache key
playwright-chromium-${{ runner.os }}-${{ hashFiles('**/package.json') }}is reasonable. Theplaywright install chromiumcommand is idempotent — if the cache hits and the version matches, it's a no-op. If Playwright gets bumped,package.jsonchanges and the cache misses. The glob**/package.jsonis broad (includes fixtures/examples) but that's fine — better to over-invalidate than under. -
Deploy concurrency group —
deploy-${{ github.event_name }}-${{ github.ref }}means push-to-main and PR deploys get separate groups, and multiple pushes to the same PR cancel previous runs. Correct. -
Whitespace cleanup in
deploy-examples.yml— trailing spaces removed in the JS script block. Purely cosmetic.
LGTM.
|
Reviewed and approved PR #314. Here's the summary: Verdict: LGTM The PR cleanly extracts the repeated pnpm/node/install triplet into a composite action and applies it consistently across all 7 eligible workflows. The key points:
|
Summary
setup-node-pnpmcomposite action replaces the repeated 3-step pnpm/node/install triplet across all eligible workflows — every workflow that does setup and install now uses it consistentlydeploy-examples.yml— prevents deploy jobs piling up on busy branchescreate-next-appjob — replaced the inlinenode {0}+require()script with a portable bash loopecosystem-run.yml— all inline Node heredocs converted fromrequire()tonode --input-type=modulewithimportstatementsExceptions (intentionally kept inline)
bonk.yml/bigbonk.yml—issue_comment-triggered workflows; GHAS flags local composite actions reachable from these as untrusted-checkout TOCTOUtip.ymlnotify-on-failurejob — only needs plainsetup-node, no pnpm or install