feat(draft): resolve a repo's GitHub App by source_id, not the only-App guess (#72)
`inventory --emit-draft` wrote the `github_apps` binding by guessing: with
exactly one App on the instance it bound every repo to it ("no other it
could be"), and with none or several it left a REVIEW marker on all of them.
The audit (#72) showed the binding is READABLE, so the guess was both
unnecessary and, on a single-App instance, silently WRONG for any public
repo (a repo cloned without a GitHub App got bound to the one App anyway).
Every application carries the `source_id`/`source_type` of the App that
clones it — `removeSensitiveData` hides neither (ApplicationsController
v4.1.2) — and `GET /github-apps` returns each App's `id` and `name` (only
`client_secret`/`webhook_secret` are hidden). So the draft now matches the
two: each repo binds to the App its application's `source_id` names. A
GitlabApp/public-repo source (or an instance that will not list its Apps)
resolves to nothing and still gets a REVIEW marker — and a `source_id` that
collides with an App id but carries a non-GithubApp `source_type` is not
mistaken for one.
The biggest gain is the multi-App instance the old heuristic could not
handle at all: it wrote REVIEW on every repo; the lookup resolves each.
semantics.md, the draft header, and the NO_API_COVERAGE row are corrected to
match (the audit's #51 arc: a limitation filed as a defect gets fixed).
`npm run check` clean · 511 tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
e3e52680e1
commit
2cbcfd2ef3
5 changed files with 165 additions and 38 deletions
|
|
@ -1097,10 +1097,14 @@ registry's own parse-time refusal (*"a registry key has no meaning without its
|
||||||
org"*) then stops the file being used until a human supplies it. That refusal is
|
org"*) then stops the file being used until a human supplies it. That refusal is
|
||||||
the design: the alternatives are inventing an org, or leaving the project out of
|
the design: the alternatives are inventing an org, or leaving the project out of
|
||||||
the registry — and a project missing from the registry is one every fleet run
|
the registry — and a project missing from the registry is one every fleet run
|
||||||
skips **in silence**. Likewise `github_apps`: nothing Coolify returns about an
|
skips **in silence**. `github_apps`, by contrast, IS readable (#72): an
|
||||||
application says which App cloned it, so cast binds every repo to the instance's
|
application carries the `source_id`/`source_type` of the App that clones it
|
||||||
only GitHub App when there is exactly one (there is no other it could be), and
|
(`removeSensitiveData` hides neither), and `GET /github-apps` returns each App's
|
||||||
writes a `REVIEW-…` marker when there is not.
|
`id` and `name` (only `client_secret`/`webhook_secret` are hidden) — so the draft
|
||||||
|
resolves each repo's App by matching the two, a lookup rather than the only-App
|
||||||
|
guess it used to make (which was silently wrong for any public repo even on a
|
||||||
|
single-App instance). A repo whose application has no GithubApp source, or an
|
||||||
|
instance that will not list its Apps, still gets a `REVIEW-…` marker.
|
||||||
|
|
||||||
## Teardown (`cast destroy`)
|
## Teardown (`cast destroy`)
|
||||||
|
|
||||||
|
|
|
||||||
20
src/cli.ts
20
src/cli.ts
|
|
@ -1867,13 +1867,14 @@ async function main(): Promise<number> {
|
||||||
otherEnvironments: others,
|
otherEnvironments: others,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
// Which GitHub App clones a repo is NOT a property of any resource — no
|
// Which GitHub App clones a repo IS readable (cast#72): an application
|
||||||
// field Coolify returns about an application says so. What the instance can
|
// carries `source_id`/`source_type` (removeSensitiveData hides neither),
|
||||||
// answer is which Apps exist; with exactly one, there is no other it could
|
// and GET /github-apps returns each App's `id` and `name` (only the
|
||||||
// be. Best-effort: an instance that will not list them still gets a draft,
|
// secrets are hidden) — so bindingsDoc resolves the binding by matching the
|
||||||
// with a REVIEW marker where the binding goes.
|
// two, instead of guessing the only App. The list is still best-effort: an
|
||||||
|
// instance that will not answer it leaves a REVIEW marker on every repo.
|
||||||
const githubApps = (await client.get("/github-apps").catch(() => [])) as
|
const githubApps = (await client.get("/github-apps").catch(() => [])) as
|
||||||
| Array<{ name?: unknown }>
|
| Array<{ id?: unknown; name?: unknown }>
|
||||||
| undefined;
|
| undefined;
|
||||||
const draftCtx = {
|
const draftCtx = {
|
||||||
env: envName,
|
env: envName,
|
||||||
|
|
@ -1882,8 +1883,11 @@ async function main(): Promise<number> {
|
||||||
team,
|
team,
|
||||||
server: sweepBinding.server,
|
server: sweepBinding.server,
|
||||||
githubApps: (Array.isArray(githubApps) ? githubApps : [])
|
githubApps: (Array.isArray(githubApps) ? githubApps : [])
|
||||||
.map((a) => a?.name)
|
.filter(
|
||||||
.filter((n): n is string => typeof n === "string"),
|
(a): a is { id: number; name: string } =>
|
||||||
|
typeof a?.id === "number" && typeof a?.name === "string",
|
||||||
|
)
|
||||||
|
.map((a) => ({ id: a.id, name: a.name })),
|
||||||
recipient,
|
recipient,
|
||||||
generatedAt: new Date().toISOString(),
|
generatedAt: new Date().toISOString(),
|
||||||
};
|
};
|
||||||
|
|
|
||||||
66
src/draft.ts
66
src/draft.ts
|
|
@ -266,12 +266,13 @@ export type DraftContext = {
|
||||||
baseUrl: string;
|
baseUrl: string;
|
||||||
team: { id: number; name: string };
|
team: { id: number; name: string };
|
||||||
server?: string;
|
server?: string;
|
||||||
// The GitHub Apps configured on the instance, by name. NOT a property of any
|
// The GitHub Apps configured on the instance, each with the `id` an
|
||||||
// resource: nothing Coolify returns about an application says which App clones
|
// application's `source_id` points at (cast#72). bindingsDoc resolves a repo's
|
||||||
// it. With exactly one on the instance there is no other it could be, and cast
|
// App by that match — a READ, not the only-App guess this used to make — and
|
||||||
// binds every repo to it; with none or several it writes a REVIEW marker
|
// falls back to a REVIEW marker only when a resource carries no resolvable
|
||||||
// instead of picking. See bindingsDoc.
|
// source (a public repo, or an instance that would not list its Apps). See
|
||||||
githubApps?: string[];
|
// bindingsDoc.
|
||||||
|
githubApps?: Array<{ id: number; name: string }>;
|
||||||
recipient?: string;
|
recipient?: string;
|
||||||
generatedAt: string;
|
generatedAt: string;
|
||||||
};
|
};
|
||||||
|
|
@ -806,7 +807,7 @@ const NO_API_COVERAGE: Array<[string, string]> = [
|
||||||
],
|
],
|
||||||
[
|
[
|
||||||
"which GitHub App clones a repo",
|
"which GitHub App clones a repo",
|
||||||
"nothing Coolify returns about an application says so. cast binds every repo to the instance's only App when there is exactly one, and writes a REVIEW marker when there is not.",
|
"resolved from the application's `source_id` against GET /github-apps (cast#72) — a lookup, not a guess. A repo whose application has no GithubApp source (a public repo) or an instance that will not list its Apps gets a REVIEW marker instead.",
|
||||||
],
|
],
|
||||||
[
|
[
|
||||||
"anything configured in the UI with no manifest field",
|
"anything configured in the UI with no manifest field",
|
||||||
|
|
@ -1078,11 +1079,11 @@ export function planDraft(
|
||||||
"`projects:`, the registry: the list of what exists, which nothing before a",
|
"`projects:`, the registry: the list of what exists, which nothing before a",
|
||||||
"whole-instance sweep was able to write down.",
|
"whole-instance sweep was able to write down.",
|
||||||
"",
|
"",
|
||||||
"`github_apps` is NOT readable from a box: nothing Coolify returns about an",
|
"`github_apps` IS readable (cast#72): an application carries the `source_id`",
|
||||||
`application says which App clones it. This instance has ${ctx.githubApps?.length ?? 0}, so cast`,
|
`of the App that clones it, and this instance lists ${ctx.githubApps?.length ?? 0}, so cast`,
|
||||||
ctx.githubApps?.length === 1
|
"resolved each repo's App by that match. A repo whose application resolves to",
|
||||||
? `bound every repo to the only one there is (${ctx.githubApps[0]}) — there is no other it could be.`
|
"none (a public repo, or an unlistable instance) gets a `REVIEW-…` marker;",
|
||||||
: "left a REVIEW marker on every repo rather than pick. `apply` will refuse until you fix them.",
|
"`apply` refuses on those until you fix them.",
|
||||||
"",
|
"",
|
||||||
"Do not copy it over a state file you already have. Merge the registry into",
|
"Do not copy it over a state file you already have. Merge the registry into",
|
||||||
"yours, by hand, having decided which of these projects are yours to declare.",
|
"yours, by hand, having decided which of these projects are yours to declare.",
|
||||||
|
|
@ -1128,6 +1129,28 @@ function registryKey(p: DraftProject): string {
|
||||||
return slug(p.name);
|
return slug(p.name);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The GitHub App a project's repo is cloned by, resolved from an APPLICATION's
|
||||||
|
// `source_id` against the instance's Apps (cast#72). Coolify hides neither
|
||||||
|
// `source_id` nor `source_type` on an application, and GET /github-apps returns
|
||||||
|
// each App's `id` — so this is a lookup, not a guess. Only a GithubApp source
|
||||||
|
// counts: a public-repo application has a different `source_type` and no App to
|
||||||
|
// bind, and a numeric `source_id` that happens to collide with an App id must
|
||||||
|
// not be mistaken for one. The first application that resolves wins; a project
|
||||||
|
// whose apps resolve to none gets a REVIEW marker (see bindingsDoc).
|
||||||
|
function githubAppNameForProject(
|
||||||
|
p: DraftProject,
|
||||||
|
appById: Map<number, string>,
|
||||||
|
): string | undefined {
|
||||||
|
for (const r of p.resources) {
|
||||||
|
if (r.kind !== "application") continue;
|
||||||
|
if (!/github.?app/i.test(String(r.raw.source_type ?? ""))) continue;
|
||||||
|
const id = r.raw.source_id;
|
||||||
|
const name = typeof id === "number" ? appById.get(id) : undefined;
|
||||||
|
if (name) return name;
|
||||||
|
}
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
|
||||||
// The bindings the box implies — plus `projects:`, THE REGISTRY (#25): the list
|
// The bindings the box implies — plus `projects:`, THE REGISTRY (#25): the list
|
||||||
// of what exists, which nothing before a whole-instance sweep was in a position
|
// of what exists, which nothing before a whole-instance sweep was in a position
|
||||||
// to write down. A rebuild cannot even be attempted without it, because you
|
// to write down. A rebuild cannot even be attempted without it, because you
|
||||||
|
|
@ -1135,20 +1158,23 @@ function registryKey(p: DraftProject): string {
|
||||||
function bindingsDoc(projects: DraftProject[], ctx: DraftContext) {
|
function bindingsDoc(projects: DraftProject[], ctx: DraftContext) {
|
||||||
const registry: Record<string, { environments: string[] }> = {};
|
const registry: Record<string, { environments: string[] }> = {};
|
||||||
const githubApps: Record<string, string> = {};
|
const githubApps: Record<string, string> = {};
|
||||||
// With exactly one GitHub App on the instance there is no other one an
|
// Resolve each repo's App by the read, not a guess (cast#72): an application
|
||||||
// application could have been cloned by, so binding every repo to it is a fact,
|
// carries the `source_id` of the App that clones it, and ctx.githubApps carries
|
||||||
// not a guess. With none or several it IS a guess, and cast does not make it:
|
// each App's `id` and `name`. A guess is what this used to be — binding every
|
||||||
// a wrong App resolves to a real uuid and clones the wrong repo, silently
|
// repo to the only App there was — and it was silently wrong for any public
|
||||||
// (githubAppNameFor, #12). A REVIEW marker resolves to nothing, and `apply`
|
// repo (source_type is not a GithubApp) even on a single-App instance, and
|
||||||
// says so.
|
// unmakeable on a multi-App one. A REVIEW marker resolves to nothing and
|
||||||
const onlyApp = ctx.githubApps?.length === 1 ? ctx.githubApps[0] : undefined;
|
// `apply` says so; a wrong App resolves to a real uuid and clones the wrong
|
||||||
|
// repo, silently (githubAppNameFor, #12).
|
||||||
|
const appById = new Map((ctx.githubApps ?? []).map((a) => [a.id, a.name]));
|
||||||
for (const p of projects) {
|
for (const p of projects) {
|
||||||
// Only what the draft actually carries a manifest for. See planDraft.
|
// Only what the draft actually carries a manifest for. See planDraft.
|
||||||
if (p.resources.length === 0) continue;
|
if (p.resources.length === 0) continue;
|
||||||
const repo = registryKey(p);
|
const repo = registryKey(p);
|
||||||
registry[repo] = { environments: [ctx.env] };
|
registry[repo] = { environments: [ctx.env] };
|
||||||
githubApps[repo] =
|
githubApps[repo] =
|
||||||
onlyApp ?? "REVIEW-which-github-app-in-coolify-clones-this-repo";
|
githubAppNameForProject(p, appById) ??
|
||||||
|
"REVIEW-which-github-app-in-coolify-clones-this-repo";
|
||||||
}
|
}
|
||||||
return {
|
return {
|
||||||
environments: {
|
environments: {
|
||||||
|
|
|
||||||
|
|
@ -62,10 +62,12 @@ async function stubCoolify(opts: { ambiguous?: boolean } = {}): Promise<Stub> {
|
||||||
res.end(JSON.stringify(body));
|
res.end(JSON.stringify(body));
|
||||||
};
|
};
|
||||||
if (path === "/teams/current") return json({ id: 0, name: "Root Team" });
|
if (path === "/teams/current") return json({ id: 0, name: "Root Team" });
|
||||||
// Exactly one App: there is no other one an application could have been
|
// GET /github-apps returns each App's `id` and `name` (cast#72). The
|
||||||
// cloned by, so cast binds every repo to it rather than leave a marker.
|
// incubator app below carries `source_id: 7`, so the binding RESOLVES to
|
||||||
|
// this App by the read; the third-party public repo has no GithubApp source
|
||||||
|
// and gets a REVIEW marker instead of being wrongly bound to it.
|
||||||
if (path === "/github-apps")
|
if (path === "/github-apps")
|
||||||
return json([{ uuid: "g1", name: "hdb-coolify" }]);
|
return json([{ id: 7, uuid: "g1", name: "hdb-coolify" }]);
|
||||||
if (path === "/projects")
|
if (path === "/projects")
|
||||||
return json([
|
return json([
|
||||||
{ uuid: "p1", name: "Incubator" },
|
{ uuid: "p1", name: "Incubator" },
|
||||||
|
|
@ -95,6 +97,10 @@ async function stubCoolify(opts: { ambiguous?: boolean } = {}): Promise<Stub> {
|
||||||
uuid: "a1",
|
uuid: "a1",
|
||||||
git_repository: "heavy-duty/incubator",
|
git_repository: "heavy-duty/incubator",
|
||||||
git_branch: "main",
|
git_branch: "main",
|
||||||
|
// Cloned by the GitHub App id 7 (hdb-coolify) — source_id/source_type
|
||||||
|
// survive serialization, so the binding is resolvable (cast#72).
|
||||||
|
source_id: 7,
|
||||||
|
source_type: "App\\Models\\GithubApp",
|
||||||
build_pack: "dockercompose",
|
build_pack: "dockercompose",
|
||||||
base_directory: "/",
|
base_directory: "/",
|
||||||
docker_compose_location: "/docker-compose.yaml",
|
docker_compose_location: "/docker-compose.yaml",
|
||||||
|
|
@ -386,10 +392,15 @@ describe("cast inventory --emit-draft (#27)", () => {
|
||||||
// The bindings the sweep could actually read.
|
// The bindings the sweep could actually read.
|
||||||
expect(yaml).toContain("server: box-b");
|
expect(yaml).toContain("server: box-b");
|
||||||
expect(yaml).toContain("name: Root Team");
|
expect(yaml).toContain("name: Root Team");
|
||||||
// Which GitHub App clones a repo is not on any resource — but this instance
|
// Which GitHub App clones a repo is READ from the application's source_id,
|
||||||
// has exactly one, and there is no other it could be.
|
// not guessed (cast#72): the incubator app's source_id: 7 resolves to
|
||||||
|
// hdb-coolify, while the third-party PUBLIC repo (no GithubApp source) gets a
|
||||||
|
// REVIEW marker rather than being wrongly bound to the only App there is.
|
||||||
expect(yaml).toContain("github_apps:");
|
expect(yaml).toContain("github_apps:");
|
||||||
expect(yaml).toContain("third-party/la-familia: hdb-coolify");
|
expect(yaml).toContain("heavy-duty/incubator: hdb-coolify");
|
||||||
|
expect(yaml).toMatch(
|
||||||
|
/third-party\/la-familia: REVIEW-which-github-app-in-coolify-clones-this-repo/,
|
||||||
|
);
|
||||||
|
|
||||||
// The barber shop has no application, so the box knows no repo for it. cast
|
// The barber shop has no application, so the box knows no repo for it. cast
|
||||||
// writes the bare project name — and the registry's own parse-time refusal
|
// writes the bare project name — and the registry's own parse-time refusal
|
||||||
|
|
|
||||||
|
|
@ -57,6 +57,88 @@ const project = (over: Partial<DraftProject> = {}): DraftProject => ({
|
||||||
...over,
|
...over,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("github_apps binding — resolved by source_id, not guessed (cast#72)", () => {
|
||||||
|
const appProject = (
|
||||||
|
name: string,
|
||||||
|
repo: string,
|
||||||
|
source?: { source_id: number; source_type: string },
|
||||||
|
): DraftProject => ({
|
||||||
|
name,
|
||||||
|
coolifyEnv: "staging",
|
||||||
|
resources: [
|
||||||
|
{
|
||||||
|
kind: "application",
|
||||||
|
name,
|
||||||
|
uuid: `u-${name}`,
|
||||||
|
raw: {
|
||||||
|
git_repository: `https://github.com/${repo}`,
|
||||||
|
git_branch: "main",
|
||||||
|
build_pack: "nixpacks",
|
||||||
|
base_directory: "/",
|
||||||
|
fqdn: "https://x.example.com",
|
||||||
|
...source,
|
||||||
|
},
|
||||||
|
env: {},
|
||||||
|
},
|
||||||
|
],
|
||||||
|
unreadable: [],
|
||||||
|
otherEnvironments: [],
|
||||||
|
});
|
||||||
|
const bindings = (plan: ReturnType<typeof planDraft>) =>
|
||||||
|
plan.files.find((f) => f.path === "environments.yaml")?.content ?? "";
|
||||||
|
|
||||||
|
// The payoff the old only-App heuristic could not deliver: with MORE THAN ONE
|
||||||
|
// App it used to write a REVIEW marker on every repo. source_id resolves each.
|
||||||
|
it("binds each repo to the App its source_id names, even with several Apps", () => {
|
||||||
|
const ghApp = "App\\Models\\GithubApp";
|
||||||
|
const plan = planDraft(
|
||||||
|
[
|
||||||
|
appProject("acme-api", "acme/api", {
|
||||||
|
source_id: 7,
|
||||||
|
source_type: ghApp,
|
||||||
|
}),
|
||||||
|
appProject("beta-web", "beta/web", {
|
||||||
|
source_id: 9,
|
||||||
|
source_type: ghApp,
|
||||||
|
}),
|
||||||
|
],
|
||||||
|
{
|
||||||
|
...ctx,
|
||||||
|
githubApps: [
|
||||||
|
{ id: 7, name: "acme-app" },
|
||||||
|
{ id: 9, name: "beta-app" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
);
|
||||||
|
const yaml = bindings(plan);
|
||||||
|
expect(yaml).toContain("acme/api: acme-app");
|
||||||
|
expect(yaml).toContain("beta/web: beta-app");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("leaves a REVIEW marker for a public repo (no GithubApp source)", () => {
|
||||||
|
const plan = planDraft([appProject("pub", "acme/public")], {
|
||||||
|
...ctx,
|
||||||
|
githubApps: [{ id: 7, name: "acme-app" }],
|
||||||
|
});
|
||||||
|
expect(bindings(plan)).toMatch(/acme\/public: REVIEW-/);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not mistake a non-GithubApp source whose id collides with an App id", () => {
|
||||||
|
const plan = planDraft(
|
||||||
|
[
|
||||||
|
appProject("gitlab", "acme/gl", {
|
||||||
|
source_id: 7,
|
||||||
|
source_type: "App\\Models\\GitlabApp",
|
||||||
|
}),
|
||||||
|
],
|
||||||
|
{ ...ctx, githubApps: [{ id: 7, name: "acme-app" }] },
|
||||||
|
);
|
||||||
|
// id 7 exists as a GitHub App, but this app's source is a GitlabApp — the
|
||||||
|
// collision must not bind it to acme-app.
|
||||||
|
expect(bindings(plan)).toMatch(/acme\/gl: REVIEW-/);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("isProviderGenerated — the one judgment that must not be wrong", () => {
|
describe("isProviderGenerated — the one judgment that must not be wrong", () => {
|
||||||
it("recognizes the datastore families whose value points at the SOURCE box", () => {
|
it("recognizes the datastore families whose value points at the SOURCE box", () => {
|
||||||
for (const key of [
|
for (const key of [
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue