From bf5d42898ba3ad93db21e716c201ee49f4dbc286 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Thu, 7 Mar 2019 22:49:12 +0100 Subject: [PATCH] fix(container): use configured image ID locally --- .../src/plugins/container/container.ts | 4 +-- .../src/plugins/container/helpers.ts | 31 +++++++++++++---- garden-service/test/src/plugins/container.ts | 34 +++++++++++++++++-- 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/garden-service/src/plugins/container/container.ts b/garden-service/src/plugins/container/container.ts index 8b371a0724..0039c51467 100644 --- a/garden-service/src/plugins/container/container.ts +++ b/garden-service/src/plugins/container/container.ts @@ -126,6 +126,7 @@ export async function configureContainerModule({ ctx, moduleConfig }: ConfigureM const hasDockerfile = await pathExists(containerHelpers.getDockerfilePathFromConfig(moduleConfig)) + // make sure we can build the thing if (moduleConfig.spec.dockerfile && !hasDockerfile) { throw new ConfigurationError( `Dockerfile not found at specififed path ${moduleConfig.spec.dockerfile} for module ${moduleConfig.name}`, @@ -133,11 +134,10 @@ export async function configureContainerModule({ ctx, moduleConfig }: ConfigureM ) } - // make sure we can build the thing if (!moduleConfig.spec.image && !hasDockerfile) { throw new ConfigurationError( `Module ${moduleConfig.name} neither specifies image nor provides Dockerfile`, - {}, + { moduleConfig }, ) } diff --git a/garden-service/src/plugins/container/helpers.ts b/garden-service/src/plugins/container/helpers.ts index 2421ce7982..ff6ad44616 100644 --- a/garden-service/src/plugins/container/helpers.ts +++ b/garden-service/src/plugins/container/helpers.ts @@ -11,7 +11,7 @@ import { join } from "path" import { ConfigurationError } from "../../exceptions" import { splitFirst, spawn } from "../../util/util" import { ModuleConfig } from "../../config/module" -import { ContainerModule, ContainerRegistryConfig, defaultTag, defaultNamespace } from "./config" +import { ContainerModule, ContainerRegistryConfig, defaultTag } from "./config" interface ParsedImageId { host?: string @@ -35,11 +35,22 @@ export const containerHelpers = { * (when we don't need to push to remote registries). */ async getLocalImageId(module: ContainerModule): Promise { - if (await containerHelpers.hasDockerfile(module)) { + const hasDockerfile = await containerHelpers.hasDockerfile(module) + + if (module.spec.image && hasDockerfile) { + const { versionString } = module.version + const parsedImage = containerHelpers.parseImageId(module.spec.image || module.name) + return containerHelpers.unparseImageId({ ...parsedImage, tag: versionString }) + } else if (!module.spec.image && hasDockerfile) { const { versionString } = module.version - return `${module.name}:${versionString}` + return containerHelpers.unparseImageId({ repository: module.name, tag: versionString }) + } else if (module.spec.image && !hasDockerfile) { + return module.spec.image } else { - return module.spec.image! + throw new ConfigurationError( + `Module ${module.name} neither specifies image nor provides Dockerfile`, + { module }, + ) } }, @@ -98,21 +109,27 @@ export const containerHelpers = { if (parts.length === 1) { return { - namespace: defaultNamespace, repository, tag, } } else if (parts.length === 2) { return { namespace: parts[0], - repository, + repository: parts[1], tag, } } else if (parts.length === 3) { return { host: parts[0], namespace: parts[1], - repository, + repository: parts[2], + tag, + } + } else if (parts.length > 3) { + return { + host: parts[0], + namespace: parts.slice(1, parts.length - 1).join("/"), + repository: parts[parts.length - 1], tag, } } else { diff --git a/garden-service/test/src/plugins/container.ts b/garden-service/test/src/plugins/container.ts index f585b5db6d..ecd7ec8d2a 100644 --- a/garden-service/test/src/plugins/container.ts +++ b/garden-service/test/src/plugins/container.ts @@ -77,17 +77,17 @@ describe("plugins.container", () => { } describe("getLocalImageId", () => { - it("should create identifier with commit hash version if module has a Dockerfile", async () => { + it("should return configured image name with local version if module has a Dockerfile", async () => { const config = cloneDeep(baseConfig) config.spec.image = "some/image:1.1" const module = await getTestModule(config) td.replace(containerHelpers, "hasDockerfile", async () => true) - expect(await containerHelpers.getLocalImageId(module)).to.equal("test:1234") + expect(await containerHelpers.getLocalImageId(module)).to.equal("some/image:1234") }) - it("should create identifier with image name if module has no Dockerfile", async () => { + it("should return configured image name and tag if module has no Dockerfile and name includes tag", async () => { const config = cloneDeep(baseConfig) config.spec.image = "some/image:1.1" const module = await getTestModule(config) @@ -96,6 +96,34 @@ describe("plugins.container", () => { expect(await containerHelpers.getLocalImageId(module)).to.equal("some/image:1.1") }) + + it("should return configured image with local version if module has Dockerfile and name has no tag", async () => { + const config = cloneDeep(baseConfig) + config.spec.image = "some/image" + const module = await getTestModule(config) + + td.replace(containerHelpers, "hasDockerfile", async () => true) + + expect(await containerHelpers.getLocalImageId(module)).to.equal("some/image:1234") + }) + + it("should return module name with local version if there is no configured name", async () => { + const config = cloneDeep(baseConfig) + const module = await getTestModule(config) + + td.replace(containerHelpers, "hasDockerfile", async () => true) + + expect(await containerHelpers.getLocalImageId(module)).to.equal("test:1234") + }) + + it("should throw if there is no Dockerfile and no image specified", async () => { + const config = cloneDeep(baseConfig) + const module = await getTestModule(config) + + td.replace(containerHelpers, "hasDockerfile", async () => false) + + await expectError(() => containerHelpers.getLocalImageId(module), "configuration") + }) }) describe("getDockerfilePathFromModule", () => {