mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 08:02:18 +00:00
feat: import only what the destination does not already have
A retry resent the whole bundle. Everything that had already landed came back as "already exists" — nine failures over work that had succeeded, with no way to tell those from the ones that genuinely failed. The same thing happened importing into a workspace that already held some of the project. `installProject` now takes `alreadyPresent`, checked after retargeting because that is what the items will actually be called, and `probeImportedPaths` fills it from the destination on every run. On a workspace the run just created the answer is empty and nothing is skipped, so this costs four scoped reads and changes nothing about a first import. Skipping is not replacing. An item that is there is left exactly as it is — the same promise `updateIfExists: false` already makes for a resource whose value someone has since filled in. `InstallResult` gains `skipped`, because "already there" is neither an import nor a failure and reporting it as either is a lie. The checklist still lists every item the project ships; a skipped one shows as skipped and says why. The import row now counts the three outcomes separately — `8 already there` rather than a green tick over `2 apps, 4 scripts, 2 resources` it did not write. That last part needed the pre-run breakdown to stand down once the run has an outcome of its own, or it went on claiming the import had happened. Checked by removing the gate: two of the four new tests fail. Verified against a real backend by re-importing Calendly into a workspace that already had it — 0 failures, 0 create requests, and the row reads "8 already there", where the same run previously produced 9 conflicts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
6553dbee11
commit
f9ad196712
@@ -179,7 +179,11 @@
|
||||
// total said in a more useful way.
|
||||
const checklist = $derived<SetupStep[]>(
|
||||
tasks.map((task) => {
|
||||
const detail = (task.key === 'import' && importSummary) || task.detail
|
||||
// The breakdown says what the import *will* bring, so it belongs to the row only
|
||||
// until the run has an outcome of its own. Left in place it would go on claiming
|
||||
// "2 apps, 4 scripts" over a run that wrote none of them because they were
|
||||
// already there.
|
||||
const detail = task.key === 'import' ? task.detail || importSummary : task.detail
|
||||
return {
|
||||
title: detail ? `${task.label} — ${detail}` : task.label,
|
||||
status: task.status,
|
||||
@@ -189,8 +193,14 @@
|
||||
task.key === 'import'
|
||||
? execution?.itemResults.map((r) => ({
|
||||
title: r.path,
|
||||
status: r.ok ? ('done' as const) : ('failed' as const),
|
||||
description: r.error
|
||||
// `skipped`, not `done`: nothing was written, and a green tick over an
|
||||
// item this run left alone claims an import that did not happen.
|
||||
status: !r.ok
|
||||
? ('failed' as const)
|
||||
: r.skipped
|
||||
? ('skipped' as const)
|
||||
: ('done' as const),
|
||||
description: r.skipped ? 'Already in the workspace — left as it is.' : r.error
|
||||
}))
|
||||
: undefined
|
||||
}
|
||||
|
||||
@@ -44,6 +44,11 @@ export interface InstallResult {
|
||||
path: string
|
||||
ok: boolean
|
||||
error?: string
|
||||
/**
|
||||
* Already in the destination, so nothing was written. Not a failure and not an import —
|
||||
* reporting it as either would be a lie, and the difference is what a retry is for.
|
||||
*/
|
||||
skipped?: boolean
|
||||
}
|
||||
|
||||
// Guarding an item's own path is not enough: the `$res:`/script/flow refs baked
|
||||
@@ -273,6 +278,16 @@ export async function installProject(args: {
|
||||
* service call: the import wizard uses it when the user confirms leaving mid-run.
|
||||
*/
|
||||
stopped?: () => boolean
|
||||
/**
|
||||
* Paths already in the destination, retargeted — so a retry writes only what is missing
|
||||
* instead of replaying the bundle into a wall of "already exists". Compared after
|
||||
* retargeting, because that is what these items will actually be called.
|
||||
*
|
||||
* Never a way to *replace* anything: an item that is there is left exactly as it is,
|
||||
* which is the same promise `updateIfExists: false` makes for a resource whose value
|
||||
* someone has since filled in.
|
||||
*/
|
||||
alreadyPresent?: Set<string>
|
||||
hasEeLicense: boolean
|
||||
onResult: (r: InstallResult) => void
|
||||
}): Promise<void> {
|
||||
@@ -284,7 +299,8 @@ export async function installProject(args: {
|
||||
hasEeLicense,
|
||||
onResult,
|
||||
onMigrationsStart,
|
||||
stopped
|
||||
stopped,
|
||||
alreadyPresent
|
||||
} = args
|
||||
|
||||
const record = (path: string, p: Promise<unknown>): Promise<void> =>
|
||||
@@ -296,6 +312,17 @@ export async function installProject(args: {
|
||||
/** Every write goes through here, so one check covers items, variables and migrations. */
|
||||
const halted = () => stopped?.() === true
|
||||
|
||||
/**
|
||||
* True when the destination already has this path, so the write is not attempted. Reported
|
||||
* rather than dropped: the checklist has to account for every item the project ships, and
|
||||
* "already there" is a different thing from "imported".
|
||||
*/
|
||||
const present = (path: string): boolean => {
|
||||
if (!alreadyPresent?.has(path)) return false
|
||||
onResult({ path, ok: true, skipped: true })
|
||||
return true
|
||||
}
|
||||
|
||||
try {
|
||||
await FolderService.createFolder({ workspace, requestBody: { name: folder } })
|
||||
} catch {}
|
||||
@@ -336,6 +363,7 @@ export async function installProject(args: {
|
||||
|
||||
for (const s of proj.scripts) {
|
||||
if (halted()) return
|
||||
if (present(s.path)) continue
|
||||
// `$var:` is resolved in job args (flow inputs, schedule args, trigger config),
|
||||
// not in script source, so there is no variable arg to contain here.
|
||||
await checkedItem(s.path, extractScriptRefs(s.content ?? ''), undefined, () =>
|
||||
@@ -344,10 +372,12 @@ export async function installProject(args: {
|
||||
}
|
||||
for (const f of proj.flows) {
|
||||
if (halted()) return
|
||||
if (present(f.path)) continue
|
||||
await checkedItem(f.path, extractFlowRefs(f.value), f.value, () => importFlow(workspace, f))
|
||||
}
|
||||
for (const r of proj.resources) {
|
||||
if (halted()) return
|
||||
if (present(r.path)) continue
|
||||
await checked(r.path, () => importResourceStub(workspace, r))
|
||||
}
|
||||
// Placeholders for the project's internal `$var:`/`$jsonvar:` refs (retargeted
|
||||
@@ -360,6 +390,7 @@ export async function installProject(args: {
|
||||
}
|
||||
for (const a of proj.apps) {
|
||||
if (halted()) return
|
||||
if (present(a.path)) continue
|
||||
const isRaw = a.app_type === 'raw'
|
||||
const refs = isRaw ? extractRawAppRefs(a.value?.raw ?? '') : extractAppRefs(a.value)
|
||||
// Raw apps hold their runnables in the `value.raw` JSON string; parse it so the
|
||||
|
||||
@@ -16,6 +16,14 @@ vi.mock('$lib/gen', () => ({
|
||||
}
|
||||
}))
|
||||
vi.mock('$lib/storeUtils', () => ({ switchWorkspace: vi.fn() }))
|
||||
// What the destination already holds. A test sets this to stand in for a workspace that has
|
||||
// some of the bundle in it — a half-finished run, or an existing workspace.
|
||||
const present = vi.hoisted(() => ({ paths: new Set<string>() }))
|
||||
vi.mock('./probe', async (orig) => ({
|
||||
...(await orig<typeof import('./probe')>()),
|
||||
probeWorkspace: vi.fn(async () => ({ exists: false, ours: false })),
|
||||
probeImportedPaths: vi.fn(async () => present.paths)
|
||||
}))
|
||||
vi.mock('$lib/user', () => ({ getUserExt: vi.fn(async () => ({ username: 'u' })) }))
|
||||
// Let a test abandon *during* a write loop, which is the only way it happens for real:
|
||||
// `run()` clears the flag on entry so a retry can proceed. Two hooks, because the item and
|
||||
@@ -32,6 +40,10 @@ vi.mock('$lib/components/workspaceSettings/projectInstall', () => ({
|
||||
// returns on success.
|
||||
for (const path of ['a', 'b', 'c']) {
|
||||
if (args.stopped?.() === true) return
|
||||
if (args.alreadyPresent?.has(path)) {
|
||||
args.onResult({ path, ok: true, skipped: true })
|
||||
continue
|
||||
}
|
||||
args.onResult({ path, ok: true })
|
||||
hooks.afterFirstItem?.()
|
||||
hooks.afterFirstItem = undefined
|
||||
@@ -109,6 +121,7 @@ describe('abandoning mid-import', () => {
|
||||
beforeEach(() => {
|
||||
hooks.afterFirstItem = undefined
|
||||
hooks.afterMigrationsStart = undefined
|
||||
present.paths = new Set()
|
||||
})
|
||||
|
||||
it('does not report done, so the resumed step offers Retry rather than Continue', async () => {
|
||||
@@ -143,3 +156,48 @@ describe('abandoning mid-import', () => {
|
||||
expect(run.done).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
/**
|
||||
* A retry used to resend the whole bundle, so everything that had already landed came back as
|
||||
* "already exists" — a wall of failures over work that had succeeded. What is already there is
|
||||
* now skipped, and reported as skipped rather than as imported.
|
||||
*/
|
||||
describe('retrying over what is already there', () => {
|
||||
beforeEach(() => {
|
||||
present.paths = new Set()
|
||||
})
|
||||
|
||||
it('writes nothing for a path the destination already holds', async () => {
|
||||
present.paths = new Set(['a', 'b'])
|
||||
const run = new ImportExecution(PLAN, deps)
|
||||
await run.run()
|
||||
const byPath = new Map(run.itemResults.map((r) => [r.path, r]))
|
||||
expect(byPath.get('a')?.skipped).toBe(true)
|
||||
expect(byPath.get('b')?.skipped).toBe(true)
|
||||
expect(byPath.get('c')?.skipped).toBeUndefined()
|
||||
})
|
||||
|
||||
it('still accounts for every item, so the checklist stays complete', async () => {
|
||||
present.paths = new Set(['a', 'b'])
|
||||
const run = new ImportExecution(PLAN, deps)
|
||||
await run.run()
|
||||
expect(run.itemResults.length).toBe(3)
|
||||
expect(run.done).toBe(true)
|
||||
})
|
||||
|
||||
it('says what it did rather than claiming to have imported all of it', async () => {
|
||||
present.paths = new Set(['a', 'b'])
|
||||
const run = new ImportExecution(PLAN, deps)
|
||||
await run.run()
|
||||
const importRow = run.tasks.find((t) => t.key === 'import')
|
||||
expect(importRow?.detail).toMatch(/1 imported/)
|
||||
expect(importRow?.detail).toMatch(/2 already there/)
|
||||
})
|
||||
|
||||
it('imports everything when the destination is empty', async () => {
|
||||
const run = new ImportExecution(PLAN, deps)
|
||||
await run.run()
|
||||
expect(run.itemResults.every((r) => !r.skipped)).toBe(true)
|
||||
expect(run.tasks.find((t) => t.key === 'import')?.detail).toMatch(/3 imported/)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -13,7 +13,7 @@ import type {
|
||||
ProjectMigration
|
||||
} from '$lib/components/workspaceSettings/projectBundle'
|
||||
import { planWorkspaceId, type ImportPlan } from './plan'
|
||||
import { probeWorkspace } from './probe'
|
||||
import { probeImportedPaths, probeWorkspace } from './probe'
|
||||
|
||||
/**
|
||||
* The only thing in the wizard that changes anything. It takes a finished plan and
|
||||
@@ -359,8 +359,15 @@ export class ImportExecution {
|
||||
}
|
||||
|
||||
this.results = []
|
||||
// Asked every run, not only on a retry: the destination may be a workspace that already
|
||||
// holds some of these paths, and a run interrupted halfway is indistinguishable from
|
||||
// one that never started. On a workspace this run just created the answer is empty and
|
||||
// nothing is skipped.
|
||||
const alreadyPresent = await probeImportedPaths(workspace, folder)
|
||||
if (this.#abandoned) return
|
||||
try {
|
||||
await installProject({
|
||||
alreadyPresent,
|
||||
workspace,
|
||||
exportData,
|
||||
folder,
|
||||
@@ -397,11 +404,16 @@ export class ImportExecution {
|
||||
|
||||
const items = this.itemResults
|
||||
const failed = items.filter((r) => !r.ok).length
|
||||
this.#set(
|
||||
'import',
|
||||
failed > 0 ? 'failed' : 'done',
|
||||
failed > 0 ? `${items.length - failed} of ${items.length} imported` : `${items.length} items`
|
||||
)
|
||||
const skipped = items.filter((r) => r.skipped).length
|
||||
// Three outcomes, so the row says which: written, left alone because it was already
|
||||
// there, and failed. Rolling the second into the first would report an import that
|
||||
// did not happen.
|
||||
const wrote = items.length - failed - skipped
|
||||
const parts: string[] = []
|
||||
if (wrote > 0 || (failed === 0 && skipped === 0)) parts.push(`${wrote} imported`)
|
||||
if (skipped > 0) parts.push(`${skipped} already there`)
|
||||
if (failed > 0) parts.push(`${failed} failed`)
|
||||
this.#set('import', failed > 0 ? 'failed' : 'done', parts.join(', '))
|
||||
|
||||
const migrated = this.migrationResults
|
||||
if (migrated.length) {
|
||||
|
||||
Reference in New Issue
Block a user