From cc552f06e2767742eba42416658405afb9d2e880 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 8 Jul 2026 09:56:49 +0200 Subject: [PATCH] feat(git-sync): PR deploy-preview comment, clearer check copy, app-only hints - The PR diff completion hook maintains one managed comment on the PR (Cloudflare deploy-preview style: workspace, status, commit, collapsible change list), upserted per synchronize via a hidden marker. The check run stays for required-check gating. - A settings difference in the diff summary is worded by cause: the PR changes wmill.yaml, vs pre-existing drift between the repo's wmill.yaml and the workspace, vs undetermined (neutral wording). - Deploy-status check titles name the target workspace ("Deployed 2 change(s) to staging"), since GitHub shows a head commit's checks on any PR containing it and a bare "Deployed" read as if the PR had deployed. - Token-based repos see a hint pointing at the open-pr-on-commit / open-pr-on-fork-commit workflows where the app-only PR toggles would be; an API-set toggle on a non-app repo now logs a warning naming the fallback; the design doc lists app-only features and their degradation. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PP5gBSPfo1YtkL1sWVAjJm --- backend/ee-repo-ref.txt | 2 +- .../windmill-worker/src/result_processor.rs | 55 ++++++++++++++++--- docs/git-sync-pull-design.md | 12 ++++ .../git_sync/GitSyncRepositoryCard.svelte | 20 +++++++ 4 files changed, 81 insertions(+), 8 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 0e2e2cd591..c61b9a65c8 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -a4f14db140b094e62514a6ed0d92f6f4daf2f8c6 +0a527a5378ef1781fd691d5d27669e63580c6eff diff --git a/backend/windmill-worker/src/result_processor.rs b/backend/windmill-worker/src/result_processor.rs index 8559937e2b..0e1d06c9fd 100644 --- a/backend/windmill-worker/src/result_processor.rs +++ b/backend/windmill-worker/src/result_processor.rs @@ -730,6 +730,14 @@ pub async fn handle_receive_completed_job( struct GitSyncCheck { check_run_id: i64, repo_url: String, + #[serde(default)] + pr_number: Option, + #[serde(default)] + head_sha: Option, + /// Whether the PR itself modifies wmill.yaml (None = undetermined); picks + /// the wording for a settings difference in the diff summary. + #[serde(default)] + wmill_yaml_changed: Option, } /// Parsed diff summary from a (dry-run or real) pull result. `None` when the @@ -985,8 +993,8 @@ async fn maybe_open_git_sync_deploy_pr(db: &DB, job_id: &uuid::Uuid, workspace_i { Ok(Some((branch, _))) => branch, Ok(None) => { - tracing::debug!( - "git sync PR: repo {repo_path} in {workspace_id} is not app-backed, skipping" + tracing::warn!( + "git sync PR: repo {repo_path} in {workspace_id} has a PR-on-deploy toggle set but is not GitHub-App-backed; skipping (connect the repo through the GitHub App, or use the open-pr-on-commit workflow)" ); return; } @@ -1079,7 +1087,7 @@ async fn maybe_post_git_sync_check( if !success { ( "failure", - "Deploy failed".to_string(), + format!("Deploy to {} failed", workspace_id), "Deploying the latest commit failed. See the job in Windmill for details." .to_string(), ) @@ -1087,7 +1095,7 @@ async fn maybe_post_git_sync_check( match parse_git_sync_changes(result_raw) { Some((changes, settings_changed)) if changes.is_empty() && !settings_changed => ( "success", - "In sync".to_string(), + format!("In sync with {}", workspace_id), format!( "No changes to deploy to `{}` from this commit.", workspace_id @@ -1105,13 +1113,13 @@ async fn maybe_post_git_sync_check( } ( "success", - format!("Deployed {} change(s)", changes.len()), + format!("Deployed {} change(s) to {}", changes.len(), workspace_id), lines.join("\n"), ) } None => ( "success", - "Deployed".to_string(), + format!("Deployed to {}", workspace_id), format!("Windmill deployed the latest commit to `{}`.", workspace_id), ), } @@ -1139,7 +1147,11 @@ async fn maybe_post_git_sync_check( )]; lines.extend(format_change_list(&changes)); if settings_changed { - lines.push("\nWorkspace settings would also change.".to_string()); + lines.push(match check.wmill_yaml_changed { + Some(true) => "\nThis PR changes wmill.yaml: pulling also applies the updated workspace settings.".to_string(), + Some(false) => "\nIndependent of this PR, the workspace's git-sync settings differ from the repo's wmill.yaml and a pull updates them to match.".to_string(), + None => "\nA pull also updates the workspace's git-sync settings to match the repo's wmill.yaml.".to_string(), + }); } ( "neutral", @@ -1169,6 +1181,35 @@ async fn maybe_post_git_sync_check( { tracing::error!("git sync-check: failed to update check run: {e:#}"); } + + // Phase 4 also maintains ONE managed comment on the PR (Cloudflare + // deploy-preview style): upserted on every synchronize, so reviewers see the + // current diff without opening the Checks tab. + if !is_deploy { + if let Some(pr_number) = check.pr_number { + let marker = ""; + let head = check + .head_sha + .as_deref() + .map(|s| &s[..s.len().min(7)]) + .unwrap_or("latest"); + let body = format!( + "{marker}\n### Windmill deploy preview\n\n | | |\n|---|---|\n | **Workspace** | `{workspace_id}` |\n | **Status** | {title} |\n | **Commit** | `{head}` |\n\n
Details\n\n{summary}\n\n
" + ); + if let Err(e) = windmill_common::git_sync_ee::upsert_pr_comment( + db, + workspace_id, + &check.repo_url, + pr_number, + marker, + &body, + ) + .await + { + tracing::warn!("git sync-check: failed to upsert PR diff comment: {e:#}"); + } + } + } } pub async fn process_completed_job( diff --git a/docs/git-sync-pull-design.md b/docs/git-sync-pull-design.md index 1c326dcbb4..2be120d0c0 100644 --- a/docs/git-sync-pull-design.md +++ b/docs/git-sync-pull-design.md @@ -253,6 +253,14 @@ every existing installation's org admin): | Pull requests: write | instance-side PR creation (promotion/forks) | 2 | | Checks: write | PR diff preview checks | 3 | +App-only features and their fallbacks: instant webhook sync (falls back to +polling), in-app PR creation for deploy branches (fall back to the +`open-pr-on-commit` / `open-pr-on-fork-commit` workflows; the toggles are +hidden for token repos and a set-but-inert toggle logs a warning), and the PR +diff comment/check + deploy status check (no fallback: they need the Checks +API and `pull_request` webhook deliveries). Token/PAT repositories keep the +full pull direction via polling. + Rollout behavior: until an org approves, webhook creation fails with a distinguishable error → the instance shows "approval pending" and stays on polling. Nothing breaks; latency is the only cost. The self-managed (GHES) @@ -451,6 +459,10 @@ Frontend: - Subscribe `pull_request` events; on open/synchronize run the existing `dry_run: true` pull and post the diff as a check run (`checks: write`). +- The same completion hook maintains ONE managed comment on the PR + (Cloudflare deploy-preview style: workspace, status, commit, collapsible + change list), upserted per synchronize via a hidden `` + marker so reviewers see the current diff without opening the Checks tab. ### Phase 5 — fork sync, configured at the parent (replaces the `push-on-merge-to-forks` GitHub Action) — implemented diff --git a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte index 8992e2fe9d..729687bb45 100644 --- a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte +++ b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte @@ -580,6 +580,16 @@ on:change={(e) => setPromotionOpenPrs(e.detail)} /> + {:else if repoMode === 'promotion' && !repo.isUnsavedConnection} +
+ Opening pull requests from Windmill uses the + GitHub App. Token-based repositories can use the + open-pr-on-commit workflow instead. +
{/if} @@ -718,6 +728,16 @@ }} on:change={(e) => setForkOpenPrs(e.detail)} /> + {:else} +
+ Opening pull requests for fork deploys uses the + GitHub App. Token-based repositories can use the + open-pr-on-fork-commit workflow instead. +
{/if} {/if}