mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-24 08:01:38 +00:00
fix(pipelines): // macros takes precedence over // pipeline (a library is never a member)
A macro library is definition-only — its macros are injected into consumers and running it is a no-op — so marking it `// pipeline` is meaningless and only produced a confusing state (an unused pipeline macro library appearing as a manual root). Make `// macros` win: parse_pipeline_annotations forces in_pipeline false when macros is set. Mirrored in all three parsers that must agree — the Rust canonical parser (drives deploy membership), the frontend TS parser (live graph), and the CLI local graph (pinned wasm still reports in_pipeline, so precedence is applied when skipping members). Shared parity fixture + unit tests on each side.
This commit is contained in:
@@ -951,6 +951,12 @@ pub fn parse_pipeline_annotations(code: &str) -> PipelineAnnotations {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// `// macros` wins over `// pipeline`: a macro library is definition-only
|
||||||
|
// (injected into consumers; running it is a no-op), never a pipeline member.
|
||||||
|
if out.macros {
|
||||||
|
out.in_pipeline = false;
|
||||||
|
}
|
||||||
|
|
||||||
out
|
out
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1316,6 +1322,17 @@ mod pipeline_annotation_tests {
|
|||||||
assert!(!parse_pipeline_annotations("// macros_v2\n").macros);
|
assert!(!parse_pipeline_annotations("// macros_v2\n").macros);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn macros_wins_over_pipeline_membership() {
|
||||||
|
// A `// macros` library is definition-only, so `// pipeline` is ignored:
|
||||||
|
// `in_pipeline` is forced false even when both markers are present.
|
||||||
|
let out = parse_pipeline_annotations("// pipeline\n// macros\nCREATE MACRO m(a) AS a;");
|
||||||
|
assert!(out.macros);
|
||||||
|
assert!(!out.in_pipeline);
|
||||||
|
// A plain `// pipeline` script (no macros) stays a member.
|
||||||
|
assert!(parse_pipeline_annotations("// pipeline\nSELECT 1;").in_pipeline);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn use_accumulates_dedups_and_rejects_prose() {
|
fn use_accumulates_dedups_and_rejects_prose() {
|
||||||
let out = parse_pipeline_annotations(
|
let out = parse_pipeline_annotations(
|
||||||
|
|||||||
@@ -670,6 +670,20 @@
|
|||||||
"macros": true
|
"macros": true
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
"name": "macros wins over pipeline (in_pipeline forced false)",
|
||||||
|
"code": "// pipeline\n// macros\nCREATE MACRO dbl(a) AS a * 2;",
|
||||||
|
"expected": {
|
||||||
|
"in_pipeline": false,
|
||||||
|
"asset_triggers": [],
|
||||||
|
"native_triggers": [],
|
||||||
|
"partition": null,
|
||||||
|
"freshness": null,
|
||||||
|
"tag": null,
|
||||||
|
"retry": null,
|
||||||
|
"macros": true
|
||||||
|
}
|
||||||
|
},
|
||||||
{
|
{
|
||||||
"name": "macros with trailing prose is not a marker",
|
"name": "macros with trailing prose is not a marker",
|
||||||
"code": "// macros are defined below\nCREATE MACRO dbl(a) AS a * 2;",
|
"code": "// macros are defined below\nCREATE MACRO dbl(a) AS a * 2;",
|
||||||
@@ -687,7 +701,7 @@
|
|||||||
"name": "macros marker with sql comment prefix",
|
"name": "macros marker with sql comment prefix",
|
||||||
"code": "-- macros\n-- pipeline\nCREATE MACRO dbl(a) AS a * 2;",
|
"code": "-- macros\n-- pipeline\nCREATE MACRO dbl(a) AS a * 2;",
|
||||||
"expected": {
|
"expected": {
|
||||||
"in_pipeline": true,
|
"in_pipeline": false,
|
||||||
"asset_triggers": [],
|
"asset_triggers": [],
|
||||||
"native_triggers": [],
|
"native_triggers": [],
|
||||||
"partition": null,
|
"partition": null,
|
||||||
|
|||||||
@@ -457,6 +457,11 @@ export async function buildLocalPipelineGraph(args: {
|
|||||||
const folderDir = path.join(args.root, "f", folderClean);
|
const folderDir = path.join(args.root, "f", folderClean);
|
||||||
const all = await collectScripts(folderDir, args.root, args.defaultTs);
|
const all = await collectScripts(folderDir, args.root, args.defaultTs);
|
||||||
|
|
||||||
|
// `// macros` DuckDB libraries across the whole workspace (a pipeline may use a
|
||||||
|
// shared library outside its folder). `// macros` wins over `// pipeline`, so a
|
||||||
|
// library is never a pipeline member — matching the backend parser precedence.
|
||||||
|
const libMacros = collectMacroLibraries(args.root);
|
||||||
|
|
||||||
const runnables: GraphRunnable[] = [];
|
const runnables: GraphRunnable[] = [];
|
||||||
const edges: GraphEdge[] = [];
|
const edges: GraphEdge[] = [];
|
||||||
const triggers: GraphTrigger[] = [];
|
const triggers: GraphTrigger[] = [];
|
||||||
@@ -472,7 +477,9 @@ export async function buildLocalPipelineGraph(args: {
|
|||||||
|
|
||||||
for (const s of all) {
|
for (const s of all) {
|
||||||
const out = await inferScriptAssets(s.content, s.language);
|
const out = await inferScriptAssets(s.content, s.language);
|
||||||
if (!out.in_pipeline) continue; // not a pipeline member
|
// Not a member: no `// pipeline` marker, OR a `// macros` library (which the
|
||||||
|
// pinned wasm still reports as `in_pipeline` — apply the precedence here).
|
||||||
|
if (!out.in_pipeline || libMacros.has(s.path)) continue;
|
||||||
const retry = normalizeRetry(out.retry);
|
const retry = normalizeRetry(out.retry);
|
||||||
const nativeTriggers = recoverHeaderNativeTriggers(s.content, s.language);
|
const nativeTriggers = recoverHeaderNativeTriggers(s.content, s.language);
|
||||||
// Carry the parsed `// tag` so previews route to the same worker the
|
// Carry the parsed `// tag` so previews route to the same worker the
|
||||||
@@ -599,15 +606,6 @@ export async function buildLocalPipelineGraph(args: {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// `// macros` libraries + lib→consumer edges. The wasm asset parser drops the
|
|
||||||
// `// macros` / `// use` annotations and never emits a macro registry, so we
|
|
||||||
// derive both from the working tree here (see ./duckdbMacros.ts) to match the
|
|
||||||
// deployed graph, which records them at deploy. Libraries are discovered
|
|
||||||
// WORKSPACE-WIDE (not just this folder) — the deployed builder fetches the
|
|
||||||
// macro registry unfiltered so a shared library (e.g. `f/shared/stats`) is the
|
|
||||||
// provider endpoint of an in-folder consumer's edge; only consumers are
|
|
||||||
// folder-scoped. Macros are DuckDB-only.
|
|
||||||
const libMacros = collectMacroLibraries(args.root);
|
|
||||||
const macroEdges = buildMacroEdges(all, libMacros, runnables);
|
const macroEdges = buildMacroEdges(all, libMacros, runnables);
|
||||||
|
|
||||||
const assets = [...assetSet.entries()].map(([key, a]) => {
|
const assets = [...assetSet.entries()].map(([key, a]) => {
|
||||||
@@ -665,13 +663,10 @@ function collectMacroLibraries(root: string): Map<string, ParsedMacro[]> {
|
|||||||
return out;
|
return out;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Resolve which of this folder's pipeline scripts call the workspace's macro
|
// Derive lib→consumer edges (lexical calls + `// use`) and add each referenced
|
||||||
// libraries (by lexical call detection + `// use` annotations), then (a) mutate
|
// library to `runnables` as a node with its signatures. Mirrors the deployed
|
||||||
// `runnables` to add each referenced library as a node carrying its macro
|
// `asset_graph`: libraries are workspace-wide, consumers folder-scoped, and an
|
||||||
// signatures, and (b) return the lib→consumer edges. Mirrors the deployed graph
|
// unused library (no edge) is not surfaced.
|
||||||
// builder (`asset_graph` in windmill-api-assets): libraries come from the
|
|
||||||
// workspace-wide registry, consumers are folder-scoped, and a library node
|
|
||||||
// appears only when it is an endpoint of at least one edge (unused → not shown).
|
|
||||||
function buildMacroEdges(
|
function buildMacroEdges(
|
||||||
all: LocalScript[],
|
all: LocalScript[],
|
||||||
libMacros: Map<string, ParsedMacro[]>,
|
libMacros: Map<string, ParsedMacro[]>,
|
||||||
@@ -755,28 +750,17 @@ function buildMacroEdges(
|
|||||||
a.consumer_path.localeCompare(b.consumer_path),
|
a.consumer_path.localeCompare(b.consumer_path),
|
||||||
);
|
);
|
||||||
|
|
||||||
// Tag library nodes with their macro signatures, like the deployed builder
|
// A library node surfaces only when it is an edge provider (a consumer uses
|
||||||
// (which sets `macros` on any node whose path provides macros, edge or not).
|
// it); an unused library is not shown. Libraries are never pipeline members
|
||||||
// Two sources:
|
// (`// macros` wins over `// pipeline`), so each is a fresh node here.
|
||||||
// • every edge provider — added as a node if it isn't already one; and
|
|
||||||
// • every library that is ALREADY a runnable (a `// pipeline` + `// macros`
|
|
||||||
// script), even with no consumers yet — so it's recognized as
|
|
||||||
// definition-only and never scheduled as a manual root.
|
|
||||||
// An unused NON-pipeline library stays absent (suppressed), matching deployed.
|
|
||||||
const libPaths = new Set<string>(edges.map((e) => e.lib_path));
|
const libPaths = new Set<string>(edges.map((e) => e.lib_path));
|
||||||
const existingPaths = new Set(runnables.map((r) => r.path));
|
|
||||||
for (const lib of libMacros.keys()) {
|
|
||||||
if (existingPaths.has(lib)) libPaths.add(lib);
|
|
||||||
}
|
|
||||||
for (const lib of libPaths) {
|
for (const lib of libPaths) {
|
||||||
const macros = (libMacros.get(lib) ?? []).map((m) => ({
|
const macros = (libMacros.get(lib) ?? []).map((m) => ({
|
||||||
name: m.name,
|
name: m.name,
|
||||||
params: m.params,
|
params: m.params,
|
||||||
is_table: m.isTable,
|
is_table: m.isTable,
|
||||||
}));
|
}));
|
||||||
const existing = runnables.find((r) => r.path === lib);
|
runnables.push({ path: lib, usage_kind: "script", macros });
|
||||||
if (existing) existing.macros = macros;
|
|
||||||
else runnables.push({ path: lib, usage_kind: "script", macros });
|
|
||||||
}
|
}
|
||||||
// Force every edge's CONSUMER endpoint into the node set too, like the deployed
|
// Force every edge's CONSUMER endpoint into the node set too, like the deployed
|
||||||
// builder, so no edge dangles at a missing runnable. A consumer that is itself
|
// builder, so no edge dangles at a missing runnable. A consumer that is itself
|
||||||
|
|||||||
@@ -526,25 +526,48 @@ test("a non-pipeline DuckDB macro consumer is a display-only node, never a run s
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
test("an UNUSED `// pipeline` + `// macros` library is still tagged with its macros", async () => {
|
test("`// macros` wins over `// pipeline`: the library is never a pipeline member", async () => {
|
||||||
// A DuckDB file can be both `// pipeline` and `// macros`. The deployed builder
|
// A DuckDB file marked both `// pipeline` and `// macros` is treated as a
|
||||||
// tags such a node with its macros even when no consumer uses it yet, so it's
|
// definition-only library (mirrors the backend parser precedence). So an
|
||||||
// recognized as definition-only and never scheduled as a manual root. Local
|
// UNUSED such library is suppressed exactly like a plain `// macros` one — not
|
||||||
// enrichment must do the same (otherwise `run --local` would run it).
|
// a member node, hence never a run step.
|
||||||
await withFolder(
|
await withFolder(
|
||||||
{
|
{
|
||||||
"lib.duckdb.sql": `-- pipeline\n-- macros\nCREATE MACRO dbl(a) AS a * 2;\n`,
|
"lib.duckdb.sql": `-- pipeline\n-- macros\nCREATE MACRO dbl(a) AS a * 2;\n`,
|
||||||
"root.duckdb.sql": `-- pipeline\n-- materialize ducklake://main/out\nSELECT 1 AS v;\n`,
|
"root.duckdb.sql": `-- pipeline\n-- materialize ducklake://main/out\nSELECT 1 AS v;\n`,
|
||||||
},
|
},
|
||||||
async (root, folder) => {
|
async (root, folder) => {
|
||||||
const { graph } = await buildLocalPipelineGraph({ root, folder, defaultTs: "bun" });
|
const { graph, scripts } = await buildLocalPipelineGraph({ root, folder, defaultTs: "bun" });
|
||||||
// the library node carries its macro signatures despite having no consumers
|
// unused library → suppressed (not a node), and never a previewable member
|
||||||
|
expect(graph.runnables.map((r) => r.path)).toEqual(["f/mypipe/root"]);
|
||||||
|
expect(scripts.map((s) => s.path)).toEqual(["f/mypipe/root"]);
|
||||||
|
expect(graph.macro_edges).toBeUndefined();
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("a USED `// pipeline` + `// macros` library appears as a library node (not a member)", async () => {
|
||||||
|
await withFolder(
|
||||||
|
{
|
||||||
|
"lib.duckdb.sql": `-- pipeline\n-- macros\nCREATE MACRO dbl(a) AS a * 2;\n`,
|
||||||
|
"root.duckdb.sql": `-- pipeline\n-- on datatable://main/t\nSELECT dbl(x) FROM main.t;\n`,
|
||||||
|
},
|
||||||
|
async (root, folder) => {
|
||||||
|
const { graph, scripts } = await buildLocalPipelineGraph({ root, folder, defaultTs: "bun" });
|
||||||
|
// the library surfaces as a node with its signatures (it's used) …
|
||||||
expect(graph.runnables.find((r) => r.path === "f/mypipe/lib")?.macros).toEqual([
|
expect(graph.runnables.find((r) => r.path === "f/mypipe/lib")?.macros).toEqual([
|
||||||
{ name: "dbl", params: "a", is_table: false },
|
{ name: "dbl", params: "a", is_table: false },
|
||||||
]);
|
]);
|
||||||
// no consumer → no macro edges, but the node is still marked (macros.length
|
// … but is NOT a pipeline member: absent from the previewable `scripts` set
|
||||||
// > 0 is what `pipeline run` uses to exclude it from the run selection)
|
expect(scripts.map((s) => s.path)).toEqual(["f/mypipe/root"]);
|
||||||
expect(graph.macro_edges).toBeUndefined();
|
expect(graph.macro_edges).toEqual([
|
||||||
|
{
|
||||||
|
lib_path: "f/mypipe/lib",
|
||||||
|
consumer_path: "f/mypipe/root",
|
||||||
|
macro_names: ["dbl"],
|
||||||
|
via_use: false,
|
||||||
|
},
|
||||||
|
]);
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -108,6 +108,12 @@ describe('parsePipelineAnnotations: macros + use', () => {
|
|||||||
expect(parsePipelineAnnotations('-- macros \nSELECT 1;').macros).toBe(true)
|
expect(parsePipelineAnnotations('-- macros \nSELECT 1;').macros).toBe(true)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('macros wins over pipeline — a library is never a pipeline member', () => {
|
||||||
|
const out = parsePipelineAnnotations('// pipeline\n// macros\nCREATE MACRO m(a) AS a;')
|
||||||
|
expect(out.macros).toBe(true)
|
||||||
|
expect(out.inPipeline).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
it('use accumulates in order and dedups', () => {
|
it('use accumulates in order and dedups', () => {
|
||||||
const out = parsePipelineAnnotations(
|
const out = parsePipelineAnnotations(
|
||||||
'// use f/lib/stats\n// use f/lib/dates\n// use f/lib/stats\n'
|
'// use f/lib/stats\n// use f/lib/dates\n// use f/lib/stats\n'
|
||||||
|
|||||||
@@ -685,5 +685,9 @@ export function parsePipelineAnnotations(code: string): PipelineAnnotations {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// `// macros` wins over `// pipeline`: a macro library is definition-only,
|
||||||
|
// never a pipeline member. Mirrors the Rust parse_pipeline_annotations.
|
||||||
|
if (out.macros) out.inPipeline = false
|
||||||
|
|
||||||
return out
|
return out
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user