mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-30 16:17:25 +00:00
feat: rebuild raw apps on a ui/-only change, fail sync push on ui/ errors (#12051)
* feat: rebuild raw apps on a ui/-only change and fail sync push on ui/ errors Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: catch unquoted css ui/ imports and surface app lookup errors Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: match the raw app metadata file on windows sync paths Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
375bf03cdc
commit
d3872cb183
@@ -35,6 +35,7 @@ import {
|
||||
import type { PermissionedAsContext } from "../../core/permissioned_as.ts";
|
||||
import { buildPermissionedAsContext } from "../../core/permissioned_as.ts";
|
||||
import { createBundle, detectFrameworks } from "./bundle.ts";
|
||||
import { importsSharedUi, sharedUiHash } from "../shared_ui.ts";
|
||||
import { APP_BACKEND_FOLDER, RECORDINGS_FOLDER } from "./app_metadata.ts";
|
||||
import {
|
||||
NEVER_DEPLOYED_DIRS,
|
||||
@@ -488,6 +489,12 @@ export async function pushRawApp(
|
||||
if (localApp.data) {
|
||||
value.data = localApp.data;
|
||||
}
|
||||
// The bundle inlines ui/, so its content decides staleness as much as the
|
||||
// app's own files do. Only the CLI reads this key back.
|
||||
const uiHash = importsSharedUi(files) ? await sharedUiHash() : undefined;
|
||||
if (uiHash) {
|
||||
value.shared_ui_hash = uiHash;
|
||||
}
|
||||
|
||||
// extra_perms is synced independently via /acls/* — strip from the
|
||||
// up-to-date comparison so a perm-only edit doesn't trigger a rebuild +
|
||||
@@ -513,7 +520,9 @@ export async function pushRawApp(
|
||||
isSuperset({ ...localAppNoPerms, runnables }, app) &&
|
||||
changedCustomPath(customPath, app) === undefined &&
|
||||
isolationUpToDate(isolation, deployedPolicy, true);
|
||||
const filesUpToDate = deepEqual(files, app.value?.files);
|
||||
const filesUpToDate =
|
||||
deepEqual(files, app.value?.files) &&
|
||||
app.value?.shared_ui_hash === value.shared_ui_hash;
|
||||
if (metadataUpToDate && filesUpToDate) {
|
||||
log.info(colors.green(`App ${remotePath} is up to date`));
|
||||
} else {
|
||||
|
||||
@@ -1,9 +1,16 @@
|
||||
import * as fs from "node:fs";
|
||||
import * as path from "node:path";
|
||||
import { createHash } from "node:crypto";
|
||||
import * as log from "../core/log.ts";
|
||||
import { colors } from "@cliffy/ansi/colors";
|
||||
import * as wmill from "../../gen/services.gen.ts";
|
||||
import { readTextFile, readTextFileSync } from "../utils/utils.ts";
|
||||
import {
|
||||
extractFolderPath,
|
||||
extractResourceName,
|
||||
isRawAppFolderMetadataFile,
|
||||
} from "../utils/resource_folders.ts";
|
||||
import { APP_BACKEND_FOLDER } from "./app/app_metadata.ts";
|
||||
|
||||
const SHARED_UI_DIR = "ui";
|
||||
|
||||
@@ -26,6 +33,77 @@ async function readDirRecursive(
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether an app's own files import from the shared `/ui/` folder. The `(`
|
||||
* covers CSS's unquoted `@import url(/ui/theme.css)`.
|
||||
*/
|
||||
export function importsSharedUi(files: Record<string, string>): boolean {
|
||||
return Object.values(files).some((c) => /["'`(]\s*\/ui\//.test(c));
|
||||
}
|
||||
|
||||
/**
|
||||
* A digest of the local <cwd>/ui/ folder, or undefined without one. An app
|
||||
* bundles ui/ into itself, so this is what tells a push that the deployed
|
||||
* bundle was built against other ui/ content than the local one.
|
||||
*/
|
||||
export async function sharedUiHash(): Promise<string | undefined> {
|
||||
const localDir = path.join(process.cwd(), SHARED_UI_DIR);
|
||||
if (!fs.existsSync(localDir)) return undefined;
|
||||
const files = await readDirRecursive(localDir);
|
||||
const entries = Object.entries(files).sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0));
|
||||
return createHash("sha256").update(JSON.stringify(entries)).digest("hex");
|
||||
}
|
||||
|
||||
/**
|
||||
* The `raw_app.yaml` keys of the local raw apps that import ui/ and whose
|
||||
* deployed bundle was built from other ui/ content: no file of theirs changed,
|
||||
* so the file diff never queues them. Apps in `skip` are already being pushed.
|
||||
*/
|
||||
export async function rawAppsStaleOnSharedUi(
|
||||
workspace: string,
|
||||
localMap: Record<string, string>,
|
||||
skip: Set<string>,
|
||||
): Promise<string[]> {
|
||||
const hash = await sharedUiHash();
|
||||
if (!hash) return [];
|
||||
const apps = new Map<
|
||||
string,
|
||||
{ metaPath?: string; files: Record<string, string> }
|
||||
>();
|
||||
for (const [key, content] of Object.entries(localMap)) {
|
||||
const name = extractResourceName(key, "raw_app");
|
||||
if (!name || skip.has(name)) continue;
|
||||
if (!apps.has(name)) apps.set(name, { files: {} });
|
||||
const app = apps.get(name)!;
|
||||
const posix = key.replaceAll("\\", "/");
|
||||
const rel = posix.slice(extractFolderPath(key, "raw_app")!.length);
|
||||
if (isRawAppFolderMetadataFile(posix)) {
|
||||
app.metaPath = key;
|
||||
} else if (!rel.startsWith(APP_BACKEND_FOLDER + "/")) {
|
||||
app.files[key] = content;
|
||||
}
|
||||
}
|
||||
const stale = await Promise.all(
|
||||
[...apps].map(async ([name, { metaPath, files }]) => {
|
||||
if (!metaPath || !importsSharedUi(files)) return undefined;
|
||||
try {
|
||||
const app = await wmill.getAppByPath({
|
||||
workspace,
|
||||
path: name.replaceAll(path.sep, "/"),
|
||||
});
|
||||
return (app.value as any)?.shared_ui_hash === hash
|
||||
? undefined
|
||||
: metaPath;
|
||||
} catch (e: any) {
|
||||
// Not deployed yet: the file diff already queues it as added.
|
||||
if (e?.status === 404) return undefined;
|
||||
throw e;
|
||||
}
|
||||
}),
|
||||
);
|
||||
return stale.filter((p): p is string => !!p);
|
||||
}
|
||||
|
||||
/** The workspace's shared UI store, or undefined when it cannot be read. */
|
||||
async function fetchSharedUi(
|
||||
workspace: string,
|
||||
|
||||
@@ -44,7 +44,12 @@ import {
|
||||
} from "../../types.ts";
|
||||
import { downloadZip } from "./pull.ts";
|
||||
import { runLint, printReport, checkMissingLocks } from "../lint/lint.ts";
|
||||
import { diffSharedUi, pullSharedUi, pushSharedUi } from "../shared_ui.ts";
|
||||
import {
|
||||
diffSharedUi,
|
||||
pullSharedUi,
|
||||
pushSharedUi,
|
||||
rawAppsStaleOnSharedUi,
|
||||
} from "../shared_ui.ts";
|
||||
import {
|
||||
pushMigrationFromDisk,
|
||||
offerToRunNewMigrations,
|
||||
@@ -5189,6 +5194,30 @@ export async function push(
|
||||
);
|
||||
}
|
||||
|
||||
// A raw app inlines ui/ into its bundle, so ui/ content its deployed bundle
|
||||
// was not built from makes it stale with no file of its own changed.
|
||||
try {
|
||||
const pushedApps = new Set(
|
||||
changes
|
||||
.map((c) => extractResourceName(c.path, "raw_app"))
|
||||
.filter((n): n is string => !!n),
|
||||
);
|
||||
for (const metaPath of await rawAppsStaleOnSharedUi(
|
||||
workspace.workspaceId,
|
||||
localMap,
|
||||
pushedApps,
|
||||
)) {
|
||||
changes.push({
|
||||
name: "edited",
|
||||
path: metaPath,
|
||||
before: localMap[metaPath],
|
||||
after: localMap[metaPath],
|
||||
});
|
||||
}
|
||||
} catch (e) {
|
||||
log.warn(`Could not check raw apps against the shared UI folder: ${e}`);
|
||||
}
|
||||
|
||||
// After the shared-lock pass, which reads a shared lockfile's deletion as the
|
||||
// signal that this checkout is not deduplicated — an advisory about the local
|
||||
// tree that holds whether or not remote items are being kept.
|
||||
@@ -6748,7 +6777,8 @@ export async function push(
|
||||
try {
|
||||
await pushSharedUi(workspace.workspaceId, opts.keepDeleted);
|
||||
} catch (e) {
|
||||
log.warn(`Failed to push shared UI folder: ${e}`);
|
||||
log.error(colors.red(`Failed to push shared UI folder: ${e}`));
|
||||
failedChanges.push({ path: "ui/", error: String(e) });
|
||||
}
|
||||
try {
|
||||
await offerToRunNewMigrations(
|
||||
@@ -6784,7 +6814,9 @@ export async function push(
|
||||
);
|
||||
}
|
||||
}
|
||||
const pushedCount = changes.length - failedChanges.length;
|
||||
// ui/ is pushed outside `changes`, so its failure is not one of them.
|
||||
const pushedCount =
|
||||
changes.length - failedChanges.filter((f) => f.path !== "ui/").length;
|
||||
if (opts.jsonOutput) {
|
||||
const result = {
|
||||
success: failedChanges.length === 0,
|
||||
@@ -6862,6 +6894,7 @@ export async function push(
|
||||
// Dry-run with no changes reaches here (a ui/ diff would have made changes
|
||||
// non-empty and returned above); never mutate the remote in that case.
|
||||
let sharedUiPushed = false;
|
||||
let sharedUiError: string | undefined;
|
||||
if (!opts.dryRun) {
|
||||
try {
|
||||
sharedUiPushed = await pushSharedUi(
|
||||
@@ -6869,7 +6902,9 @@ export async function push(
|
||||
opts.keepDeleted,
|
||||
);
|
||||
} catch (e) {
|
||||
log.warn(`Failed to push shared UI folder: ${e}`);
|
||||
sharedUiError = String(e);
|
||||
log.error(colors.red(`Failed to push shared UI folder: ${e}`));
|
||||
process.exitCode = 1;
|
||||
}
|
||||
}
|
||||
// No changes pushed, so no new datatable migrations to run.
|
||||
@@ -6879,10 +6914,15 @@ export async function push(
|
||||
console.log(
|
||||
JSON.stringify(
|
||||
{
|
||||
success: true,
|
||||
message: sharedUiPushed
|
||||
success: sharedUiError === undefined,
|
||||
message: sharedUiError !== undefined
|
||||
? "Failed to push shared UI changes"
|
||||
: sharedUiPushed
|
||||
? "Pushed shared UI changes"
|
||||
: "No changes to push",
|
||||
...(sharedUiError !== undefined
|
||||
? { failed: [{ path: "ui/", error: sharedUiError }] }
|
||||
: {}),
|
||||
total: 0,
|
||||
},
|
||||
null,
|
||||
|
||||
@@ -14,6 +14,7 @@ import * as path from "node:path";
|
||||
let remoteFiles: Record<string, string> = {};
|
||||
let remoteUnreadable = false;
|
||||
let pushedFiles: Record<string, string> | undefined;
|
||||
let deployedApps: Record<string, { value: Record<string, unknown> }> = {};
|
||||
|
||||
mockServices({
|
||||
getSharedUi: async (_args: { workspace: string }) => {
|
||||
@@ -26,9 +27,15 @@ mockServices({
|
||||
}) => {
|
||||
pushedFiles = args.requestBody.files;
|
||||
},
|
||||
getAppByPath: async (args: { workspace: string; path: string }) => {
|
||||
const app = deployedApps[args.path];
|
||||
if (!app) throw Object.assign(new Error("not found"), { status: 404 });
|
||||
return app;
|
||||
},
|
||||
});
|
||||
|
||||
const { diffSharedUi, pushSharedUi } = await import(
|
||||
const { diffSharedUi, pushSharedUi, rawAppsStaleOnSharedUi, sharedUiHash } =
|
||||
await import(
|
||||
"../src/commands/shared_ui.ts"
|
||||
);
|
||||
|
||||
@@ -148,3 +155,46 @@ describe("pushSharedUi with keepDeleted", () => {
|
||||
.toEqual("keep me");
|
||||
});
|
||||
});
|
||||
|
||||
describe("rawAppsStaleOnSharedUi", () => {
|
||||
let tmpDir: string;
|
||||
let prevCwd: string;
|
||||
|
||||
beforeEach(() => {
|
||||
prevCwd = process.cwd();
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "wm-shared-ui-apps-"));
|
||||
process.chdir(tmpDir);
|
||||
fs.mkdirSync(path.join(tmpDir, "ui"), { recursive: true });
|
||||
fs.writeFileSync(path.join(tmpDir, "ui", "button.ts"), "v2", "utf-8");
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
process.chdir(prevCwd);
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
const app = (name: string, index: string) => ({
|
||||
[path.join("f", `${name}.raw_app`, "raw_app.yaml")]: "summary: x",
|
||||
[path.join("f", `${name}.raw_app`, "index.tsx")]: index,
|
||||
});
|
||||
|
||||
test("queues an app importing ui/ only when its bundle was built from other ui/ content", async () => {
|
||||
const hash = await sharedUiHash();
|
||||
deployedApps = {
|
||||
"f/stale": { value: { shared_ui_hash: "old" } },
|
||||
"f/fresh": { value: { shared_ui_hash: hash } },
|
||||
"f/plain": { value: {} },
|
||||
"f/css": { value: {} },
|
||||
};
|
||||
const localMap = {
|
||||
...app("stale", `import { B } from "/ui/button"`),
|
||||
...app("fresh", `import { B } from "/ui/button"`),
|
||||
...app("plain", `import x from "./x"`),
|
||||
...app("css", `@import url(/ui/theme.css);`),
|
||||
};
|
||||
expect(await rawAppsStaleOnSharedUi("ws", localMap, new Set())).toEqual([
|
||||
path.join("f", "stale.raw_app", "raw_app.yaml"),
|
||||
path.join("f", "css.raw_app", "raw_app.yaml"),
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user