[CI]: Restructure and Standardize the preview-deploy workflow across repos subscribed to the meshery-academy topic. - #36
Conversation
Signed-off-by: Lenox Wiltshire <lenoxwiltshire@gmail.com>
|
🚀 Preview deployment: https://meshery-extensions.github.io/tcslabs-academy/pr-preview/pr-36/
|
|
Hey @banana-three-join, this is a really clean approach! I've been learning a lot about the Quick question — does The |
ianrwhitney
left a comment
ianrwhitney
left a comment
There was a problem hiding this comment.
a few comments for your consideration. Also need to look into making these reusable so that all repos that require this can benefit.
| } > artifact/pr.env | ||
|
|
||
| - name: Checkout PR code | ||
| if: github.event.action != 'closed' |
There was a problem hiding this comment.
Instaad of having all these steps with != closed. Can this functionality be in a different workflow
There was a problem hiding this comment.
I've abstracted away build, deploy and clean into different workflows as requested.
| > *GitHub Pages may take a moment to publish — refresh if it 404s.* | ||
|
|
||
| # Notify any PRs whose previews were pruned to stay under the limit. | ||
| - name: Comment on pruned previews |
There was a problem hiding this comment.
we probably want best effort on clean up and not fail a workflow if this has an error. So something like continue-on-error: true
| path: gh-pages | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Publish preview and prune |
There was a problem hiding this comment.
The other workflow was using this action rossjrw/pr-preview-action. Does this help simplify anything for us?
There was a problem hiding this comment.
In my most recent approach, I've opted to use the SHA instead of using the pr-number, that approach isn't possible through the usage of rossjrw/pr-preview-action. Also, in their README, they state that the action shouldn't work with forks but it can be bypassed so if the approach of using the SHA instead of the pr-number doesn't bring us that much value, I can revert this behavior.
…act their logic into separate centralizable actions Signed-off-by: Lenox Wiltshire <lenoxwiltshire@gmail.com>
|
The PR description (and issue #35) both call for building via the shared |
There was a problem hiding this comment.
@banana-three-join Overall, this looks like a solid refactor. I like the separation of the preview pipeline into dedicated build, deploy, and cleanup workflows—it follows GitHub's recommended trust separation and makes the responsibilities much clearer.
A few things that stood out to me:
- The least-privilege permission model is applied consistently across the workflows.
- Using pinned action SHAs is a nice security improvement.
- The build/deploy separation with
workflow_runand scoped artifact retrieval makes the flow much safer than the previouspull_request_targetapproach. - The retry logic around
git pushand the artifact sanity checks are thoughtful additions.
I left one small suggestion regarding treating the PR notification step in preview-clean.yml as best-effort so that a transient GitHub API failure wouldn't cause an otherwise successful cleanup to be marked as failed. Other than that, I didn't notice any major concerns.
Nice work on the refactor and on documenting the design—it made the workflow much easier to review.
| echo "closed_prs=$(to_json "${RUNNER_TEMP}/closed.txt")" >> "$GITHUB_OUTPUT" | ||
| echo "pruned_prs=$(to_json "${RUNNER_TEMP}/pruned.txt")" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Notify affected pull requests |
There was a problem hiding this comment.
The cleanup completes before the notification step. Would it make sense to treat the PR notifications as best-effort (for example with continue-on-error: true on this step)? That way, a transient GitHub API failure wouldn't cause the workflow to be marked as failed after the previews have already been reconciled.
ishwar170695
left a comment
ishwar170695
left a comment
There was a problem hiding this comment.
Reviewed the latest changes. The split into dedicated build, deploy, and cleanup workflows makes the trust boundaries much clearer, and I didn't find any issues after walking through the main race conditions and failure scenarios. Looks good from my side. 👍
|
Hey @banana-three-join The PR description says deploy pushes use a single global concurrency group so gh-pages writes serialize, but in the code changes this is scoped per repo/branch, not global. So two different PRs deploying at the same time wouldn't actually be serialized by this, they'd just race on gh-pages and rely on the retry loop (fetch/reset/push with retries) to sort it out. |
KumarNirupam1
left a comment
KumarNirupam1
left a comment
There was a problem hiding this comment.
I went through the restructure.
the build / deploy / clean split looks good to me. untrusted build on pull_request, privileged deploy on workflow_run with the artifact handoff, and cleanup is separate. least privilege and pinned actions also look good.
just a couple of things i wanted to clarify before i approve this:
- preview-build.yml takes repo-name, but i don't see it being used anywhere in the build workflow. only deployment-url is used. is repo-name there for future reusable workflows or is it just leftover?
- the issue/pr description mentions make setup -> make build-preview, but the workflow now builds with npm ci + hugo (using --environment dev). was that an intentional change, or do we still want preview builds to go through the make targets so they stay in sync with production?
also on concurrency, the deploy group is per head repo/branch, not global. so different pr deploys can still race on gh-pages and rely on the retry logic. that's fine if it's intentional, just wanted to confirm.
|
@banana-three-join Thank you for your contribution! Let's discuss this during the website call today (20 July) at 5:30 PM IST | 7 AM CST Add it as an agenda item to the meeting minutes, if you would 🙂 |
|
|
||
| jobs: | ||
| clean: | ||
| if: contains(fromJSON('["pull_request_target", "schedule", "workflow_dispatch"]'), github.event_name) |
There was a problem hiding this comment.
One behaviour change I would like to confirm: retention enforcement. In the old deploy-preview.yml, the "keep newest N" limit was pruned on every deploy (the Prune old PR previews step, gated on action != 'closed'). In the new split, preview-deploy.yml doesn't prune at all; retention lives only in preview-clean.yml, which is invoked (via preview-cleanup-pr.yml) only on pull_request_target: [closed] / workflow_dispatch. So retention only runs when a PR closes (or manual dispatch), not on deploy.
Consequence: with several long-lived open PRs and no close events, the live preview count on gh-pages can exceed the limit until something closes, the exact size-limit condition retention was meant to prevent. This guard also allows scheduling, but nothing wires up a scheduled trigger; was a periodic cron reconcile intended to enforce retention independently of closes? It may not have made it into preview-cleanup-pr.yml.
ianrwhitney
left a comment
ianrwhitney
left a comment
There was a problem hiding this comment.
A few more comments for your consideration. thanks
Signed-off-by: Lenox Wiltshire <lenoxwiltshire@gmail.com>
📝 WalkthroughWalkthroughThe legacy preview deployment workflow is replaced with separate pull-request build, artifact deployment, and preview cleanup workflows. Builds run under ChangesPR preview pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant BuildWorkflow
participant ArtifactStorage
participant DeployWorkflow
participant GitHubPages
participant PRComment
PullRequest->>BuildWorkflow: trigger preview build
BuildWorkflow->>ArtifactStorage: upload preview-site
BuildWorkflow->>DeployWorkflow: successful workflow run
DeployWorkflow->>ArtifactStorage: download preview-site
DeployWorkflow->>GitHubPages: publish PR preview
DeployWorkflow->>PRComment: create or update preview URL
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
|
@ianrwhitney Comments have been addressed, thanks for the feedback! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/preview-clean.yml:
- Around line 60-65: Update the PR state lookup in the cleanup loop so transient
failures from gh api are retried or treated as an unavailable lookup, not as a
closed PR. Only remove the preview directory when the API confirms the PR state
is closed or the PR is explicitly missing with a 404; skip the entry for rate
limits, network errors, and other lookup failures, preserving the existing
cleanup for confirmed non-open states.
In @.github/workflows/preview-deploy-pr.yml:
- Around line 10-12: Update the workflow’s concurrency configuration to use one
global group for all preview deployments targeting the shared gh-pages branch,
rather than grouping by repository and branch. Set cancel-in-progress to false
so concurrent PR deployments queue instead of canceling or racing each other,
while preserving the existing retry behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 512314f7-95fd-4806-b12f-e671ef8935f1
📒 Files selected for processing (7)
.github/workflows/deploy-preview.yml.github/workflows/preview-build-pr.yml.github/workflows/preview-build.yml.github/workflows/preview-clean.yml.github/workflows/preview-cleanup-pr.yml.github/workflows/preview-deploy-pr.yml.github/workflows/preview-deploy.yml
💤 Files with no reviewable changes (1)
- .github/workflows/deploy-preview.yml
| state="$(gh api "repos/${GH_REPO_FULL}/pulls/${n}" --jq .state 2>/dev/null || echo missing)" | ||
| if [[ "$state" != "open" ]]; then | ||
| echo "Removing preview for PR #${n} (state: ${state})" | ||
| rm -rf "$dir" | ||
| echo "$n" >> "${RUNNER_TEMP}/closed.txt" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A transient gh api failure is treated the same as "PR closed", deleting live previews.
gh api ... 2>/dev/null || echo missing maps any failure (rate limiting, network blip, transient 5xx) to state=missing, which is then treated identically to a closed/deleted PR and the preview directory is removed. An open PR's preview can be wrongly pruned on a transient error, forcing the author to push a new commit to regenerate it. Distinguish "confirmed not open" (e.g. explicit closed/404) from "lookup failed" (retry or skip that entry this run) before deleting.
🐛 Proposed fix sketch
- state="$(gh api "repos/${GH_REPO_FULL}/pulls/${n}" --jq .state 2>/dev/null || echo missing)"
- if [[ "$state" != "open" ]]; then
+ if ! state="$(gh api "repos/${GH_REPO_FULL}/pulls/${n}" --jq .state)"; then
+ echo "Could not determine state for PR #${n}; skipping this run"
+ continue
+ fi
+ if [[ "$state" != "open" ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state="$(gh api "repos/${GH_REPO_FULL}/pulls/${n}" --jq .state 2>/dev/null || echo missing)" | |
| if [[ "$state" != "open" ]]; then | |
| echo "Removing preview for PR #${n} (state: ${state})" | |
| rm -rf "$dir" | |
| echo "$n" >> "${RUNNER_TEMP}/closed.txt" | |
| fi | |
| if ! state="$(gh api "repos/${GH_REPO_FULL}/pulls/${n}" --jq .state)"; then | |
| echo "Could not determine state for PR #${n}; skipping this run" | |
| continue | |
| fi | |
| if [[ "$state" != "open" ]]; then | |
| echo "Removing preview for PR #${n} (state: ${state})" | |
| rm -rf "$dir" | |
| echo "$n" >> "${RUNNER_TEMP}/closed.txt" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/preview-clean.yml around lines 60 - 65, Update the PR
state lookup in the cleanup loop so transient failures from gh api are retried
or treated as an unavailable lookup, not as a closed PR. Only remove the preview
directory when the API confirms the PR state is closed or the PR is explicitly
missing with a 404; skip the entry for rate limits, network errors, and other
lookup failures, preserving the existing cleanup for confirmed non-open states.
| concurrency: | ||
| group: preview-deploy-pr-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Concurrency group is scoped per PR/branch, so different PRs' deploys can still race on gh-pages.
group: preview-deploy-pr-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} only serializes deploys for the same PR. Two different PRs completing their builds around the same time can run this job concurrently, and both jobs will attempt git push origin gh-pages against the shared branch. preview-deploy.yml's 5-attempt retry-with-backoff loop reduces but does not eliminate the chance of exhausting retries under heavier concurrent-PR load, so writes aren't truly serialized as the PR description states. A single global group (e.g. preview-deploy-pr-gh-pages) with cancel-in-progress: false would queue rather than race concurrent PR deploys, while still needing the retry loop as a safety net.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/preview-deploy-pr.yml around lines 10 - 12, Update the
workflow’s concurrency configuration to use one global group for all preview
deployments targeting the shared gh-pages branch, rather than grouping by
repository and branch. Set cancel-in-progress to false so concurrent PR
deployments queue instead of canceling or racing each other, while preserving
the existing retry behavior.
|
@banana-three-join Great work on the refactor! I went through all 7 files in detail. The trust separation, pinned SHAs, and artifact-based handoff are exactly what we need. A few observations from my review: What looks solid:
One thing to consider:
Overall this is really well structured. Happy to help with the rollout across the other repos once this lands! |
ianrwhitney
left a comment
ianrwhitney
left a comment
There was a problem hiding this comment.
There also has to be some throught into if these workflows are centralized how is the workflow_run going to be triggered by the consuming repo's workflow.
| `updated previews on GitHub Pages.\n\n` + | ||
| `Push a new commit to regenerate it.\n${marker}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This has a big surface area and alternative approaches to cleanup or ways to simplify? Any precedent we can follow from others out there.
| run: | | ||
| set -euo pipefail | ||
| [[ "$HEAD_SHA" =~ ^[0-9a-f]{40}$ ]] || { echo "Invalid head SHA"; exit 1; } | ||
| matches="$(gh api --paginate "repos/${GH_REPO_FULL}/pulls?state=open&per_page=100" \ |
There was a problem hiding this comment.
If your going to call the gh api here and santize the string, you could just get the PR itself from the authorative source gh api. I dont think need to go that far since we use, we can simplify
| path: ${{ runner.temp }}/preview-site | ||
|
|
||
| - name: Artifact sanity checks | ||
| if: steps.pr.outputs.number != '' |
There was a problem hiding this comment.
If your going to have checks at every step just exit in the above script if the value you expect isnt there.
| run: | | ||
| set -euo pipefail | ||
| test -f "${SITE_DIR}/index.html" || { echo "No index.html at artifact root"; exit 1; } | ||
| if find "${SITE_DIR}" -type l | grep -q .; then |
There was a problem hiding this comment.
If this is a security check, probably just exit if anything is found vs santize we wont go forward with a contrubtion in which this occured
| issue_number: prNumber, | ||
| body, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Is there any existing action in the marketplace that can be used in any of these custom scripts?
| name: preview-cleanup-pr | ||
|
|
||
| on: | ||
| pull_request_target: |
| types: [closed] | ||
| workflow_dispatch: | ||
|
|
||
| permissions: {} |
There was a problem hiding this comment.
If the wrapping workflow doesnt request permissions the underneath workflow cannot elevate those permissions.
permissions:
contents: write # Push removals to gh-pages.
pull-requests: write # Notify PRs whose preview was removed.
|
|
||
| jobs: | ||
| build: | ||
| if: github.event_name == 'pull_request' |
There was a problem hiding this comment.
I may be mistaken, but I suspect this condition could always evaluate to false, which would cause the reusable workflow to be skipped.
My understanding is that when a workflow is invoked through workflow_call, the event name inside the called workflow becomes workflow_call, not pull_request.
If that is the case, then:
if: github.event_name == 'pull_request'would evaluate to false, and the build job would be skipped whenever the reusable workflow is invoked.
Even when the caller workflow itself is triggered by on: pull_request, I believe the called workflow still sees:
github.event_name == 'workflow_call'so the condition above would remain false.
Could we confirm this behavior? If it is correct, it might be better to remove the condition and let the caller workflow decide when to invoke the reusable workflow.
|
merge conflict |
|
@marblom007 The merge conflict is from my most recent update to the |
12 participants
Notes for Reviewers
This PR fixes #35
Summary
Replaces the single
preview-deployworkflow with two workflows that follow GitHub's recommended pattern for previewing pull requests: an unprivileged build stage and a privileged deploy stage that communicate through an artifact. This also standardizes the preview build on the sharedmaketargets, so previews are produced the same way as production.preview-build.yml— builds the sitepreview-deploy.yml— publishes the built site togh-pagesHow the two workflows work together
Build (
preview-build.yml) runs onpull_request. It builds the site through the shared pipeline (make setup→make build-preview, with the preview base URL injected viaDEPLOY_PRIME_URL) and records the PR number and intended action (deploy/remove) in a smallpr.envfile. The builtpublic/output andpr.envare uploaded together as an artifact. This stage never touchesgh-pages.Deploy (
preview-deploy.yml) runs onworkflow_run, triggered when the build completes. It downloads the artifact from the specific build run that triggered it (scoped byrun-id), reads and validatespr.env, then checks outgh-pagesand publishes the static files topr-preview/pr-<number>/. It applies preview retention (keeps the newestPREVIEW_RETENTION_LIMIT), pushes once, and posts the preview-URL comment (and prune notices).Because the deploy fetches the artifact by the triggering run's ID and reads the PR number from inside that artifact, concurrent PRs never cross wires — each deploy is bound to its own build. All
gh-pageswrites share a single deploy-side concurrency group so pushes serialize instead of racing.How this aligns with GitHub's standards
pull_request; privileged publishing runs separately underworkflow_run, with the artifact as the handoff — the pattern GitHub documents for pull request previews.contents: read; the deploy workflow requests onlycontents: write,pull-requests: write, andactions: read. No workflow relies on broad default token permissions.actions/checkoutforward-compatibility. The build no longer performs a PR-ref checkout under a privileged trigger, so it is unaffected by GitHub's upcoming strictercheckoutdefaults forpull_request_target/workflow_run.maketargets and the Node-managed Hugo, matching production. The ad‑hoc.debHugo install and Dart Sass install are removed. Go is set up fromgo.mod, since the theme is a Hugo module.gh-pageswrites).Behavior preserved
pr-preview/pr-<number>/Signed commits
Summary by CodeRabbit
New Features
Bug Fixes
Chores