From c0a59a59ca9230a98713553a1c0335d6957be154 Mon Sep 17 00:00:00 2001 From: Sairam Sakhamuri Date: Wed, 28 Jun 2023 15:52:50 -0700 Subject: [PATCH] Integrate discovery with composer (#6042) * Added runtime command discovery * Resolved comments * Added error case to analyse codebase method * Updated install command * Reorganized tests and removed unwated promise.resolve stmt * Added review changes on install command and node version string array * Changes to node.ts to include additional condions on run script * Added code comments to types * Integrate discovery with composer * Minor modification * Minor changes * Minor changes * Resolved code commits * Added undefied to return if no cmd * Added undefied to return if no cmd * Added frameworkhook interface * code comments * format error msg * format error msg * format error msg * format error msg * Code comments * Fixed imports and compose command * Fix bugs. * Update base image for node runtime. * bug fix * bug fix * Remove hooks --------- Co-authored-by: Daniel Young Lee --- .../internaltesting-frameworks-compose.ts | 6 +- src/frameworks/compose/discover/filesystem.ts | 2 +- src/frameworks/compose/discover/index.ts | 59 ++++++++++++------- .../compose/discover/runtime/node.ts | 7 +-- src/frameworks/compose/discover/types.ts | 17 ++++++ src/frameworks/compose/driver/docker.ts | 33 ++++++----- src/frameworks/compose/driver/index.ts | 5 +- src/frameworks/compose/driver/local.ts | 17 ++++-- src/frameworks/compose/index.ts | 21 ++++--- src/frameworks/compose/interfaces.ts | 16 +---- .../compose/discover/runtime/node.spec.ts | 4 +- 11 files changed, 113 insertions(+), 74 deletions(-) diff --git a/src/commands/internaltesting-frameworks-compose.ts b/src/commands/internaltesting-frameworks-compose.ts index ee85ec27d8a..a2731ef2a7e 100644 --- a/src/commands/internaltesting-frameworks-compose.ts +++ b/src/commands/internaltesting-frameworks-compose.ts @@ -4,18 +4,20 @@ import { logger } from "../logger"; import { Mode, SUPPORTED_MODES } from "../frameworks/compose/driver"; import { compose } from "../frameworks/compose"; import { FirebaseError } from "../error"; +import { LocalFileSystem } from "../frameworks/compose/discover/filesystem"; +import { frameworkSpecs } from "../frameworks/compose/discover/frameworkSpec"; export const command = new Command("internaltesting:frameworks:compose") .option("-m, --mode ", "Composer mode (local or docker)", "local") .description("compose framework in current directory") - .action((options: Options) => { + .action(async (options: Options) => { const mode = options.mode as string; if (!(SUPPORTED_MODES as unknown as string[]).includes(mode)) { throw new FirebaseError( `Unsupported mode ${mode}. Supported modes are [${SUPPORTED_MODES.join(", ")}]` ); } - const bundle = compose(mode as Mode); + const bundle = await compose(mode as Mode, new LocalFileSystem("."), frameworkSpecs); logger.info(JSON.stringify(bundle, null, 2)); return {}; }); diff --git a/src/frameworks/compose/discover/filesystem.ts b/src/frameworks/compose/discover/filesystem.ts index b2c7b7f9b47..f1e7aa513d0 100644 --- a/src/frameworks/compose/discover/filesystem.ts +++ b/src/frameworks/compose/discover/filesystem.ts @@ -2,7 +2,7 @@ import { FileSystem } from "./types"; import { pathExists, readFile } from "fs-extra"; import * as path from "path"; import { FirebaseError } from "../../../error"; -import { logger } from "../../../../src/logger"; +import { logger } from "../../../logger"; /** * Find files or read file contents present in the directory. diff --git a/src/frameworks/compose/discover/index.ts b/src/frameworks/compose/discover/index.ts index 8d3c8542ab3..43a7ba72e80 100644 --- a/src/frameworks/compose/discover/index.ts +++ b/src/frameworks/compose/discover/index.ts @@ -1,28 +1,43 @@ -import { AppSpec } from "../interfaces"; +import { Runtime, FileSystem, FrameworkSpec, RuntimeSpec } from "./types"; +import { NodejsRuntime } from "./runtime/node"; +import { FirebaseError } from "../../../error"; + +const supportedRuntimes: Runtime[] = [new NodejsRuntime()]; /** - * Discover framework in the given project directory + * Discover the best matching runtime specs for the application. */ -export function discover(): AppSpec { - return { - baseImage: "us-docker.pkg.dev/firestack-build/test/run:latest", - environmentVariables: { - NODE_ENV: "PRODUCTION", - }, - installCommand: "npm install", - buildCommand: "npm run build", - startCommand: "npm run start", +export async function discover( + fs: FileSystem, + allFrameworkSpecs: FrameworkSpec[] +): Promise { + try { + let discoveredRuntime = undefined; + for (const runtime of supportedRuntimes) { + if (await runtime.match(fs)) { + if (!discoveredRuntime) { + discoveredRuntime = runtime; + } else { + throw new FirebaseError( + `Conflit occurred as multiple runtimes ${discoveredRuntime.getRuntimeName()}, ${runtime.getRuntimeName()} are discovered in the application.` + ); + } + } + } - // eslint-disable-next-line @typescript-eslint/no-unused-vars - afterInstall: (b) => { - console.log("HOOK: AFTER INSTALL"); - return { ...b, version: "v1alpha", notes: "afterInstall" }; - }, + if (!discoveredRuntime) { + throw new FirebaseError( + `Unable to determine the specific runtime for the application. The supported runtime options include ${supportedRuntimes + .map((x) => x.getRuntimeName()) + .join(" , ")}.` + ); + } + const runtimeSpec = await discoveredRuntime.analyseCodebase(fs, allFrameworkSpecs); - // eslint-disable-next-line @typescript-eslint/no-unused-vars - afterBuild(b) { - console.log("HOOK: AFTER BUILD"); - return { ...b, version: "v1alpha", notes: "afterBuild" }; - }, - }; + return runtimeSpec; + } catch (error: any) { + throw new FirebaseError( + `Failed to identify required specifications to execute the application: ${error}` + ); + } } diff --git a/src/frameworks/compose/discover/runtime/node.ts b/src/frameworks/compose/discover/runtime/node.ts index 976657deac9..58a6e94fdc5 100644 --- a/src/frameworks/compose/discover/runtime/node.ts +++ b/src/frameworks/compose/discover/runtime/node.ts @@ -5,7 +5,7 @@ import { frameworkMatcher } from "../frameworkMatcher"; import { LifecycleCommands } from "../types"; import { Command } from "../types"; import { FirebaseError } from "../../../../error"; -import { logger } from "../../../../../src/logger"; +import { logger } from "../../../../logger"; import { conjoinOptions } from "../../../utils"; export interface PackageJSON { @@ -23,7 +23,6 @@ const YARN_LOCK = "yarn.lock"; export class NodejsRuntime implements Runtime { private readonly runtimeRequiredFiles: string[] = [PACKAGE_JSON]; - private readonly contentCache: Record = {}; // Checks if the codebase is using Node as runtime. async match(fs: FileSystem): Promise { @@ -41,7 +40,7 @@ export class NodejsRuntime implements Runtime { getNodeImage(engine: Record | undefined): string { // If no version is mentioned explicitly, assuming application is compatible with latest version. if (!engine || !engine.node) { - return `node:${supportedNodeVersions[supportedNodeVersions.length - 1]}-slim`; + return "us-docker.pkg.dev/firestack-build/test/run"; } const versionNumber = engine.node; @@ -54,7 +53,7 @@ export class NodejsRuntime implements Runtime { ); } - return `node:${versionNumber}-slim`; + return "us-docker.pkg.dev/firestack-build/test/run"; } async getPackageManager(fs: FileSystem): Promise { diff --git a/src/frameworks/compose/discover/types.ts b/src/frameworks/compose/discover/types.ts index b919e552c4b..a89fb00c312 100644 --- a/src/frameworks/compose/discover/types.ts +++ b/src/frameworks/compose/discover/types.ts @@ -1,3 +1,5 @@ +import { AppBundle } from "../interfaces"; + export interface FileSystem { exists(file: string): Promise; read(file: string): Promise; @@ -77,4 +79,19 @@ export interface RuntimeSpec { // The runtime has detected a command that should always be run irrespective of // the framework (e.g. the "build" script always wins in Node) detectedCommands?: LifecycleCommands; + + environmentVariables?: Record; + + // Framework authors can execute framework-specific code using hooks at different stages of Frameworks API build process. + frameworkHooks?: FrameworkHooks; +} + +export interface FrameworkHooks { + // Programmatic hook with access to filesystem and nodejs API to inspect the workspace. + // Primarily intended to gather hints relevant to the build. + afterInstall?: (b: AppBundle) => AppBundle; + + // Programmatic hook with access to filesystem and nodejs API to inspect the build artifacts. + // Primarily intended to informs what assets should be deployed. + afterBuild?: (b: AppBundle) => AppBundle; } diff --git a/src/frameworks/compose/driver/docker.ts b/src/frameworks/compose/driver/docker.ts index 70d9d4344b9..8033d7c16cd 100644 --- a/src/frameworks/compose/driver/docker.ts +++ b/src/frameworks/compose/driver/docker.ts @@ -2,8 +2,9 @@ import * as fs from "node:fs"; import * as path from "node:path"; import * as spawn from "cross-spawn"; -import { AppBundle, AppSpec, Driver, Hook } from "../interfaces"; +import { AppBundle, Driver, Hook } from "../interfaces"; import { BUNDLE_PATH, genHookScript } from "./hooks"; +import { RuntimeSpec } from "../discover/types"; const ADAPTER_SCRIPTS_PATH = "./.firebase/adapters" as const; @@ -98,7 +99,7 @@ export class DockerfileBuilder { export class DockerDriver implements Driver { private dockerfileBuilder; - constructor(readonly spec: AppSpec) { + constructor(readonly spec: RuntimeSpec) { this.dockerfileBuilder = new DockerfileBuilder(); this.dockerfileBuilder.from(spec.baseImage, "base").user("firebase"); } @@ -148,21 +149,25 @@ export class DockerDriver implements Driver { } install(): void { - this.dockerfileBuilder - .fromLastStage(DOCKER_STAGE_INSTALL) - .workdir("/home/firebase/app") - .envs(this.spec.environmentVariables || {}) - .copyForFirebase("package.json", ".") - .run(this.spec.installCommand); - this.buildStage(DOCKER_STAGE_INSTALL, "."); + if (this.spec.installCommand) { + this.dockerfileBuilder + .fromLastStage(DOCKER_STAGE_INSTALL) + .workdir("/home/firebase/app") + .envs(this.spec.environmentVariables || {}) + .copyForFirebase("package.json", ".") + .run(this.spec.installCommand); + this.buildStage(DOCKER_STAGE_INSTALL, "."); + } } build(): void { - this.dockerfileBuilder - .fromLastStage(DOCKER_STAGE_BUILD) - .copyForFirebase(".", ".") - .run(this.spec.buildCommand); - this.buildStage(DOCKER_STAGE_BUILD, "."); + if (this.spec.detectedCommands?.build) { + this.dockerfileBuilder + .fromLastStage(DOCKER_STAGE_BUILD) + .copyForFirebase(".", ".") + .run(this.spec.detectedCommands.build.cmd); + this.buildStage(DOCKER_STAGE_BUILD, "."); + } } export(bundle: AppBundle): void { diff --git a/src/frameworks/compose/driver/index.ts b/src/frameworks/compose/driver/index.ts index 8adc6480708..1779fdb5c3e 100644 --- a/src/frameworks/compose/driver/index.ts +++ b/src/frameworks/compose/driver/index.ts @@ -1,6 +1,7 @@ -import { AppSpec, Driver } from "../interfaces"; +import { Driver } from "../interfaces"; import { LocalDriver } from "./local"; import { DockerDriver } from "./docker"; +import { RuntimeSpec } from "../discover/types"; export const SUPPORTED_MODES = ["local", "docker"] as const; export type Mode = (typeof SUPPORTED_MODES)[number]; @@ -8,7 +9,7 @@ export type Mode = (typeof SUPPORTED_MODES)[number]; /** * Returns the driver that provides the execution context for the composer. */ -export function getDriver(mode: Mode, app: AppSpec): Driver { +export function getDriver(mode: Mode, app: RuntimeSpec): Driver { if (mode === "local") { return new LocalDriver(app); } else if (mode === "docker") { diff --git a/src/frameworks/compose/driver/local.ts b/src/frameworks/compose/driver/local.ts index da23187bf58..5bd219c113c 100644 --- a/src/frameworks/compose/driver/local.ts +++ b/src/frameworks/compose/driver/local.ts @@ -1,11 +1,12 @@ import * as fs from "node:fs"; import * as spawn from "cross-spawn"; -import { AppBundle, AppSpec, Hook, Driver } from "../interfaces"; +import { AppBundle, Hook, Driver } from "../interfaces"; import { BUNDLE_PATH, genHookScript } from "./hooks"; +import { RuntimeSpec } from "../discover/types"; export class LocalDriver implements Driver { - constructor(readonly spec: AppSpec) {} + constructor(readonly spec: RuntimeSpec) {} private execCmd(cmd: string, args: string[]) { const ret = spawn.sync(cmd, args, { @@ -18,13 +19,17 @@ export class LocalDriver implements Driver { } install(): void { - const [cmd, ...args] = this.spec.installCommand.split(" "); - this.execCmd(cmd, args); + if (this.spec.installCommand) { + const [cmd, ...args] = this.spec.installCommand.split(" "); + this.execCmd(cmd, args); + } } build(): void { - const [cmd, ...args] = this.spec.buildCommand.split(" "); - this.execCmd(cmd, args); + if (this.spec.detectedCommands?.build) { + const [cmd, ...args] = this.spec.detectedCommands.build.cmd.split(" "); + this.execCmd(cmd, args); + } } // eslint-disable-next-line @typescript-eslint/no-unused-vars diff --git a/src/frameworks/compose/index.ts b/src/frameworks/compose/index.ts index 24e45490be1..4a5d066ca8c 100644 --- a/src/frameworks/compose/index.ts +++ b/src/frameworks/compose/index.ts @@ -1,31 +1,36 @@ import { AppBundle } from "./interfaces"; import { getDriver, Mode } from "./driver"; import { discover } from "./discover"; +import { FrameworkSpec, FileSystem } from "./discover/types"; /** * Run composer in the specified execution context. */ -export function compose(mode: Mode): AppBundle { +export async function compose( + mode: Mode, + fs: FileSystem, + allFrameworkSpecs: FrameworkSpec[] +): Promise { let bundle: AppBundle = { version: "v1alpha" }; - const spec = discover(); + const spec = await discover(fs, allFrameworkSpecs); const driver = getDriver(mode, spec); - if (spec.startCommand) { + if (spec.detectedCommands?.run) { bundle.server = { start: { - cmd: spec.startCommand.split(" "), + cmd: spec.detectedCommands.run.cmd.split(" "), }, }; } driver.install(); - if (spec.afterInstall) { - bundle = driver.execHook(bundle, spec.afterInstall); + if (spec.frameworkHooks?.afterInstall) { + bundle = driver.execHook(bundle, spec.frameworkHooks.afterInstall); } driver.build(); - if (spec.afterBuild) { - bundle = driver.execHook(bundle, spec.afterBuild); + if (spec.frameworkHooks?.afterBuild) { + bundle = driver.execHook(bundle, spec.frameworkHooks?.afterBuild); } if (bundle.server) { diff --git a/src/frameworks/compose/interfaces.ts b/src/frameworks/compose/interfaces.ts index db3caee4898..068b28a178b 100644 --- a/src/frameworks/compose/interfaces.ts +++ b/src/frameworks/compose/interfaces.ts @@ -1,3 +1,5 @@ +import { RuntimeSpec } from "./discover/types"; + export interface AppBundle { version: "v1alpha"; server?: ServerConfig; @@ -22,22 +24,10 @@ interface StartConfig { runtime?: "nodejs18" | string; } -export interface AppSpec { - baseImage: string; - packageManagerInstallCommand?: string; - environmentVariables?: Record; - installCommand: string; - buildCommand: string; - startCommand: string; - - afterInstall?: (b: AppBundle) => AppBundle; - afterBuild?: (b: AppBundle) => AppBundle; -} - export type Hook = (b: AppBundle) => AppBundle; export class Driver { - constructor(readonly spec: AppSpec) {} + constructor(readonly spec: RuntimeSpec) {} install(): void { throw new Error("install() not implemented"); diff --git a/src/test/frameworks/compose/discover/runtime/node.spec.ts b/src/test/frameworks/compose/discover/runtime/node.spec.ts index b2bbaee767c..4dedc70db6a 100644 --- a/src/test/frameworks/compose/discover/runtime/node.spec.ts +++ b/src/test/frameworks/compose/discover/runtime/node.spec.ts @@ -41,7 +41,7 @@ describe("NodejsRuntime", () => { node: "18", }; const actualImage = nodeJSRuntime.getNodeImage(version); - const expectedImage = "node:18-slim"; + const expectedImage = "us-docker.pkg.dev/firestack-build/test/run"; expect(actualImage).to.deep.equal(expectedImage); }); @@ -189,7 +189,7 @@ describe("NodejsRuntime", () => { const actual = await nodeJSRuntime.analyseCodebase(fileSystem, allFrameworks); const expected = { id: "nodejs", - baseImage: "node:18-slim", + baseImage: "us-docker.pkg.dev/firestack-build/test/run", packageManagerInstallCommand: undefined, installCommand: "npm install", detectedCommands: {