From fb33f0a2f3c45fe3e17925b200a5955f6101af0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BE=C3=B3r=20Magn=C3=BAsson?= Date: Tue, 16 Oct 2018 12:04:06 +0200 Subject: [PATCH] feat: allow custom dockerfile path for container modules --- docs/reference/config.md | 5 + garden-service/src/plugins/container.ts | 61 ++- .../src/plugins/kubernetes/deployment.ts | 4 +- .../module-a/docker-dir/Dockerfile | 1 + garden-service/test/src/plugins/container.ts | 377 +++++++----------- 5 files changed, 197 insertions(+), 251 deletions(-) create mode 100644 garden-service/test/data/test-project-container/module-a/docker-dir/Dockerfile diff --git a/docs/reference/config.md b/docs/reference/config.md index e91211c7586..72837052cc9 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -444,6 +444,11 @@ module: # Optional. image: + # POSIX-style name of Dockerfile, relative to project root. Defaults to $MODULE_ROOT/Dockerfile. + # + # Optional. + dockerfile: + # List of services to deploy from this container module. # # Optional. diff --git a/garden-service/src/plugins/container.ts b/garden-service/src/plugins/container.ts index c83378de8bd..a610b6d7f2e 100644 --- a/garden-service/src/plugins/container.ts +++ b/garden-service/src/plugins/container.ts @@ -7,9 +7,10 @@ */ import * as Joi from "joi" -import * as childProcess from "child-process-promise" import dedent = require("dedent") -import deline = require("deline") +import deline = require("dedent") +import execa = require("execa") + import { Module } from "../types/module" import { absolutePathRegex, @@ -254,6 +255,7 @@ export const containerTestSchema = genericTestSchema export interface ContainerModuleSpec extends ModuleSpec { buildArgs: PrimitiveMap, image?: string, + dockerfile?: string, services: ContainerServiceSpec[], tests: ContainerTestSpec[], hotReload?: HotReloadConfigSpec, @@ -276,6 +278,8 @@ export const containerModuleSpecSchema = Joi.object() Specify the image name for the container. Should be a valid docker image identifier. If specified and the module does not contain a Dockerfile, this image will be used to deploy the container services. If specified and the module does contain a Dockerfile, this identifier is used when pushing the built image.`), + dockerfile: Joi.string().uri({ relativeOnly: true }) + .description("POSIX-style name of Dockerfile, relative to project root. Defaults to $MODULE_ROOT/Dockerfile."), services: joiArray(serviceSchema) .unique("name") .description("List of services to deploy from this container module."), @@ -298,6 +302,13 @@ interface ParsedImageId { tag: string } +function getDockerfilePath(basePath: string, dockerfile?: string) { + if (dockerfile) { + return join(basePath, dockerfile) + } + return join(basePath, "Dockerfile") +} + export const helpers = { /** * Returns the image ID used locally, when building and deploying to local environments @@ -403,24 +414,32 @@ export const helpers = { async pullImage(module: ContainerModule) { const identifier = await helpers.getPublicImageId(module) - await helpers.dockerCli(module, `pull ${identifier}`) + await helpers.dockerCli(module, ["pull", identifier]) }, async imageExistsLocally(module: ContainerModule) { const identifier = await helpers.getLocalImageId(module) - const exists = (await helpers.dockerCli(module, `images ${identifier} -q`)).stdout.trim().length > 0 + const exists = (await helpers.dockerCli(module, ["images", identifier, "-q"])).length > 0 return exists ? identifier : null }, - async dockerCli(module: ContainerModule, args) { + async dockerCli(module: ContainerModule, args: string[]) { // TODO: use dockerode instead of CLI - return childProcess.exec("docker " + args, { cwd: module.buildPath, maxBuffer: 1024 * 1024 }) + return execa.stdout("docker", args, { cwd: module.buildPath, maxBuffer: 1024 * 1024 }) }, async hasDockerfile(module: ContainerModule) { - const buildPath = module.buildPath - return pathExists(join(buildPath, "Dockerfile")) + return pathExists(helpers.getDockerfilePathFromModule(module)) + }, + + getDockerfilePathFromModule(module: ContainerModule) { + return getDockerfilePath(module.buildPath, module.spec.dockerfile) + }, + + getDockerfilePathFromConfig(config: ModuleConfig) { + return getDockerfilePath(config.path, config.spec.dockerfile) }, + } export async function validateContainerModule({ moduleConfig }: ValidateModuleParams) { @@ -481,8 +500,17 @@ export async function validateContainerModule({ moduleConfig }: ValidateModulePa timeout: t.timeout, })) + const hasDockerfile = await pathExists(helpers.getDockerfilePathFromConfig(moduleConfig)) + + if (moduleConfig.spec.dockerfile && !hasDockerfile) { + throw new ConfigurationError( + `Dockerfile not found at specififed path ${moduleConfig.spec.dockerfile} for module ${moduleConfig.name}`, + {}, + ) + } + // make sure we can build the thing - if (!moduleConfig.spec.image && !(await pathExists(join(moduleConfig.path, "Dockerfile")))) { + if (!moduleConfig.spec.image && !hasDockerfile) { throw new ConfigurationError( `Module ${moduleConfig.name} neither specifies image nor provides Dockerfile`, {}, @@ -556,14 +584,23 @@ export const gardenPlugin = (): GardenPlugin => ({ // build doesn't exist, so we create it logEntry && logEntry.setState(`Building ${identifier}...`) + const cmdOpts = ["build", "-t", identifier] const buildArgs = Object.entries(module.spec.buildArgs).map(([key, value]) => { // TODO: may need to escape this return `--build-arg ${key}=${value}` }).join(" ") + if (buildArgs) { + cmdOpts.push(buildArgs) + } + + if (module.spec.dockerfile) { + cmdOpts.push("--file", helpers.getDockerfilePathFromModule(module)) + } + // TODO: log error if it occurs // TODO: stream output to log if at debug log level - await helpers.dockerCli(module, `build ${buildArgs} -t ${identifier} ${buildPath}`) + await helpers.dockerCli(module, [...cmdOpts, buildPath]) return { fresh: true, details: { identifier } } }, @@ -580,13 +617,13 @@ export const gardenPlugin = (): GardenPlugin => ({ logEntry && logEntry.setState({ msg: `Publishing image ${remoteId}...` }) if (localId !== remoteId) { - await helpers.dockerCli(module, `tag ${localId} ${remoteId}`) + await helpers.dockerCli(module, ["tag", localId, remoteId]) } // TODO: log error if it occurs // TODO: stream output to log if at debug log level // TODO: check if module already exists remotely? - await helpers.dockerCli(module, `push ${remoteId}`) + await helpers.dockerCli(module, ["push", remoteId]) return { published: true, message: `Published ${remoteId}` } }, diff --git a/garden-service/src/plugins/kubernetes/deployment.ts b/garden-service/src/plugins/kubernetes/deployment.ts index 62ad1c13a9b..9a402a5de68 100644 --- a/garden-service/src/plugins/kubernetes/deployment.ts +++ b/garden-service/src/plugins/kubernetes/deployment.ts @@ -528,8 +528,8 @@ export async function pushModule({ ctx, module, logEntry }: PushModuleParams { const projectRoot = resolve(dataDir, "test-project-container") const modulePath = resolve(dataDir, "test-project-container", "module-a") + const relDockerfilePath = "docker-dir/Dockerfile" const handler = gardenPlugin() const validate = handler.moduleActions!.container!.validate! @@ -25,6 +30,27 @@ describe("plugins.container", () => { const publishModule = handler.moduleActions!.container!.publishModule! const getBuildStatus = handler.moduleActions!.container!.getBuildStatus! + const baseConfig: ModuleConfig = { + allowPublish: false, + build: { + command: [], + dependencies: [], + }, + name: "test", + path: modulePath, + type: "container", + variables: {}, + + spec: { + buildArgs: {}, + services: [], + tests: [], + }, + + serviceConfigs: [], + testConfigs: [], + } + let garden: Garden let ctx: PluginContext @@ -49,27 +75,9 @@ describe("plugins.container", () => { describe("helpers", () => { describe("getLocalImageId", () => { it("should create identifier with commit hash version if module has a Dockerfile", async () => { - const module = await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image:1.1", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - }) + const config = cloneDeep(baseConfig) + config.spec.image = "some/image:1.1" + const module = await getTestModule(config) td.replace(helpers, "hasDockerfile", async () => true) @@ -77,27 +85,9 @@ describe("plugins.container", () => { }) it("should create identifier with image name if module has no Dockerfile", async () => { - const module = await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image:1.1", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - }) + const config = cloneDeep(baseConfig) + config.spec.image = "some/image:1.1" + const module = await getTestModule(config) td.replace(helpers, "hasDockerfile", async () => false) @@ -107,27 +97,9 @@ describe("plugins.container", () => { describe("getPublicImageId", () => { it("should use image name including version if specified", async () => { - const module = await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image:1.1", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - }) + const config = cloneDeep(baseConfig) + config.spec.image = "some/image:1.1" + const module = await getTestModule(config) expect(await helpers.getPublicImageId(module)).to.equal("some/image:1.1") }) @@ -159,32 +131,50 @@ describe("plugins.container", () => { }) it("should use local id if no image name is set", async () => { - const module = await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - }) + const module = await getTestModule(baseConfig) td.replace(helpers, "getLocalImageId", async () => "test:1234") expect(await helpers.getPublicImageId(module)).to.equal("test:1234") }) }) + + describe("getDockerfilePathFromModule", () => { + it("should return the absolute default Dockerfile path", async () => { + const module = await getTestModule(baseConfig) + + td.replace(helpers, "hasDockerfile", async () => true) + + const path = await helpers.getDockerfilePathFromModule(module) + expect(path).to.equal(join(module.buildPath, "Dockerfile")) + }) + + it("should return the absolute user specified Dockerfile path", async () => { + const config = cloneDeep(baseConfig) + config.spec.dockerfile = relDockerfilePath + const module = await getTestModule(config) + + td.replace(helpers, "hasDockerfile", async () => true) + + const path = await helpers.getDockerfilePathFromModule(module) + expect(path).to.equal(join(module.buildPath, relDockerfilePath)) + }) + }) + + describe("getDockerfilePathFromConfig", () => { + it("should return the absolute default Dockerfile path", async () => { + const path = await helpers.getDockerfilePathFromConfig(baseConfig) + expect(path).to.equal(join(baseConfig.path, "Dockerfile")) + }) + + it("should return the absolute user specified Dockerfile path", async () => { + const config = cloneDeep(baseConfig) + config.spec.dockerfile = relDockerfilePath + + const path = await helpers.getDockerfilePathFromConfig(config) + expect(path).to.equal(join(config.path, relDockerfilePath)) + }) + }) }) describe("DockerModuleHandler", () => { @@ -326,6 +316,16 @@ describe("plugins.container", () => { }) }) + it("should fail if user specified Dockerfile not found", async () => { + const moduleConfig = cloneDeep(baseConfig) + moduleConfig.spec.dockerfile = "path/to/non-existing/Dockerfile" + + await expectError( + () => validate({ ctx, moduleConfig }), + "configuration", + ) + }) + it("should fail with invalid port in ingress spec", async () => { const moduleConfig: ContainerModuleConfig = { allowPublish: false, @@ -463,26 +463,7 @@ describe("plugins.container", () => { describe("getBuildStatus", () => { it("should return ready:true if build exists locally", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - })) + const module = td.object(await getTestModule(baseConfig)) td.replace(helpers, "imageExistsLocally", async () => true) @@ -491,26 +472,7 @@ describe("plugins.container", () => { }) it("should return ready:false if build does not exist locally", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - })) + const module = td.object(await getTestModule(baseConfig)) td.replace(helpers, "imageExistsLocally", async () => false) @@ -520,28 +482,10 @@ describe("plugins.container", () => { }) describe("build", () => { - it("pull image if image tag is set and the module doesn't container a Dockerfile", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - })) + it("should pull image if image tag is set and the module doesn't container a Dockerfile", async () => { + const config = cloneDeep(baseConfig) + config.spec.image = "some/image" + const module = td.object(await getTestModule(config)) td.replace(helpers, "hasDockerfile", async () => false) td.replace(helpers, "pullImage", async () => null) @@ -552,30 +496,35 @@ describe("plugins.container", () => { expect(result).to.eql({ fetched: true }) }) - it("build image if module contains Dockerfile", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, + it("should build image if module contains Dockerfile", async () => { + const config = cloneDeep(baseConfig) + config.spec.image = "some/image" + const module = td.object(await getTestModule(config)) - spec: { - buildArgs: {}, - image: "some/image", - services: [], - tests: [], - }, + td.replace(helpers, "hasDockerfile", async () => true) + td.replace(helpers, "imageExistsLocally", async () => false) + td.replace(helpers, "getLocalImageId", async () => "some/image") - serviceConfigs: [], - testConfigs: [], - })) + const dockerCli = td.replace(helpers, "dockerCli") + + const result = await build({ ctx, module, buildDependencies: {} }) + + expect(result).to.eql({ + fresh: true, + details: { identifier: "some/image" }, + }) + + td.verify(dockerCli(module, ["build", "-t", "some/image", module.buildPath])) + }) + + it("should build image using the user specified Dockerfile path", async () => { + const config = cloneDeep(baseConfig) + config.spec.dockerfile = relDockerfilePath td.replace(helpers, "hasDockerfile", async () => true) + + const module = td.object(await getTestModule(config)) + td.replace(helpers, "imageExistsLocally", async () => false) td.replace(helpers, "getLocalImageId", async () => "some/image") @@ -588,33 +537,23 @@ describe("plugins.container", () => { details: { identifier: "some/image" }, }) - td.verify(dockerCli(module, "build -t some/image " + module.buildPath)) + const cmdArgs = [ + "build", + "-t", + "some/image", + "--file", + join(module.buildPath, relDockerfilePath), + module.buildPath, + ] + td.verify(dockerCli(module, cmdArgs)) }) }) describe("publishModule", () => { - it("not publish image if module doesn't container a Dockerfile", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - })) + it("should not publish image if module doesn't container a Dockerfile", async () => { + const config = cloneDeep(baseConfig) + config.spec.image = "some/image" + const module = td.object(await getTestModule(config)) td.replace(helpers, "hasDockerfile", async () => false) @@ -622,28 +561,10 @@ describe("plugins.container", () => { expect(result).to.eql({ published: false }) }) - it("publish image if module contains a Dockerfile", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image:1.1", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - })) + it("should publish image if module contains a Dockerfile", async () => { + const config = cloneDeep(baseConfig) + config.spec.image = "some/image:1.1" + const module = td.object(await getTestModule(config)) td.replace(helpers, "hasDockerfile", async () => true) td.replace(helpers, "getLocalImageId", async () => "some/image:12345") @@ -654,32 +575,14 @@ describe("plugins.container", () => { const result = await publishModule({ ctx, module, buildDependencies: {} }) expect(result).to.eql({ message: "Published some/image:12345", published: true }) - td.verify(dockerCli(module, "tag some/image:12345 some/image:12345"), { times: 0 }) - td.verify(dockerCli(module, "push some/image:12345")) + td.verify(dockerCli(module, ["tag", "some/image:12345", "some/image:12345"]), { times: 0 }) + td.verify(dockerCli(module, ["push", "some/image:12345"])) }) - it("tag image if remote id differs from local id", async () => { - const module = td.object(await getTestModule({ - allowPublish: false, - build: { - command: [], - dependencies: [], - }, - name: "test", - path: modulePath, - type: "container", - variables: {}, - - spec: { - buildArgs: {}, - image: "some/image:1.1", - services: [], - tests: [], - }, - - serviceConfigs: [], - testConfigs: [], - })) + it("should tag image if remote id differs from local id", async () => { + const config = cloneDeep(baseConfig) + config.spec.image = "some/image:1.1" + const module = td.object(await getTestModule(config)) td.replace(helpers, "hasDockerfile", async () => true) td.replace(helpers, "getLocalImageId", () => "some/image:12345") @@ -690,8 +593,8 @@ describe("plugins.container", () => { const result = await publishModule({ ctx, module, buildDependencies: {} }) expect(result).to.eql({ message: "Published some/image:1.1", published: true }) - td.verify(dockerCli(module, "tag some/image:12345 some/image:1.1")) - td.verify(dockerCli(module, "push some/image:1.1")) + td.verify(dockerCli(module, ["tag", "some/image:12345", "some/image:1.1"])) + td.verify(dockerCli(module, ["push", "some/image:1.1"])) }) }) })