From 1a55cf7c2e5ccb46963dc8d0f7938b346bc13833 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 11 Oct 2023 17:20:53 +0200 Subject: [PATCH] fix: allow generated files in build actions (#5230) In the container and jib-container build actions, we checked for the existance of files checked in to version control for certain checks. Garden allows for files to be injected into the build action dynamically at run time, e.g. by using `copyFrom` and `generateFiles`. We want to check for actual presence of files in the action's build directory instead of checking for the files that are included from version control. Fixes #5201 --- core/src/plugins/container/helpers.ts | 20 +++++-- core/src/vcs/vcs.ts | 6 ++ core/test/unit/src/plugins/container/build.ts | 3 + plugins/jib/src/index.ts | 6 +- plugins/jib/src/util.ts | 27 ++++----- plugins/jib/test/util.ts | 57 +++++++++---------- 6 files changed, 62 insertions(+), 57 deletions(-) diff --git a/core/src/plugins/container/helpers.ts b/core/src/plugins/container/helpers.ts index ac73d46c3e..eb61f2336e 100644 --- a/core/src/plugins/container/helpers.ts +++ b/core/src/plugins/container/helpers.ts @@ -373,15 +373,25 @@ const helpers = { moduleHasDockerfile(config: ContainerModuleConfig, version: ModuleVersion): boolean { // If we explicitly set a Dockerfile, we take that to mean you want it to be built. // If the file turns out to be missing, this will come up in the build handler. - return !!config.spec.dockerfile || version.files.includes(getDockerfilePath(config.path, config.spec.dockerfile)) + + if (!!config.spec.dockerfile) { + return true + } + + // NOTE: The fact that we overloaded the `image` field of a container module means the Dockerfile must be checked into version control + // This means it's not possible to use `copyFrom` or `generateFiles` to get it from dependencies or generate it at runtime. + // That's because the `image` field has the following two meanings: + // 1. Build an image with this name, if a Dockerfile exists + // 2. Deploy this image from the registry, if no Dockerfile exists + // This means we need to know if the Dockerfile exists before we know wether or not the Dockerfile will be present at runtime. + return version.files.includes(getDockerfilePath(config.path, config.spec.dockerfile)) }, async actionHasDockerfile(action: Resolved): Promise { - // If we explicitly set a Dockerfile, we take that to mean you want it to be built. - // If the file turns out to be missing, this will come up in the build handler. const dockerfile = action.getSpec("dockerfile") - const dockerfileSourcePath = getDockerfilePath(action.basePath(), dockerfile) - return action.getFullVersion().files.includes(dockerfileSourcePath) + // NOTE: it's important to check for the files existence in the build path to allow dynamically copying the Dockerfile from other actions using `copyFrom`. + const dockerfileSourcePath = getDockerfilePath(action.getBuildPath(), dockerfile) + return await pathExists(dockerfileSourcePath) }, /** diff --git a/core/src/vcs/vcs.ts b/core/src/vcs/vcs.ts index 1b37b367fa..e1b8a53e95 100644 --- a/core/src/vcs/vcs.ts +++ b/core/src/vcs/vcs.ts @@ -57,6 +57,12 @@ export async function validateGitInstall() { export interface TreeVersion { contentHash: string + /** + * Important! Do not use the files to determine if a file will exist when performing an action. + * Other mechanisms, e.g. the build command itself and `copyFrom` might affect available files at runtime. + * + * See also https://github.com/garden-io/garden/issues/5201 + */ files: string[] } diff --git a/core/test/unit/src/plugins/container/build.ts b/core/test/unit/src/plugins/container/build.ts index 9f7c44d319..96cef25baf 100644 --- a/core/test/unit/src/plugins/container/build.ts +++ b/core/test/unit/src/plugins/container/build.ts @@ -18,6 +18,7 @@ import { ContainerProvider, gardenPlugin } from "../../../../../src/plugins/cont import { containerHelpers } from "../../../../../src/plugins/container/helpers" import { joinWithPosix } from "../../../../../src/util/fs" import { getDataDir, TestGarden, makeTestGarden } from "../../../../helpers" +import { createFile } from "fs-extra" context("build.ts", () => { const projectRoot = getDataDir("test-project-container") @@ -85,6 +86,7 @@ context("build.ts", () => { sinon.replace(action, "getOutputs", () => ({ localImageId: "some/image" })) const buildPath = action.getBuildPath() + await createFile(join(buildPath, "Dockerfile")) const cmdArgs = getCmdArgs(action, buildPath) sinon.replace(containerHelpers, "dockerCli", async ({ cwd, args, ctx: _ctx }) => { @@ -109,6 +111,7 @@ context("build.ts", () => { sinon.replace(action, "getOutputs", () => ({ localImageId: "some/image" })) const buildPath = action.getBuildPath() + await createFile(join(buildPath, "docker-dir/Dockerfile")) const cmdArgs = getCmdArgs(action, buildPath) sinon.replace(containerHelpers, "dockerCli", async ({ cwd, args, ctx: _ctx }) => { diff --git a/plugins/jib/src/index.ts b/plugins/jib/src/index.ts index b7f9e4e7c2..9486ff0e4c 100644 --- a/plugins/jib/src/index.ts +++ b/plugins/jib/src/index.ts @@ -206,11 +206,7 @@ export const gardenPlugin = () => let projectType = spec.projectType if (!projectType) { - projectType = detectProjectType({ - actionName: action.name, - actionBasePath: action.basePath(), - actionFiles: action.getFullVersion().files, - }) + projectType = await detectProjectType(action) statusLine.info(`Detected project type ${projectType}`) } diff --git a/plugins/jib/src/util.ts b/plugins/jib/src/util.ts index 53b9fc5a5e..21764f0838 100644 --- a/plugins/jib/src/util.ts +++ b/plugins/jib/src/util.ts @@ -15,9 +15,10 @@ import { ContainerModuleBuildSpec, ContainerModuleSpec, } from "@garden-io/core/build/src/plugins/container/moduleConfig" -import { BuildAction, BuildActionConfig } from "@garden-io/core/build/src/actions/build" +import { BuildAction, BuildActionConfig, ResolvedBuildAction } from "@garden-io/core/build/src/actions/build" import { ContainerBuildOutputs } from "@garden-io/core/build/src/plugins/container/config" import { Resolved } from "@garden-io/core/build/src/actions/types" +import { pathExists } from "fs-extra" interface JibBuildSpec { dockerBuild?: boolean @@ -58,40 +59,32 @@ const gradlePaths = [ const mavenPaths = ["pom.xml", ".mvn"] const mavendPaths = ["pom.xml", ".mvnd"] -export function detectProjectType({ - actionName, - actionBasePath, - actionFiles, -}: { - actionName: string - actionBasePath: string - actionFiles: string[] -}): JibPluginType { +export async function detectProjectType(action: ResolvedBuildAction): Promise { // TODO: support the Jib CLI for (const filename of gradlePaths) { - const path = resolve(actionBasePath, filename) - if (actionFiles.includes(path)) { + const path = resolve(action.getBuildPath(), filename) + if (await pathExists(path)) { return "gradle" } } for (const filename of mavenPaths) { - const path = resolve(actionBasePath, filename) - if (actionFiles.includes(path)) { + const path = resolve(action.getBuildPath(), filename) + if (await pathExists(path)) { return "maven" } } for (const filename of mavendPaths) { - const path = resolve(actionBasePath, filename) - if (actionFiles.includes(path)) { + const path = resolve(action.getBuildPath(), filename) + if (await pathExists(path)) { return "mavend" } } throw new ConfigurationError({ - message: `Could not detect a gradle or maven project to build ${actionName}`, + message: `Could not detect a gradle or maven project to build ${action.longDescription()}`, }) } diff --git a/plugins/jib/test/util.ts b/plugins/jib/test/util.ts index 21e62bd150..f73f770443 100644 --- a/plugins/jib/test/util.ts +++ b/plugins/jib/test/util.ts @@ -9,10 +9,12 @@ import { expectError, makeTestGarden, TestGarden } from "@garden-io/sdk/build/src/testing" import { expect } from "chai" import { detectProjectType, getBuildFlags, JibBuildAction } from "../src/util" -import { resolve } from "path" +import { join, resolve } from "path" import { ResolvedConfigGraph } from "@garden-io/core/build/src/graph/config-graph" import { Resolved } from "@garden-io/core/build/src/actions/types" import { gardenPlugin } from "../src/index" +import { rm } from "fs/promises" +import { createFile } from "fs-extra" describe("util", function () { // eslint-disable-next-line no-invalid-this @@ -23,6 +25,7 @@ describe("util", function () { let garden: TestGarden let graph: ResolvedConfigGraph let action: Resolved + let buildPath: string before(async () => { garden = await makeTestGarden(projectRoot, { @@ -33,41 +36,35 @@ describe("util", function () { beforeEach(async () => { graph = await garden.getResolvedConfigGraph({ log: garden.log, emit: false }) action = graph.getBuild("foo") + buildPath = action.getBuildPath() }) - describe("detectProjectType", () => { - it("returns gradle if action files include a gradle config", () => { - expect( - detectProjectType({ - actionName: "foo", - actionBasePath: "/foo", - actionFiles: ["/foo/build.gradle"], - }) - ).to.equal("gradle") + describe("detectProjectType", async () => { + afterEach(async () => { + try { + await rm(join(`${buildPath}/build.gradle`), { recursive: true }) + } catch (e) {} + try { + await rm(join(`${buildPath}/pom.xml`), { recursive: true }) + } catch (e) {} }) - it("returns maven if action files include a maven config", () => { - expect( - detectProjectType({ - actionName: "foo", - actionBasePath: "/foo", - actionFiles: ["/foo/pom.xml"], - }) - ).to.equal("maven") + it("returns gradle if action files include a gradle config", async () => { + await createFile(resolve(buildPath, "build.gradle")) + + expect(await detectProjectType(action)).to.equal("gradle") }) - it("throws if no maven or gradle config is in the action file list", () => { - void expectError( - () => - detectProjectType({ - actionName: "foo", - actionBasePath: "/foo", - actionFiles: [], - }), - { - contains: "Could not detect a gradle or maven project to build foo", - } - ) + it("returns maven if action files include a maven config", async () => { + await createFile(resolve(buildPath, "pom.xml")) + + expect(await detectProjectType(action)).to.equal("maven") + }) + + it("throws if no maven or gradle config is in the action file list", async () => { + await expectError(() => detectProjectType(action), { + contains: `Could not detect a gradle or maven project to build ${action.longDescription()}`, + }) }) })