From 75facfc2d651ae9b4e7d56154938f2196195584f Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Fri, 22 Mar 2019 17:03:16 +0100 Subject: [PATCH 1/2] chore: clean .garden directories in `npm run clean` script --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 8b0c08ff15..14af4f16f0 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "check-licenses": "gulp check-licenses", "check-package-lock": "git diff --quiet HEAD -- package-lock.json || (echo 'package-lock.json is dirty!' && exit 1)", "check-all": "npm run check-docs && npm run check-package-lock && npm run check-licenses && npm run lint", - "clean": "lerna run clean && git clean -X -f", + "clean": "lerna run clean && git clean -X -f && find . -name \".garden\" -type d -prune -exec rm -rf '{}' '+'", "fix-format": "tslint -p . --fix && tsfmt -r", "generate-docs": "gulp generate-docs", "integ": "lerna run integ", @@ -67,4 +67,4 @@ }, "snyk": true, "dependencies": {} -} +} \ No newline at end of file From 5230408a40b498c61814410629c2e95548c04129 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Fri, 22 Mar 2019 17:39:02 +0100 Subject: [PATCH 2/2] fix(container): further issues with deployment image IDs --- garden-service/src/plugins/container/build.ts | 14 ++++- .../src/plugins/container/container.ts | 18 ------- .../src/plugins/container/helpers.ts | 20 +++++--- .../maven-container/maven-container.ts | 7 +++ ...{Dockerfile => maven-container.Dockerfile} | 0 .../test/unit/src/plugins/container.ts | 51 ++++++++++++------- 6 files changed, 64 insertions(+), 46 deletions(-) rename garden-service/static/maven-container/{Dockerfile => maven-container.Dockerfile} (100%) diff --git a/garden-service/src/plugins/container/build.ts b/garden-service/src/plugins/container/build.ts index e3c7cbd1df..c13f1a46cf 100644 --- a/garden-service/src/plugins/container/build.ts +++ b/garden-service/src/plugins/container/build.ts @@ -9,6 +9,7 @@ import { BuildModuleParams, GetBuildStatusParams } from "../../types/plugin/params" import { containerHelpers } from "./helpers" import { ContainerModule } from "./config" +import { ConfigurationError } from "../../exceptions" export async function getContainerBuildStatus({ module, log }: GetBuildStatusParams) { const identifier = await containerHelpers.imageExistsLocally(module) @@ -27,8 +28,9 @@ export async function getContainerBuildStatus({ module, log }: GetBuildStatusPar export async function buildContainerModule({ module, log }: BuildModuleParams) { const buildPath = module.buildPath const image = module.spec.image + const hasDockerfile = await containerHelpers.hasDockerfile(module) - if (!!image && !(await containerHelpers.hasDockerfile(module))) { + if (!!image && !hasDockerfile) { if (await containerHelpers.imageExistsLocally(module)) { return { fresh: false } } @@ -37,6 +39,14 @@ export async function buildContainerModule({ module, log }: BuildModuleParamsctx.provider const deploymentImageName = await containerHelpers.getDeploymentImageName( moduleConfig, diff --git a/garden-service/src/plugins/container/helpers.ts b/garden-service/src/plugins/container/helpers.ts index 2c762cf10f..46c087b814 100644 --- a/garden-service/src/plugins/container/helpers.ts +++ b/garden-service/src/plugins/container/helpers.ts @@ -112,10 +112,14 @@ export const containerHelpers = { repository: imageName, tag: module.version.versionString, }) - } else { + } else if (module.spec.image) { // Otherwise, return the configured image ID. - // Note: This will always be set here, otherwise validation will have failed already. - return module.spec.image! + return module.spec.image + } else { + throw new ConfigurationError( + `Module ${module.name} neither specifies image nor provides Dockerfile`, + { spec: module.spec }, + ) } }, @@ -186,15 +190,17 @@ export const containerHelpers = { return output.output || "" }, - async hasDockerfile(module: ContainerModule) { - return pathExists(containerHelpers.getDockerfilePathFromModule(module)) + async hasDockerfile(moduleConfig: ContainerModuleConfig) { + // 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 moduleConfig.spec.dockerfile || pathExists(containerHelpers.getDockerfileSourcePath(moduleConfig)) }, - getDockerfilePathFromModule(module: ContainerModule) { + getDockerfileBuildPath(module: ContainerModule) { return getDockerfilePath(module.buildPath, module.spec.dockerfile) }, - getDockerfilePathFromConfig(config: ModuleConfig) { + getDockerfileSourcePath(config: ModuleConfig) { return getDockerfilePath(config.path, config.spec.dockerfile) }, diff --git a/garden-service/src/plugins/maven-container/maven-container.ts b/garden-service/src/plugins/maven-container/maven-container.ts index bfff88c6c0..ecf7e52131 100644 --- a/garden-service/src/plugins/maven-container/maven-container.ts +++ b/garden-service/src/plugins/maven-container/maven-container.ts @@ -111,6 +111,13 @@ export async function configureMavenContainerModule(params: ConfigureModuleParam const configured = await configureContainerModule({ ...params, moduleConfig: containerConfig }) + const hasOwnDockerfile = await containerHelpers.hasDockerfile(moduleConfig) + + if (!hasOwnDockerfile) { + // Set the default Dockerfile provided by the plugin + configured.spec.dockerfile = "maven-container.Dockerfile" + } + return { ...configured, spec: { diff --git a/garden-service/static/maven-container/Dockerfile b/garden-service/static/maven-container/maven-container.Dockerfile similarity index 100% rename from garden-service/static/maven-container/Dockerfile rename to garden-service/static/maven-container/maven-container.Dockerfile diff --git a/garden-service/test/unit/src/plugins/container.ts b/garden-service/test/unit/src/plugins/container.ts index e6c94436ea..41122c6d7f 100644 --- a/garden-service/test/unit/src/plugins/container.ts +++ b/garden-service/test/unit/src/plugins/container.ts @@ -145,15 +145,22 @@ describe("plugins.container", () => { expect(await containerHelpers.getDeploymentImageId(module)).to.equal("some/image:1.1") }) + + it("should throw if no image name is set and there is no Dockerfile", async () => { + const config = cloneDeep(baseConfig) + const module = await getTestModule(config) + + td.replace(containerHelpers, "hasDockerfile", async () => false) + + await expectError(() => containerHelpers.getDeploymentImageId(module), "configuration") + }) }) - describe("getDockerfilePathFromModule", () => { + describe("getDockerfileBuildPath", () => { it("should return the absolute default Dockerfile path", async () => { const module = await getTestModule(baseConfig) - td.replace(containerHelpers, "hasDockerfile", async () => true) - - const path = await containerHelpers.getDockerfilePathFromModule(module) + const path = await containerHelpers.getDockerfileBuildPath(module) expect(path).to.equal(join(module.buildPath, "Dockerfile")) }) @@ -162,13 +169,29 @@ describe("plugins.container", () => { config.spec.dockerfile = relDockerfilePath const module = await getTestModule(config) - td.replace(containerHelpers, "hasDockerfile", async () => true) - - const path = await containerHelpers.getDockerfilePathFromModule(module) + const path = await containerHelpers.getDockerfileBuildPath(module) expect(path).to.equal(join(module.buildPath, relDockerfilePath)) }) }) + describe("getDockerfileSourcePath", () => { + it("should return the absolute default Dockerfile path", async () => { + const module = await getTestModule(baseConfig) + + const path = await containerHelpers.getDockerfileSourcePath(module) + expect(path).to.equal(join(module.path, "Dockerfile")) + }) + + it("should return the absolute user specified Dockerfile path", async () => { + const config = cloneDeep(baseConfig) + config.spec.dockerfile = relDockerfilePath + const module = await getTestModule(config) + + const path = await containerHelpers.getDockerfileSourcePath(module) + expect(path).to.equal(join(module.path, relDockerfilePath)) + }) + }) + describe("getPublicImageId", () => { it("should use image name including version if specified", async () => { const config = cloneDeep(baseConfig) @@ -218,7 +241,7 @@ describe("plugins.container", () => { describe("getDockerfilePathFromConfig", () => { it("should return the absolute default Dockerfile path", async () => { - const path = await containerHelpers.getDockerfilePathFromConfig(baseConfig) + const path = await containerHelpers.getDockerfileSourcePath(baseConfig) expect(path).to.equal(join(baseConfig.path, "Dockerfile")) }) @@ -226,7 +249,7 @@ describe("plugins.container", () => { const config = cloneDeep(baseConfig) config.spec.dockerfile = relDockerfilePath - const path = await containerHelpers.getDockerfilePathFromConfig(config) + const path = await containerHelpers.getDockerfileSourcePath(config) expect(path).to.equal(join(config.path, relDockerfilePath)) }) }) @@ -512,16 +535,6 @@ 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( - () => configure({ ctx, moduleConfig, log }), - "configuration", - ) - }) - it("should fail with invalid port in ingress spec", async () => { const moduleConfig: ContainerModuleConfig = { allowPublish: false,