mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-24 08:01:38 +00:00
0258f3f81b96bb8d4e343ba8aeba614f9c836579
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ruben Fiszel
|
f365929eaa |
feat(fork): merge a fork deletion on evidence, not on the counters (#10484)
* feat(fork): merge a fork deletion on evidence, not on the counters `workspace_diff.ahead`/`.behind` record that a write happened on a side, not what it was or who made it. That leaves one row shape undecidable: an item the parent has and the fork does not can mean the parent added it, the fork deleted it, or a git-sync pull reverted a deploy that had just brought it in. #10467 kept every such row out of the merge direction, which killed the phantom but also dropped the only way to propagate a fork-side deletion and left a rename's old path behind in the parent. Record the evidence instead: - `workspace_diff` gains, per side, the last event's kind (`write` / `delete` / `rename_from`) and origin (`authored` / `sync`). Rows written before the migration have neither and keep #10467's behavior. - The kind is probed from whether the path still holds an item once the write has committed; an item kind the probe doesn't map records no evidence rather than a deletion. Create and update are not split — nothing at that point tells them apart for every kind, and the comparison already recomputes existence per side. - The origin comes from an `X-Windmill-Deploy-Origin` header the API scopes into a task-local for the request. It is the load-bearing half: recording `delete` alone would read a git-sync revert as a fork deletion and reproduce the original bug. Two clients set it — `wmill sync push` (which the git-sync auto-pull runs inside a job) and the compare page's parent→fork "Update fork". Merging the other way stays authored so a deletion keeps propagating up a fork chain. - The merge direction admits a parent-only row only when the fork's last event was an authored delete or rename-away. Such a row stays opt-in, never bulk-selected, and reads "Removes in <parent>"; the update direction keeps offering it back as "New". A fork deletion and a rename now merge into the parent, a rename leaves no duplicate behind, and a fork the parent also edited surfaces in both directions instead of the parent silently winning. Fixes WIN-2289 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): address review — detached tallies, enum wire values, doc duplication Codex P1: a dependency job tallies its deploy whenever it happens to finish, and the event kind is probed from the state at that moment. If anything removed the path in between (a git-sync revert), the stale tally read that deletion as its own and filed it as authored — handing the merge exactly the removal this is meant to withhold. `tally_deployed_object_changes` now takes `Option<DeployOrigin>`; `None` bumps the counter and leaves the evidence columns as the last vouching tally left them, and the worker path passes it. Covered by extending the removal-origin test: a detached tally after the sync archive must not disturb `(delete, sync)`. Also from review: - `fork_removed_it` compares through `DeployOrigin::as_str()` / `DeployEventKind::as_str()` rather than repeating their wire values, so a renamed variant can't silently make the predicate always false. - `deploy_origin`'s module doc no longer claims `sync` is inert: it cannot make the merge propose a removal, but it does drop a row out of both sides of the `all_ahead_items_visible` comparison. - `WorkspaceDiffRow` says why only the fork half of the evidence is consumed. - The delete-vs-revert rationale is stated once (the migration) instead of restated in eight files. - `PATH_KEYED_TABLES` is swept by a test: its query is built at runtime, so a wrong table name is not a compile error and would only surface as a failed tally for that trigger kind in a fork. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): let only a request task vouch for a deploy event Round 2 found the first fix incomplete. Detaching only the failed/cancelled dependency path left the common route untouched: a dependency job that succeeds calls `handle_deployment_metadata` from the worker, where `deploy_origin::current()` read as `Authored`. A sync archiving the script while its lock generation was pending then had its deletion probed on completion and refiled as authored — the same fabricated removal, on the path most deploys actually take. `current()` now returns `Option`, `Some` only inside the request scope the API always enters. Having no scope means "not the task that served this write", which is true of every worker-side call and needs no marking at the call site. The integration test drives the real `handle_deployment_metadata` off a request task instead of the tally directly, and fails without this. Two more from the same round: - The script dependency handler passed no `renamed_from`, unlike the flow and app handlers next to it. A lock-generating create has no earlier tally, so that was the only chance for the path a rename vacated to be recorded at all — renames of Python/TS scripts left the old path in the parent, which the bash-only manual check missed. - The tally now drops a `renamed_from` equal to the path itself. Callers pass the previous path whether or not the deploy moved the item, so an unfiltered one both counted the path twice and stamped it `rename_from` when nothing was renamed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): carry a deploy's origin into the dependency job it queues Round 3 caught the previous fix cutting too deep. Refusing a detached tally any claim also refused its rename evidence, and a lock-generating deploy has no other tally — so the `renamed_from` added alongside it was inert, and a renamed flow, app or Python script still left its old path in the parent with nothing to merge. Flows and apps always generate, so renames worked essentially nowhere. The two capabilities are now separate. `TallyEvidence` says whether the tallying task served the write (`Served`, may probe what the path holds now) or is reporting one that committed earlier (`Deferred`, may not), and each column is written only from a source that answers for it. The origin itself is a fact of the deploy either way, so the request stamps it into the dependency job's args and the worker re-enters the scope with it — the last place that knows it handing it to the only tally that will run. Also from round 3: `WorkspaceDiffRow`'s event fields skip serializing `None` rather than emitting `null`, matching what the schema declares (OpenAPI 3.0.3 ignores a `description` sibling of `$ref`, so those moved onto the shared schemas). Verified against a live worker: renaming a flow in a fork records `(rename_from, authored)` on the vacated path and the merge offers its removal, while the deployed path claims nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): mark the CLI's parent-to-fork merge as sync `wmill workspace merge --direction to-fork` is the CLI's "Update fork" and deletes items in the fork, but without the marker the compare page sets. Its deletions were recorded as authored fork decisions, so once the parent recreated such a path the merge would offer deleting it there. Also from review: an unrecognized deploy-origin arg now reads as no evidence rather than as authored — strict where a request header is lenient, since an unmarked request really is authored but an unreadable stored value is skew. Reading the arg moved next to `stamp_origin_arg`, the half that writes it, so the round trip a lock-generating deploy depends on is covered by one test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: drop the imports the shared arg reader made unused CI compiles with `-D warnings`, so this was four red Backend jobs rather than a lint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): stop a stale deferred rename from restating a removed path Nothing orders these events. A tally that served the write made its claim inside its own commit, but a deferred one reports a write that landed at an unknown remove. So a lock-generating rename whose dependency job finished after a sync had removed the vacated path could overwrite `(delete, sync)` with `(rename_from, authored)` — the path is gone either way, so the merge would then offer removing it from the parent on the strength of the older event. A deferred claim now only writes where the side has none, which is the case it exists for: a vacated path that nothing else has spoken for. The regression asserts the ordering directly, and fails without the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): record a rename's vacated path from the request that made it The deferred mechanism could not be made correct, as round 7 showed: its guard protected an existing row, but that row is deleted as soon as the two workspaces agree on the path — so a rename job finishing after the reconciliation inserted fresh, and the stale claim reappeared against whatever the parent later recreated there. Ordering cannot be recovered outside the row, because the row is disposable. So the vacated path is now recorded by the request, which is inside its own commit and whose row shares the counter's lifetime. A deploy that hands its metadata to a dependency job — every flow and app, and any script needing a lock — calls `tally_rename_vacated_path` once its transaction has committed; scripts reach it through the post-commit hook they already had, which grew a second variant rather than new plumbing. That lets the whole deferred apparatus go: `TallyEvidence`, the origin job arg and its round trip. `deploy_origin::current` is `Some` only inside a request scope again, and `handle_deployment_metadata` hands `renamed_from` to the tally only when it can answer for it — git-sync still gets it either way, so the rename keeps naming itself in the commit message. The vacated path's kind now reads `delete` rather than `rename_from` for these deploys, since it is probed rather than declared. The merge treats the two alike; only the row's tooltip is less specific. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(fork): cover raw-app renames, and stop firing CI before the lock exists Two things the vacated-path call broke or missed: - `create_script` reads its third return value as "no lock generation needed" to decide whether the script is runnable now, and the new `VacatedPath` variant made that true for renames that do generate. Those fired dependent CI tests from the API against a version with no lockfile, and again from the dependency job. The variant now decides it explicitly. - Raw apps rename through `update_app_raw`, a separate route into `update_app_internal`, which the new call had not been attached to. Both routes now go through one helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(fork): assert the kind only an inline rename can record `rename_from` is what a deploy says when it knows it moved the item, which only the path that reports both halves from its own request can. Nothing pinned it, and that is the side the vacated-path change touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: update ee-repo-ref to a45bec03922d305aad5893ed354dc029c7f97bb4 This commit updates the EE repository reference after PR #709 was merged in windmill-ee-private. Previous ee-repo-ref: 62f494b2a51de0dfc0cfa0c3530ff19a1d32667c New ee-repo-ref: a45bec03922d305aad5893ed354dc029c7f97bb4 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com> |
||
|
Ruben Fiszel
|
f9a547b8b8 |
fix(forks): record fork changes that never reached the diff tally (#10403)
* fix(forks): tally fork changes when a deploy's lock job fails Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(forks): tally fork changes even when deploy_to is unset Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: bump ee-repo-ref to the fork tally companion commit Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(forks): never tally a dependency deploy twice Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(forks): pin the ahead tally for a fork with no deploy_to Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(forks): cover the dedup case and commit the offline query cache Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(forks): let the tally settle before asserting the dedup count Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(forks): key the ahead tally on the fork lineage, not deploy_to Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: update ee-repo-ref to caf6abc45afd910620ef66f7550c076c18ca4589 This commit updates the EE repository reference after PR #693 was merged in windmill-ee-private. Previous ee-repo-ref: d5c6ee8b774993aa341196746d141107aa4567d1 New ee-repo-ref: caf6abc45afd910620ef66f7550c076c18ca4589 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com> |
||
|
Ruben Fiszel
|
04a08976ae |
fix: batch encryption-key rotation into one git-sync job (#9355)
* fix: trigger git sync for re-encrypted secrets on encryption key change When changing a workspace encryption key, the secret variables get re-encrypted with the new key, but the git sync was only dispatched for the encryption_key.yaml metadata file. Repos with Secrets sync enabled were left with stale ciphertexts until the next per-variable deployment. Now, after the transaction commits, we also dispatch a Variable git sync event for each re-encrypted secret so the new encrypted values are pushed to the configured repos. Errors are logged but don't roll back the key rotation. Fixes WIN-1994 Fixes #9344 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: batch encryption-key rotation into one git-sync job Workspace encryption key rotation now re-encrypts every secret variable and then dispatches a single batched git-sync job carrying the Key event plus one Variable item per re-encrypted secret. Repos with Secrets sync enabled receive every new ciphertext in one commit instead of nothing (previously only `encryption_key.yaml` was pushed) — and instead of N separate jobs the debouncer might or might not merge. Wires through the new `handle_deployment_metadata_batch` entry point added in the companion EE PR; OSS has a no-op shim so the build stays green. Adds an integration test (`workspace_encryption_key_git_sync`) asserting that rotating the key with 3 secret variables in scope produces exactly one deployment-callback job whose `items` array contains the Key event + all 3 variable entries and `skip_secret=false`. Fixes WIN-1994 Fixes #9344 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump ee-repo-ref for git-sync helper simplification Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: cover non-debouncing git-sync fallback on key rotation Adds a regression test exercising a workspace whose sync script predates hub version 28103: the rotation must still queue a legacy-format deployment-callback job per item (encryption_key + each re-encrypted secret) instead of silently skipping the repo. Bumps ee-repo-ref to the EE fallback fix. Addresses the P1 raised in the PR review (Codex/Pi/Claude): batch path dropped git sync entirely for repos without sync-job debouncing support. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: add sqlx offline cache for encryption-key git-sync test queries The cargo_test CI job builds with SQLX_OFFLINE=true; the two new sqlx::query!/query_as! calls in windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs had no cached entries, failing the build with E0282. Regenerated and added only the two new query caches (no EE/feature cache loss). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump ee-repo-ref to updated EE companion PR (08e3b9b) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
 wendrul
|
998f11a10d |
fix: visibility bug on deployment UI (issue when renaming items) + add tracking of folders and resource types (#7739)
* fix: Raw apps deployment UI (and merge UI) * Add folders and resource tpyes to merge UI * claude first pass on adding the new arg for h_deploy_metadata * Add missing argument to handle_deployment_metadata in all its calls * Add support for folders and resource types in merge UI * Update eereporef for CI * Update ee repo * Add migration to reset cached diff with potential artifacts * fix type in frontend * Preapare sqlx * Remove unused import and logs * update ee-repo * Update eerepo * chore: update ee-repo-ref to aca38475afd2cafaf63f4bbffc65be9437d57d86 This commit updates the EE repository reference after PR #397 was merged in windmill-ee-private. Previous ee-repo-ref: 19c64cf8c61d83f45047b37660054b29658cd403 New ee-repo-ref: aca38475afd2cafaf63f4bbffc65be9437d57d86 Automated by sync-ee-ref workflow. * Make integration test for workspace comparisons * Update SQLx metadata --------- Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com> |
||
|
wendrul
|
192fecc86f |
fix: create git branch right before creating the workspace fork to catch errors and have a coherent fork point (#7073)
* Workspace forks: add endpoint to create a branch before creating a fork * Update hubPaths + create branch before creating fork on frontend * Update tmp ee-repo-ref * Remove debug hubPath * Prepare sqlx * Fix ee imports * Update ee-ref * Update ee-repo-ref final * Prepare sqlx |
||
 Diego Imbert
|
0e316239dd |
EE Refactor (#5844)
* app compiles with every ee substituted
* Replace all oss files content
* Revert "Replace all oss files content"
This reverts commit
|