Add reusable PR-preview/publish workflow family#34
Conversation
Centralizes the three-workflow Quarto PR-preview pipeline (build, deploy, cleanup) that d-morrison/rme carried as fully-inlined local workflows, so consumer repos call thin stubs instead of maintaining their own copy. Fixes to this pipeline (e.g. the rme#913 git-clean-ordering regression) now land once, centrally. Mirrors the repo's two-layer pattern: - preview/action.yml — composite action for the build half: checkout, write PR metadata *after* checkout (so `git clean -ffdx` can't wipe it from the artifact, per rme#913), Quarto/R/renv setup, apt deps, label-gated pdf/docx/revealjs renders, freeze cache, and artifact upload. Parameterized for non-rme consumers (R version, apt packages, renv on/off, local-package install, Chrome, submodules, render profile). - .github/workflows/preview.yml — reusable build workflow (read-only, fork context) wrapping the composite, with the concurrency cancel-in-progress and recognized-label gates preserved. - .github/workflows/preview-deploy.yml — reusable deploy workflow: on workflow_run completion, publishes the artifact to gh-pages and comments the preview link in the base-repo context. Kept split from the build half so untrusted fork code never holds write permissions (the trust boundary). - .github/workflows/cleanup-pr-previews.yml — reusable scheduled housekeeping that removes preview directories for closed PRs. Adds caller stubs under examples/, documents required caller permissions and the build-name/workflow_run wiring in the README, records the family in the table and CHANGELOG, and drops publish/preview from the "later" scope list. Refs #33. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BXDXuaTiRaLUud6ET7EJxZ
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - name: Deploy PR Preview | ||
| if: steps.meta.outputs.action == 'deploy' | ||
| uses: rossjrw/pr-preview-action@v1 |
There was a problem hiding this comment.
Supply-chain risk: third-party actions on mutable major tags
rossjrw/pr-preview-action@v1 (repeated on line 90) and marocchino/sticky-pull-request-comment@v3 (lines 100, 114) use major-version tags rather than pinned SHAs. The deploy job runs in the base-repo context with contents: write and pull-requests: write, so a tag-overwrite or repo compromise by any of these upstreams would have real impact.
Consider pinning to commit SHAs (with the version in a comment for readability), for example:
uses: rossjrw/pr-preview-action@6e624b14d5d44f41a6d0cb4bc3cc6f5c01a2a76b # v1.6.2browser-actions/setup-chrome@v1 in preview/action.yml line 95 has the same issue, though that job is read-only.
| if [ "$gh_exit" -ne 0 ]; then | ||
| # Distinguish "PR does not exist" from other failures | ||
| if echo "$gh_output" | grep -qi "could not resolve\|no pull requests found\|404\|not found"; then |
There was a problem hiding this comment.
Fragile: error-message grep is a GH CLI implementation detail
The pattern "could not resolve\|no pull requests found\|404\|not found" relies on the exact wording of GH CLI error messages, which can change across CLI versions without notice. If a message changes, a non-existent PR would fall into the generic "skip" branch rather than NOT_FOUND, potentially leaving stale previews indefinitely.
A more robust approach is to use gh api and distinguish on the HTTP status code:
http_code=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$pr_number" \
--jq '.state' \
-i 2>/dev/null | head -1 | awk '{print $2}') || true
if [ "$http_code" = "404" ]; then
pr_state="NOT_FOUND"
fiAlternatively, just treat any non-zero exit from gh pr view as "not found" for simplicity (accepting the risk of deleting previews on transient API errors), making the intent explicit with a comment.
| tinytex: false | ||
|
|
||
| - name: Set up R |
There was a problem hiding this comment.
Input type asymmetry (informational)
The reusable workflow (preview.yml) declares use-renv, install-package, and setup-chrome as type: boolean. GitHub coerces them to the strings 'true'/'false' when passing to this composite action, so the == 'true' comparisons work correctly.
Worth a one-line comment here for future maintainers:
# Boolean inputs from workflow_call are coerced to strings 'true'/'false'
use-renv: ${{ inputs.use-renv }}
- preview/action.yml: route `render-profile` through an `env:` var and quote
it in all four render steps, so a value with shell metacharacters can't be
injected into the render command (GitHub expands ${{ }} before bash runs).
- preview/action.yml: collapse the two quarto-actions/setup calls into one,
gating TinyTeX install on the `preview:pdf` label via an expression instead
of a second setup step (DRY; avoids a redundant Quarto install on PDF builds).
- preview-deploy.yml: use the built-in $GITHUB_REPOSITORY env var in the gate
step instead of an inline ${{ github.repository }} expression, matching the
repo's env-routing convention.
- cleanup-pr-previews.yml: detect a missing PR by the REST API's HTTP 404
status rather than grepping gh's human-readable error text, which is an
unstable CLI implementation detail; transient errors still skip deletion.
- preview.yml: note that the boolean workflow_call inputs are coerced to
'true'/'false' strings when passed to the composite action.
SHA-pinning of third-party actions is deferred to #35 (repo-wide policy; the
whole repo uses major tags today, so pinning only this family would be
inconsistent).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXDXuaTiRaLUud6ET7EJxZ
This comment has been minimized.
This comment has been minimized.
|
Thanks for the review — addressed all six findings in 670826b. Dispositions:
@claude review Generated by Claude Code |
|
👀 Picked up by workflow run #27800017401. Setup runs first; Claude itself responds after that. |
|
@claude review |
|
👀 Picked up by workflow run #27812339890. Setup runs first; Claude itself responds after that. |
|
Claude Code CLI (local session) is driving this PR to clean — paws off until I'm done. |
…-t1tdzt # Conflicts: # CHANGELOG.md # README.md
This comment has been minimized.
This comment has been minimized.
ARD summary — review of the completed reviewSynced the branch with
On #4 (SHA pinning) — rebut + deferThe concern is legitimate (the deploy job has
The right fix is a repo-wide policy decision (pin everything + add dependabot, or keep major tags everywhere), which is out of scope for "add the preview family." Tracked in #48. The merge push ( |
| key: quarto-freezer-${{ runner.os }}-${{ hashFiles('renv.lock') }}-${{ github.event.pull_request.head.sha }}-${{ github.run_attempt }} | ||
| restore-keys: | | ||
| quarto-freezer-${{ runner.os }}-${{ hashFiles('renv.lock') }}-${{ github.event.pull_request.head.sha }}- | ||
| quarto-freezer-${{ runner.os }}-${{ hashFiles('renv.lock') }}-main- |
There was a problem hiding this comment.
Minor: hardcoded main in freeze restore key
This fallback level will only warm the freeze from a base-branch build when the caller's default branch is literally main. Consumers with master or another default branch name skip straight to the broader renv.lock-only key.
github.event.pull_request.base.ref is available in the pull_request event context and gives the actual base branch name:
| quarto-freezer-${{ runner.os }}-${{ hashFiles('renv.lock') }}-main- | |
| quarto-freezer-${{ runner.os }}-${{ hashFiles('renv.lock') }}-${{ github.event.pull_request.base.ref }}- |
Non-blocking — the outer fallback keys still provide a reasonable warm start — but worth fixing before rolling out to master-based repos.
Replace the hardcoded `-main-` fallback level in the Quarto freeze
cache restore-keys with `${{ github.event.pull_request.base.ref }}`,
so consumers whose default branch isn't `main` still hit that fallback
instead of falling through to the broader renv.lock-only key.
No behavior change for rme (base ref is `main`).
Addresses the round-2 review finding on PR #34.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ARD summary — round 2 (re: follow-up review)The follow-up review confirmed all 6 original findings resolved and raised one new minor item.
The deferred SHA-pinning item is tracked in #48 (the pre-existing #35 was closed as a duplicate of it, so there's a single open canonical issue). Push |
|
Claude finished @d-morrison's task in 2m 47s —— View job Round 3 Review: Add reusable PR-preview/publish workflow familyTasks:
Verification of prior finding
The restore-key now reads: quarto-freezer-${{ runner.os }}-${{ hashFiles('renv.lock') }}-${{ github.event.pull_request.base.ref }}-
Full sweep — no new findingsReviewed the complete current state of all 9 changed files against the prior findings and for anything new:
VerdictAll six findings from round 1 and the one finding from round 2 are resolved. No new issues found. This PR is ready to merge. |
|
✅ Clean — done. PR is free (paws off lifted). Round-3 review verdict: "All six findings from round 1 and the one finding from round 2 are resolved. No new issues found. This PR is ready to merge." Zero flagged items under any heading; all required checks green ( The one deferred item — SHA-pinning third-party actions — is tracked repo-wide in #48. Not merging unless you ask. |
2 participants
Closes #33.
What
Centralizes the three-workflow Quarto PR-preview / publish pipeline that
d-morrison/rmecurrently carries as fully-inlined local workflows (preview.yml,preview-deploy.yml,cleanup-pr-previews.yml) into reusableworkflow_callworkflows + a composite action here, so consumer repos call thin stubs instead of maintaining their own copy. Fixes to the pipeline (e.g. the rme#913 git-clean-ordering regression) now land once, centrally.How it's structured
Mirrors this repo's existing two-layer pattern (composite action + reusable workflow):
preview/action.yml.github/workflows/preview.yml.github/workflows/preview-deploy.ymlworkflow_run-triggered; publishes the artifact togh-pagesand comments the preview link in the base-repo context..github/workflows/cleanup-pr-previews.ymlexamples/preview.yml,examples/preview-deploy.yml,examples/cleanup-pr-previews.ymlpermissions:blocks.Load-bearing pieces preserved (per the issue's design notes)
gh-pagespush happens in the separateworkflow_rundeploy half against base-repo code. Not collapsed into one job.git cleanordering — PR metadata is written afteractions/checkoutsogit clean -ffdxcan't wipe it from the artifact (rme#913).cancel-in-progress: true(rme#812); deploy keeps the per-branch serialized group withcancel-in-progress: false.preview:pdf/preview:docx/preview:revealjs, plusclear freezerfreeze-bypass.renv.lock+ head SHA.Parameterized for non-rme consumers
The build half takes inputs:
r-version,apt-packages(defaults to rme's exact list; set empty to skip),use-renv,install-package(R CMD INSTALL .),setup-chrome,submodules,render-profile. Cleanup takespreview-dir.Docs
name:⇄workflow_runwiring requirement and the default-branch caveat), extended the Permissions list, and dropped publish/preview from the "later" scope note.Notes / out of scope
_selftest.yml: the build half needs a real Quarto/R project (this repo isn't one) and deploy/cleanup needgh-pages+workflow_run, so a meaningful self-test isn't feasible here. The family will be exercised by the rme migration. Same caveat thecheck-phiselftest comment already documents for brand-new capabilities.REVDEPS.mdnot updated: the issue scopes that to "once rme migrates" — no consumer is registered yet.@v1, open the companion rme PR replacing its three local preview workflows with these stubs.🤖 Generated with Claude Code
https://claude.ai/code/session_01BXDXuaTiRaLUud6ET7EJxZ
Generated by Claude Code