mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 16:03:47 +00:00
fix: address round 9 — abandon during the probe, and copy that outlived it
Abandoning while probeImportedPaths was in flight returned without settling anything. `import` goes running before the probe is asked, so the checklist kept a spinner on a run that had stopped, beside an enabled Retry and with no explanation. The settling the post-installProject path already did is now a helper both paths call. Three pieces of copy still described the behaviour this branch replaced: the resource alert said an existing path is "reported as failed" when the probe now leaves it alone and reports it as already there; and the step-4 footer and skip confirmation both told the user to set up a data table that the new `unknown` state means they already set up — only its schema could not be read. Those two now branch, so the strong warning stays strong for a data table that genuinely does not exist. The presence-key doc named `trigger:http_trigger`; WorkspaceTriggerKind has no such value. It is `http`, in the comment and in the two test mocks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx
This commit is contained in:
co-authored by
Claude Opus 5
parent
b2ebf63825
commit
2b50aeddc3
@@ -374,8 +374,9 @@
|
||||
bgClass="border-0"
|
||||
collapsible
|
||||
>
|
||||
Resources are imported as empty stubs — set their values after import; a resource whose path
|
||||
already exists is reported as failed (existing values are never overwritten). Trigger kinds are
|
||||
Resources are imported as empty stubs — set their values after import; one whose path is
|
||||
already in the workspace is left exactly as it is and reported as already there, so a value
|
||||
you have since filled in is never overwritten. Trigger kinds are
|
||||
recreated disabled, except GCP and Azure triggers, which manage cloud subscriptions at creation
|
||||
and must be re-created manually after filling their resource. Kafka, NATS, SQS, GCP and Azure
|
||||
triggers all require Enterprise. Triggers that reference a resource depend on stubs imported
|
||||
|
||||
@@ -81,6 +81,11 @@
|
||||
let resourceEditor: ResourceEditorDrawer | undefined = $state(undefined)
|
||||
|
||||
const pendingTables = $derived(rows.filter((r) => r.status !== 'done'))
|
||||
// Split because the two say different things to the user: one data table was never
|
||||
// created, the other exists and could not be read. Telling someone to set up what they
|
||||
// have already set up is how a warning stops being believed.
|
||||
const unmadeTables = $derived(rows.filter((r) => r.status === 'unconfigured'))
|
||||
const uncheckedTables = $derived(rows.filter((r) => r.status === 'unknown'))
|
||||
/** Rows the user has not dealt with, of either kind. */
|
||||
const outstanding = $derived(pendingTables.length + blanks.filter((b) => !b.done).length)
|
||||
|
||||
@@ -357,16 +362,24 @@
|
||||
// element would otherwise run script in this authenticated origin.
|
||||
const names = pendingTables.map((r) => escapeHtml(r.name)).join(', ')
|
||||
const one = pendingTables.length === 1
|
||||
// An `unknown` row is set up — only its schema could not be read — so it cannot be
|
||||
// told it is "not set up". Where the list mixes the two, say the weaker thing that
|
||||
// is true of both rather than the stronger one that is false of half.
|
||||
const anyUnmade = unmadeTables.length > 0
|
||||
const confirmed = await confirmationModal.ask({
|
||||
title: 'The project will not run',
|
||||
title: anyUnmade ? 'The project will not run' : 'This has not been verified',
|
||||
confirmationText: 'Skip anyway',
|
||||
type: 'danger',
|
||||
children:
|
||||
`${one ? 'The data table' : 'The data tables'} <b>${names}</b> ` +
|
||||
`${one ? 'is' : 'are'} not set up, so the tables this project's apps and flows read ` +
|
||||
`do not exist. Every one of them will fail as soon as it opens.<br /><br />` +
|
||||
`Setting ${one ? 'it' : 'them'} up later from workspace settings creates the ` +
|
||||
`connection but not the tables — only this step runs the project's migration.`
|
||||
type: anyUnmade ? 'danger' : 'info',
|
||||
children: anyUnmade
|
||||
? `${one ? 'The data table' : 'The data tables'} <b>${names}</b> ` +
|
||||
`${one ? 'is' : 'are'} not set up, so the tables this project's apps and flows read ` +
|
||||
`do not exist. Every one of them will fail as soon as it opens.<br /><br />` +
|
||||
`Setting ${one ? 'it' : 'them'} up later from workspace settings creates the ` +
|
||||
`connection but not the tables — only this step runs the project's migration.`
|
||||
: `${one ? 'The data table' : 'The data tables'} <b>${names}</b> ` +
|
||||
`${one ? 'is' : 'are'} set up, but ${one ? 'its' : 'their'} schema could not be ` +
|
||||
`read, so whether this project's tables exist is unknown. Its apps and flows will ` +
|
||||
`fail wherever they query a table that is missing.`
|
||||
})
|
||||
if (!confirmed) return
|
||||
}
|
||||
@@ -590,12 +603,27 @@
|
||||
Everything this project needs is configured. Finish, and it is ready to run.
|
||||
</Alert>
|
||||
{:else if pendingTables.length > 0}
|
||||
<Alert type="warning" title="The project will not run without this" size="xs">
|
||||
{pendingTables.length === 1 ? 'This data table does not' : 'These data tables do not'} exist
|
||||
yet, and the project's apps and flows query
|
||||
{pendingTables.length === 1 ? 'tables inside it' : 'tables inside them'}. Until
|
||||
{pendingTables.length === 1 ? 'it is' : 'they are'} set up, every one of them fails as soon as
|
||||
it opens.
|
||||
<Alert
|
||||
type="warning"
|
||||
title={unmadeTables.length > 0
|
||||
? 'The project will not run without this'
|
||||
: 'This could not be checked'}
|
||||
size="xs"
|
||||
>
|
||||
{#if unmadeTables.length > 0}
|
||||
{unmadeTables.length === 1 ? 'This data table does not' : 'These data tables do not'} exist
|
||||
yet, and the project's apps and flows query
|
||||
{unmadeTables.length === 1 ? 'tables inside it' : 'tables inside them'}. Until
|
||||
{unmadeTables.length === 1 ? 'it is' : 'they are'} set up, every one of them fails as soon
|
||||
as it opens.
|
||||
{/if}
|
||||
{#if uncheckedTables.length > 0}
|
||||
{#if unmadeTables.length > 0}<br /><br />{/if}
|
||||
{uncheckedTables.length === 1 ? 'One data table is' : 'Some data tables are'} set up, but
|
||||
{uncheckedTables.length === 1 ? 'its' : 'their'} schema could not be read, so whether the
|
||||
project's tables are there is unknown. Running the migrations again is safe — they create
|
||||
nothing that already exists.
|
||||
{/if}
|
||||
</Alert>
|
||||
{:else}
|
||||
<Alert type="info" title="You can skip this" size="xs" collapsible>
|
||||
|
||||
@@ -266,10 +266,11 @@ export async function applyOneMigration(
|
||||
/**
|
||||
* The kinds an import writes that carry a path and can therefore already be there.
|
||||
*
|
||||
* Triggers carry their own kind too (`trigger:schedule`, `trigger:http_trigger`, …): each
|
||||
* trigger kind is a separate table keyed on `(path, workspace_id)`, so one workspace can hold
|
||||
* a schedule and an HTTP trigger both called `f/cal/sync`. Flattening them to `trigger` would
|
||||
* let whichever exists answer for the other.
|
||||
* Triggers carry their own kind too (`trigger:schedule`, `trigger:http`, … — the values of
|
||||
* `WorkspaceTriggerKind`): each trigger kind is a separate table keyed on
|
||||
* `(path, workspace_id)`, so one workspace can hold a schedule and an HTTP trigger both
|
||||
* called `f/cal/sync`. Flattening them to `trigger` would let whichever exists answer for
|
||||
* the other.
|
||||
*/
|
||||
export type ImportedKind =
|
||||
| 'script'
|
||||
|
||||
@@ -24,7 +24,7 @@ vi.mock('$lib/gen', () => {
|
||||
vi.mock('../triggers/workspaceTriggersList', () => ({
|
||||
TRIGGER_KINDS: {
|
||||
schedule: { badge: 'Schedule', resourceField: undefined },
|
||||
http_trigger: { badge: 'HTTP', resourceField: undefined }
|
||||
http: { badge: 'HTTP', resourceField: undefined }
|
||||
},
|
||||
createWorkspaceTriggerDisabled: vi.fn(async (_ws: string, t: { path: string; kind: string }) => {
|
||||
created.triggers.push(`${t.kind}:${t.path}`)
|
||||
@@ -99,7 +99,7 @@ describe('installProject presence', () => {
|
||||
// hold a schedule and an HTTP trigger both called `f/calendly/nightly`. The one that
|
||||
// exists must not answer for the one that does not.
|
||||
it('does not let another trigger kind at the same path mask this one', async () => {
|
||||
const results = await run(new Set([presenceKey('trigger:http_trigger', 'f/calendly/nightly')]))
|
||||
const results = await run(new Set([presenceKey('trigger:http', 'f/calendly/nightly')]))
|
||||
expect(created.triggers).toEqual(['schedule:f/calendly/nightly'])
|
||||
expect(results).toContainEqual({ path: 'f/calendly/nightly', ok: true })
|
||||
})
|
||||
|
||||
@@ -18,11 +18,18 @@ 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>() }))
|
||||
const present = vi.hoisted(() => ({
|
||||
paths: new Set<string>(),
|
||||
/** Fires when the probe is entered, so a test can abandon while it is in flight. */
|
||||
onProbe: undefined as (() => void) | undefined
|
||||
}))
|
||||
vi.mock('./probe', async (orig) => ({
|
||||
...(await orig<typeof import('./probe')>()),
|
||||
probeWorkspace: vi.fn(async () => ({ exists: false, ours: false })),
|
||||
probeImportedPaths: vi.fn(async () => present.paths)
|
||||
probeImportedPaths: vi.fn(async () => {
|
||||
present.onProbe?.()
|
||||
return 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:
|
||||
@@ -204,3 +211,27 @@ describe('retrying over what is already there', () => {
|
||||
expect(run.tasks.find((t) => t.key === 'import')?.detail).toMatch(/3 imported/)
|
||||
})
|
||||
})
|
||||
|
||||
describe('abandoning while the presence probe is in flight', () => {
|
||||
beforeEach(() => {
|
||||
present.paths = new Set()
|
||||
present.onProbe = undefined
|
||||
})
|
||||
|
||||
/**
|
||||
* `import` goes `running` before the probe is asked, so returning straight out of an
|
||||
* abandon here leaves a spinner on a run that has stopped — next to an enabled Retry and
|
||||
* with no explanation of why it stopped.
|
||||
*/
|
||||
it('leaves no task running', async () => {
|
||||
const run = new ImportExecution(PLAN, deps as any)
|
||||
// Abandoned from inside the probe: `import` is already `running` by then, and the
|
||||
// executor's next look at the flag is the early return under test.
|
||||
present.onProbe = () => run.abandon()
|
||||
await run.run()
|
||||
expect(run.tasks.find((t) => t.status === 'running')).toBeUndefined()
|
||||
expect(run.tasks.find((t) => t.key === 'import')?.status).toBe('failed')
|
||||
expect(run.error).toBeTruthy()
|
||||
expect(run.done).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -324,6 +324,26 @@ export class ImportExecution {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Leave the checklist saying what actually happened, from wherever the run stopped.
|
||||
*
|
||||
* Reached from every point after `import` goes `running`, so nothing is left spinning on a
|
||||
* run that has ended. A partial import is failed rather than done: calling it done reports
|
||||
* a clean import over items that never started, and the resumed step offers Continue where
|
||||
* it should offer Retry.
|
||||
*/
|
||||
#settleAbandoned() {
|
||||
const landed = this.itemResults.length
|
||||
this.#set('import', 'failed', `stopped after ${landed} item${landed === 1 ? '' : 's'}`)
|
||||
// The migrate row is appended once the review settles and set running by
|
||||
// `onMigrationsStart`. Stopping before its loop leaves it spinning forever, which
|
||||
// reads as work still in progress on a run that has stopped.
|
||||
if (this.tasks.some((t) => t.key === 'migrate' && t.status === 'running')) {
|
||||
this.#set('migrate', 'pending')
|
||||
}
|
||||
this.error = 'Import stopped. Retry to import what is left.'
|
||||
}
|
||||
|
||||
async #import(workspace: string, exportData: ProjectExport): Promise<void> {
|
||||
this.#set('import', 'running')
|
||||
const folder = this.#plan.folder?.trim() || exportData.project.slug
|
||||
@@ -366,7 +386,12 @@ export class ImportExecution {
|
||||
triggers: exportData.triggers.length > 0,
|
||||
hasEeLicense: this.#deps.hasEeLicense
|
||||
})
|
||||
if (this.#abandoned) return
|
||||
// Settled, not just returned: `import` has been `running` since before the probe, and
|
||||
// leaving it there shows a spinner on a run that has stopped, next to a Retry button.
|
||||
if (this.#abandoned) {
|
||||
this.#settleAbandoned()
|
||||
return
|
||||
}
|
||||
try {
|
||||
await installProject({
|
||||
alreadyPresent,
|
||||
@@ -392,15 +417,7 @@ export class ImportExecution {
|
||||
// what it got through; calling that `done` reports a clean import over items that
|
||||
// never started, and the resumed step would offer Continue instead of Retry.
|
||||
if (this.#abandoned) {
|
||||
const landed = this.itemResults.length
|
||||
this.#set('import', 'failed', `stopped after ${landed} item${landed === 1 ? '' : 's'}`)
|
||||
// The migrate row is appended once the review settles and set running by
|
||||
// `onMigrationsStart`. Stopping before its loop leaves it spinning forever, which
|
||||
// reads as work still in progress on a run that has stopped.
|
||||
if (this.tasks.some((t) => t.key === 'migrate' && t.status === 'running')) {
|
||||
this.#set('migrate', 'pending')
|
||||
}
|
||||
this.error = 'Import stopped. Retry to import what is left.'
|
||||
this.#settleAbandoned()
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -66,9 +66,9 @@ describe('trigger presence keys', () => {
|
||||
* key on the path alone and the one that exists reports the other as already imported.
|
||||
*/
|
||||
it('keys a trigger by its kind, so one kind cannot answer for another', async () => {
|
||||
triggerRows.rows = [{ kind: 'http_trigger', path: 'f/calendly/sync' }]
|
||||
triggerRows.rows = [{ kind: 'http', path: 'f/calendly/sync' }]
|
||||
const found = await probeImportedPaths('w', 'calendly', { triggers: true })
|
||||
expect(found.has(presenceKey('trigger:http_trigger', 'f/calendly/sync'))).toBe(true)
|
||||
expect(found.has(presenceKey('trigger:http', 'f/calendly/sync'))).toBe(true)
|
||||
// The schedule the project also ships at that path has not been imported.
|
||||
expect(found.has(presenceKey('trigger:schedule', 'f/calendly/sync'))).toBe(false)
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user