chore: improvements to various ci workflows#300
Conversation
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
All six fixes look correct.
tip.yml — ${{ secrets.X != '' }} in an if: is evaluated as a string expression (always truthy), not a secrets check. Removing the ${{ }} wrapper lets GitHub evaluate it as a native workflow expression where secrets are accessible. Good catch.
benchmarks.yml — Each run: block starts in GITHUB_WORKSPACE, so the chained cd approach (cd benchmarks/nextjs && npm install; cd ../vinext && ...) was broken because each cd is relative to the repo root, not the previous directory. Actually — the original used newlines not &&, so each line ran from the repo root, meaning the first cd benchmarks/nextjs && npm install worked, but cd ../vinext resolved to <repo-root>/../vinext which doesn't exist. --prefix is the clean fix.
ci.yml — Without event_name in the concurrency group, a push to main (triggered by merging a PR) and a simultaneous PR run would share the same group key (ci-refs/heads/main vs ci-refs/pull/N/merge — wait, actually those refs are already different). The real scenario this fixes: a push to main triggers both a push event and can still have an in-flight pull_request event for the same ref. With event_name included, they no longer collide. Correct.
deploy-preview-command.yml — Two good fixes: --frozen-lockfile ensures reproducible installs in CI (was the only workflow missing it), and the if: always() && needs.check.result == 'success' && needs.deploy.result == 'success' on the comment job prevents posting preview URLs when the deploy actually failed. The always() is needed because without it the job would be skipped when any upstream job fails (default behavior), but with it plus the explicit success checks, it only runs when everything succeeded — which is the desired behavior.
preview-release.yml / publish.yml — Gating on repository_owner == 'cloudflare' prevents fork PRs from attempting to run pkg-pr-new (which would fail without write access) and prevents publish from running in fork contexts. Sensible guard.
Clean PR, all changes are minimal and targeted.
|
PR approved. All six workflow fixes are correct and well-targeted. Here's the summary:
|
tip.ymlspurious runs - invalid${{ }}wrapper on secretsif:check caused GitHub to fire the workflow on every pushcd ../vinextin arun:block resolves from repo root, not previous directory; usenpm install --prefixinsteadrepository_owner == 'cloudflare'ci.ymlconcurrency group to includeevent_name, preventing push and PR runs from cancelling each otherdeploy-preview-command.yml: add--frozen-lockfile; only post preview URLs when deploy succeeds