mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 08:02:38 +00:00
feat: report script metadata with no content file in wmill lint (#11053)
Claude-Session: https://claude.ai/code/session_01PENfHNJceAbrcUsgfFCjXN Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
9a563f6d72
commit
8820b9fc64
@@ -31,10 +31,18 @@ import {
|
||||
import {
|
||||
isFlowInlineScriptPath,
|
||||
isAppInlineScriptPath,
|
||||
isRawAppPath,
|
||||
isFolderResourcePathAnyFormat,
|
||||
getFolderSuffix,
|
||||
getScriptBasePathFromModulePath,
|
||||
} from "../../utils/resource_folders.ts";
|
||||
import { exts } from "../script/script.ts";
|
||||
import { isFilesetResource } from "../../utils/utils.ts";
|
||||
import {
|
||||
exts,
|
||||
findContentFile,
|
||||
hasScriptExt,
|
||||
isModuleEntryMetadata,
|
||||
UnresolvableScriptContentFileError,
|
||||
} from "../script/script.ts";
|
||||
|
||||
interface LintOptions extends GlobalOptions {
|
||||
json?: boolean;
|
||||
@@ -67,6 +75,9 @@ export interface LintReport {
|
||||
|
||||
const YAML_FILE_REGEX = /\.ya?ml$/i;
|
||||
const NATIVE_TRIGGER_REGEX = /\.[^.]+_native_trigger\.ya?ml$/i;
|
||||
// The metadata suffixes `findContentFile` resolves a flat script from. `.yml` is
|
||||
// deliberately absent, since the push does not accept it there either.
|
||||
const FLAT_SCRIPT_METADATA_REGEX = /\.script\.(yaml|json|lock)$/;
|
||||
|
||||
function normalizePath(p: string): string {
|
||||
return p.replaceAll(SEP, "/");
|
||||
@@ -643,6 +654,83 @@ export async function checkMissingLocks(
|
||||
return issues;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a path is a script's own metadata, as opposed to metadata the push
|
||||
* deploys through some parent: a folder resource's inline scripts, a fileset's
|
||||
* children (arbitrarily named, so one may be spelled exactly like a script's
|
||||
* metadata) and the files of a module or dbt bundle all belong to that parent.
|
||||
*
|
||||
* Takes the path as the SYNC ROOT spells it, like the push. Relative to the
|
||||
* lint target the enclosing folder is gone whenever the target IS that folder;
|
||||
* absolute, the classifiers match their suffixes ANYWHERE in the string, so a
|
||||
* checkout under `acme.app` reads as one app and nothing is ever reported.
|
||||
*/
|
||||
function isStandaloneScriptMetadata(rootedPath: string): boolean {
|
||||
// Both suffix formats, because the dotted/non-dotted setting is read from the
|
||||
// invocation directory and an explicit lint target may not share it.
|
||||
if (
|
||||
isFolderResourcePathAnyFormat(rootedPath) ||
|
||||
isFilesetResource(rootedPath)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
// A module folder keeps its metadata inside itself (`<base>__mod/script.yaml`),
|
||||
// which is standalone even though every other path under `__mod/` is not.
|
||||
if (isModuleEntryMetadata(rootedPath)) return true;
|
||||
if (getScriptBasePathFromModulePath(rootedPath) !== undefined) return false;
|
||||
return FLAT_SCRIPT_METADATA_REGEX.test(rootedPath);
|
||||
}
|
||||
|
||||
/**
|
||||
* `findContentFile` quotes the paths it was given back in its errors, so the
|
||||
* lint target's own prefix comes off them again. Anchored at a path start: a
|
||||
* plain substring replace of `f/` also eats the one inside `conf/`, mangling
|
||||
* the very filename the message is telling the reader to delete.
|
||||
*/
|
||||
function relativizeMessage(message: string, prefix: string): string {
|
||||
if (!prefix) return message;
|
||||
const escaped = prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
return message.replaceAll(new RegExp(`(^|[\\s(])${escaped}/`, "g"), "$1");
|
||||
}
|
||||
|
||||
/**
|
||||
* Script metadata files that cannot be paired with exactly one content file:
|
||||
* the push refuses those, and no metadata format makes them deployable, so the
|
||||
* inactive twin of a format switch (`foo.script.json` in a yaml repo) is dead
|
||||
* weight worth reporting even though the push skips it rather than refusing it.
|
||||
*
|
||||
* Resolved through `findContentFile` rather than by probing `exts` directly, so
|
||||
* lint and push agree on what counts as paired: a dbt project's descriptor is
|
||||
* optional and its absence is not an orphan, while two content files beside one
|
||||
* metadata file is just as undeployable as none. It classifies what it is given
|
||||
* and looks under `syncRoot`, so where the command was invoked from is not part
|
||||
* of the answer.
|
||||
*/
|
||||
async function checkOrphanScriptMetadata(
|
||||
syncRoot: string,
|
||||
prefix: string,
|
||||
metadataPaths: string[],
|
||||
): Promise<FileIssue[]> {
|
||||
const issues: FileIssue[] = [];
|
||||
for (const metadataPath of metadataPaths) {
|
||||
const rootedPath = prefix ? `${prefix}/${metadataPath}` : metadataPath;
|
||||
try {
|
||||
await findContentFile(rootedPath, syncRoot);
|
||||
} catch (e) {
|
||||
if (!(e instanceof UnresolvableScriptContentFileError)) {
|
||||
log.debug(`Failed to resolve content file for ${rootedPath}: ${e}`);
|
||||
continue;
|
||||
}
|
||||
issues.push({
|
||||
path: metadataPath,
|
||||
target: "script",
|
||||
errors: [relativizeMessage(e.message, prefix)],
|
||||
});
|
||||
}
|
||||
}
|
||||
return issues;
|
||||
}
|
||||
|
||||
export async function runLint(
|
||||
opts: LintOptions,
|
||||
directory?: string,
|
||||
@@ -674,8 +762,16 @@ export async function runLint(
|
||||
const root = await FSFSElement(targetDirectory, [], false);
|
||||
const validator = new WindmillYamlValidator();
|
||||
|
||||
// Walked paths are relative to the lint target; this puts them back the way
|
||||
// the sync root spells them, which is what the two below are written against.
|
||||
const syncRoot = await findSyncRoot(targetDirectory);
|
||||
const metadataPrefix = normalizePath(
|
||||
path.relative(syncRoot, targetDirectory),
|
||||
);
|
||||
|
||||
const warnings: LintWarning[] = [];
|
||||
const issues: FileIssue[] = [];
|
||||
const scriptMetadataPaths: string[] = [];
|
||||
let scannedFiles = 0;
|
||||
let validatedFiles = 0;
|
||||
let validFiles = 0;
|
||||
@@ -689,6 +785,17 @@ export async function runLint(
|
||||
const normalizedPath = normalizePath(entry.path);
|
||||
|
||||
scannedFiles += 1;
|
||||
|
||||
// Collected before the YAML filter below: `.script.lock` and `.script.json`
|
||||
// are metadata too, and both fail the push when nothing pairs with them.
|
||||
if (
|
||||
isStandaloneScriptMetadata(
|
||||
metadataPrefix ? `${metadataPrefix}/${normalizedPath}` : normalizedPath,
|
||||
)
|
||||
) {
|
||||
scriptMetadataPaths.push(normalizedPath);
|
||||
}
|
||||
|
||||
if (!YAML_FILE_REGEX.test(normalizedPath)) {
|
||||
continue;
|
||||
}
|
||||
@@ -727,6 +834,16 @@ export async function runLint(
|
||||
}
|
||||
}
|
||||
|
||||
// Unconditional: unlike a missing lock, metadata with no content file fails
|
||||
// every push, so there is no mode in which it is acceptable.
|
||||
issues.push(
|
||||
...(await checkOrphanScriptMetadata(
|
||||
syncRoot,
|
||||
metadataPrefix,
|
||||
scriptMetadataPaths,
|
||||
)),
|
||||
);
|
||||
|
||||
// Check for missing locks if --locks-required is set
|
||||
if (opts.locksRequired) {
|
||||
const lockIssues = await checkMissingLocks(opts, explicitTargetDirectory);
|
||||
@@ -820,6 +937,15 @@ async function lint(opts: LintOptions & { watch?: boolean }, directory?: string)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a changed file can change what a lint run reports: metadata in any of
|
||||
* its formats, and the content files whose presence is what keeps that metadata
|
||||
* from being an orphan.
|
||||
*/
|
||||
function affectsLint(filename: string): boolean {
|
||||
return /\.(ya?ml|json|lock)$/i.test(filename) || hasScriptExt(filename);
|
||||
}
|
||||
|
||||
async function lintWatch(opts: LintOptions, directory?: string) {
|
||||
const { watch } = await import("node:fs");
|
||||
const targetDir = directory ? path.resolve(process.cwd(), directory) : process.cwd();
|
||||
@@ -842,7 +968,7 @@ async function lintWatch(opts: LintOptions, directory?: string) {
|
||||
|
||||
let debounce: ReturnType<typeof setTimeout> | null = null;
|
||||
watch(targetDir, { recursive: true }, (_event, filename) => {
|
||||
if (!filename || !filename.toString().endsWith(".yaml") && !filename.toString().endsWith(".yml")) return;
|
||||
if (!filename || !affectsLint(filename.toString())) return;
|
||||
if (debounce) clearTimeout(debounce);
|
||||
debounce = setTimeout(runAndReport, 300);
|
||||
});
|
||||
@@ -853,7 +979,7 @@ async function lintWatch(opts: LintOptions, directory?: string) {
|
||||
|
||||
const command = new Command()
|
||||
.description(
|
||||
"Validate Windmill flow, schedule, and trigger YAML files in a directory",
|
||||
"Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file",
|
||||
)
|
||||
.arguments("[directory:string]")
|
||||
.option("--json", "Output results in JSON format")
|
||||
|
||||
@@ -1075,10 +1075,12 @@ export class DbtPathCollisionError extends UnresolvableScriptContentFileError {}
|
||||
* guard on one of them leaves the other silently overwriting.
|
||||
*/
|
||||
export async function collidingDbtProject(
|
||||
basePath: string
|
||||
basePath: string,
|
||||
baseDir?: string
|
||||
): Promise<string | undefined> {
|
||||
const project = basePath + "__dbt/dbt_project.yml";
|
||||
return (await stat(project).then(() => true).catch(() => false))
|
||||
const onDisk = baseDir ? path.join(baseDir, project) : project;
|
||||
return (await stat(onDisk).then(() => true).catch(() => false))
|
||||
? project
|
||||
: undefined;
|
||||
}
|
||||
@@ -1139,7 +1141,14 @@ async function readScriptContent(filePath: string): Promise<string> {
|
||||
}
|
||||
}
|
||||
|
||||
export async function findContentFile(filePath: string) {
|
||||
/**
|
||||
* The script file `filePath`'s metadata belongs to. `baseDir`, when given, is
|
||||
* where the disk lookups happen, leaving `filePath` classified as written: the
|
||||
* layout helpers below match their suffixes ANYWHERE in a path, so a caller
|
||||
* that prefixed a checkout named `repo__mod` would have it read as the module.
|
||||
*/
|
||||
export async function findContentFile(filePath: string, baseDir?: string) {
|
||||
const onDisk = (p: string) => (baseDir ? path.join(baseDir, p) : p);
|
||||
// Folder layout: __mod/script.yaml -> __mod/script.ts
|
||||
const isModuleFolderMeta = isModuleEntryMetadata(filePath);
|
||||
const toCandidate = (ext: string) =>
|
||||
@@ -1163,7 +1172,7 @@ export async function findContentFile(filePath: string) {
|
||||
const validCandidates = (
|
||||
await Promise.all(
|
||||
candidates.map((x) => {
|
||||
return stat(x)
|
||||
return stat(onDisk(x))
|
||||
.catch(() => undefined)
|
||||
.then((x) => x?.isFile())
|
||||
.then((e) => {
|
||||
@@ -1183,6 +1192,7 @@ export async function findContentFile(filePath: string) {
|
||||
const dbtCandidate = toCandidate("__dbt/" + DBT_DESCRIPTOR_NAME);
|
||||
const dbtProject = await collidingDbtProject(
|
||||
dbtCandidate.slice(0, -("__dbt/" + DBT_DESCRIPTOR_NAME).length),
|
||||
baseDir,
|
||||
);
|
||||
const nonDbtCandidates = validCandidates.filter((c) => c !== dbtCandidate);
|
||||
if (dbtProject && nonDbtCandidates.length > 0) {
|
||||
|
||||
Generated
+1
-1
@@ -7403,7 +7403,7 @@ Manage jobs (import/export)
|
||||
|
||||
### lint
|
||||
|
||||
Validate Windmill flow, schedule, and trigger YAML files in a directory
|
||||
Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file
|
||||
|
||||
**Arguments:** \`[directory:string]\`
|
||||
|
||||
|
||||
Reference in New Issue
Block a user