-
Notifications
You must be signed in to change notification settings - Fork 74
refactor(project): move build behind a project backend #2055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { afterEach, describe, expect, test } from "bun:test"; | ||
| import { mkdir, mkdtemp, rm } from "node:fs/promises"; | ||
| import { join } from "node:path"; | ||
| import { tmpdir } from "node:os"; | ||
| import { ProjectSpecSchema } from "../../../projectSchemas/project"; | ||
| import { createSilentLogger } from "../../../testing"; | ||
| import type { Project, ProjectEvent } from "../../../handlers/project/types"; | ||
| import { CdkBackend } from "./cdk"; | ||
|
|
||
| const tempDirectories: string[] = []; | ||
|
|
||
| afterEach(async () => { | ||
| await Promise.all( | ||
| tempDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true })), | ||
| ); | ||
| }); | ||
|
|
||
| async function project(withDependencies = true): Promise<Project> { | ||
| const rootPath = await mkdtemp(join(tmpdir(), "agentcore-cdk-backend-")); | ||
| tempDirectories.push(rootPath); | ||
| const cdkDir = join(rootPath, "agentcore", "cdk"); | ||
| await mkdir(withDependencies ? join(cdkDir, "node_modules") : cdkDir, { recursive: true }); | ||
| return { | ||
| name: "example", | ||
| rootPath, | ||
| spec: ProjectSpecSchema.parse({ name: "example", version: 1 }), | ||
| }; | ||
| } | ||
|
|
||
| async function drain(generator: AsyncGenerator<ProjectEvent, void>): Promise<ProjectEvent[]> { | ||
| const events: ProjectEvent[] = []; | ||
| for await (const event of generator) events.push(event); | ||
| return events; | ||
| } | ||
|
|
||
| describe("CdkBackend.build", () => { | ||
| test("compiles and synthesizes through the generated CDK script", async () => { | ||
| const commands: { command: string[]; cwd: string }[] = []; | ||
| const subject = new CdkBackend({ | ||
| logger: createSilentLogger(), | ||
| runner: async (command, { cwd }) => { | ||
| commands.push({ command, cwd }); | ||
| }, | ||
| checkTool: async () => {}, | ||
| }); | ||
| const input = await project(); | ||
|
|
||
| expect(await drain(subject.build(input))).toEqual([ | ||
| { message: "Synthesizing CloudFormation templates" }, | ||
| ]); | ||
| expect(commands).toEqual([ | ||
| { | ||
| command: ["npm", "run", "cdk", "--", "synth", "--quiet"], | ||
| cwd: join(input.rootPath, "agentcore", "cdk"), | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
||
| test("fails actionably when CDK dependencies are missing", async () => { | ||
| const commands: string[][] = []; | ||
| const subject = new CdkBackend({ | ||
| logger: createSilentLogger(), | ||
| runner: async (command) => { | ||
| commands.push(command); | ||
| }, | ||
| checkTool: async () => {}, | ||
| }); | ||
|
|
||
| await expect(drain(subject.build(await project(false)))).rejects.toThrow(/npm install/); | ||
| expect(commands).toEqual([]); | ||
| }); | ||
|
|
||
| test("propagates synthesis failures", async () => { | ||
| const subject = new CdkBackend({ | ||
| logger: createSilentLogger(), | ||
| runner: async () => { | ||
| throw new Error("cdk synth exploded"); | ||
| }, | ||
| checkTool: async () => {}, | ||
| }); | ||
|
|
||
| await expect(drain(subject.build(await project()))).rejects.toThrow("cdk synth exploded"); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { existsSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { ProjectStateError } from "../../../errors/errors"; | ||
| import type { Project, ProjectEvent } from "../../../handlers/project/types"; | ||
| import { requireTool, runProcess, type ProcessRunner } from "../../../io"; | ||
| import type { Logger } from "../../../logging"; | ||
| import type { ProjectBackend } from "./types"; | ||
|
|
||
| export type CdkBackendConfig = { | ||
| logger: Logger; | ||
| runner?: ProcessRunner; | ||
| checkTool?: typeof requireTool; | ||
| }; | ||
|
|
||
| /** Builds projects through the CDK app scaffolded by `agentcore project create`. */ | ||
| export class CdkBackend implements ProjectBackend { | ||
| private readonly logger: Logger; | ||
| private readonly runner: ProcessRunner; | ||
| private readonly checkTool: typeof requireTool; | ||
|
|
||
| constructor(config: CdkBackendConfig) { | ||
| this.logger = config.logger; | ||
| this.runner = config.runner ?? runProcess; | ||
| this.checkTool = config.checkTool ?? requireTool; | ||
| } | ||
|
|
||
| public async *build(project: Project): AsyncGenerator<ProjectEvent, void> { | ||
| const cdkDir = join(project.rootPath, "agentcore", "cdk"); | ||
|
|
||
| if (!existsSync(join(cdkDir, "node_modules"))) { | ||
| throw new ProjectStateError( | ||
| `CDK dependencies are missing for project '${project.name}'. ` + | ||
| `Run 'cd ${cdkDir} && npm install'.`, | ||
| ); | ||
| } | ||
| await this.checkTool("npm", "Install Node.js: https://nodejs.org/"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
|
|
||
| yield { message: "Synthesizing CloudFormation templates" }; | ||
| await this.runner(["npm", "run", "cdk", "--", "synth", "--quiet"], { | ||
| cwd: cdkDir, | ||
| onOutput: (chunk) => this.logger.debug(chunk), | ||
| }); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import type { Project, ProjectEvent } from "../../../handlers/project/types"; | ||
|
|
||
| /** Builds the deployable artifacts owned by a project's selected backend. */ | ||
| export interface ProjectBackend { | ||
| build(project: Project): AsyncGenerator<ProjectEvent, void>; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,3 @@ | ||
| export { FsProjectManager } from "./manager"; | ||
| export { CdkBackend, type CdkBackendConfig } from "./backends/cdk"; | ||
| export type { ProjectBackend } from "./backends/types"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ import { | |
| } from "../../io"; | ||
| import { defaultSource, type AssetSource } from "./source"; | ||
| import { createHarnessTreeFromSpec, createProjectTreeFromTemplate, TEMPLATES } from "./templates"; | ||
| import { ProjectSpecSchema } from "../../projectSchemas/project"; | ||
| import { ProjectSpecSchema, type ManagedBy } from "../../projectSchemas/project"; | ||
| import { enclosingProjectRoot } from "./fsUtils"; | ||
| import { | ||
| AgentCoreCLIError, | ||
|
|
@@ -30,13 +30,16 @@ import { | |
| } from "../../errors/errors"; | ||
| import type { HarnessSpecSchema } from "../../projectSchemas/harness"; | ||
| import z from "zod"; | ||
| import { CdkBackend } from "./backends/cdk"; | ||
| import type { ProjectBackend } from "./backends/types"; | ||
|
|
||
| type ProjectManagerConfig = { | ||
| logger: Logger; | ||
| source?: AssetSource; // Bun executable or dist/assets depending on runtime | ||
| runner?: ProcessRunner; // injectable so tests never spawn real processes | ||
| checkTool?: typeof requireTool; // injectable so tests don't depend on the host's PATH | ||
| json?: ReadWriteJson; // injectable so tests read fixtures instead of disk | ||
| backends?: Partial<Record<ManagedBy, ProjectBackend>>; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -48,13 +51,21 @@ export class FsProjectManager implements ProjectManager { | |
| private readonly runner: ProcessRunner; | ||
| private readonly checkTool: typeof requireTool; | ||
| private readonly json: ReadWriteJson; | ||
| private readonly backends: Partial<Record<ManagedBy, ProjectBackend>>; | ||
|
|
||
| constructor(config: ProjectManagerConfig) { | ||
| this.logger = config.logger; | ||
| this.source = config.source ?? defaultSource(); | ||
| this.runner = config.runner ?? runProcess; | ||
| this.checkTool = config.checkTool ?? requireTool; | ||
| this.json = config.json ?? new FsReadWriteJson({ logger: config.logger }); | ||
| this.backends = config.backends ?? { | ||
| CDK: new CdkBackend({ | ||
| logger: config.logger, | ||
| runner: config.runner, | ||
| checkTool: config.checkTool, | ||
| }), | ||
| }; | ||
| } | ||
|
|
||
| public async resolve(input: ResolveProjectInput): Promise<Project | undefined> { | ||
|
|
@@ -229,47 +240,17 @@ export class FsProjectManager implements ProjectManager { | |
| } | ||
|
|
||
| public async *build(project: Project): AsyncGenerator<ProjectEvent, void> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice, I really like how clean this is now! |
||
| // agentcore.json records which backend owns the project's artifacts. CDK is the | ||
| // only one today; a terraform or no-IaC backend adds an arm here rather than | ||
| // editing the CDK path. | ||
| switch (project.spec.managedBy) { | ||
| case "CDK": | ||
| yield* this.buildWithCdk(project); | ||
| break; | ||
| default: { | ||
| // Exhaustiveness: a new ManagedBy member fails to compile until it is handled. | ||
| const unsupported: never = project.spec.managedBy; | ||
| throw new ProjectStateError( | ||
| `project '${project.name}' declares an unsupported backend: ${String(unsupported)}`, | ||
| ); | ||
| } | ||
| } | ||
| yield* this.backendFor(project).build(project); | ||
| } | ||
|
|
||
| // Compiles the generated CDK app and synthesizes its CloudFormation templates. | ||
| private async *buildWithCdk(project: Project): AsyncGenerator<ProjectEvent, void> { | ||
| const cdkDir = join(project.rootPath, "agentcore", "cdk"); | ||
|
|
||
| // The generated CDK app is built from its own node_modules; without them the | ||
| // failure would otherwise surface as an opaque "cdk: not found". | ||
| if (!existsSync(join(cdkDir, "node_modules"))) { | ||
| private backendFor(project: Project): ProjectBackend { | ||
| const backend = this.backends[project.spec.managedBy]; | ||
| if (!backend) { | ||
| throw new ProjectStateError( | ||
| `CDK dependencies are missing for project '${project.name}'. ` + | ||
| `Run 'cd ${cdkDir} && npm install'.`, | ||
| `project '${project.name}' declares an unsupported backend: ${project.spec.managedBy}`, | ||
| ); | ||
| } | ||
| await this.checkTool("npm", "Install Node.js: https://nodejs.org/"); | ||
|
|
||
| // The generated package.json defines `cdk` as "npm run build && cdk", so this | ||
| // single command compiles the app and then synthesizes it. Synthesis needs no | ||
| // AWS credentials: each stack's environment comes from aws-targets.json. | ||
| // | ||
| // Build deliberately does not require a deployment target. A freshly created | ||
| // project has none, and building is how the user first typechecks their agent, so | ||
| // the generated app synthesizes one environment-agnostic stack when the list is | ||
| // empty. Only deploying somewhere needs a real target. | ||
| yield { message: "Synthesizing CloudFormation templates" }; | ||
| await this.run(["npm", "run", "cdk", "--", "synth", "--quiet"], cdkDir); | ||
| return backend; | ||
| } | ||
|
|
||
| // Runs a command with its output streamed to the file logger. | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would also be deploying eventually too right? (no need to change anything, just want to make sure I understand the vision)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!