Merge pull request #660 from garden-io/fix-init-issue

Fix init issue

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<ContainerModule>) {
   const identifier = await containerHelpers.imageExistsLocally(module)
@@ -27,8 +28,9 @@ export async function getContainerBuildStatus({ module, log }: GetBuildStatusPar
 export async function buildContainerModule({ module, log }: BuildModuleParams<ContainerModule>) {
   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 }: BuildModuleParams<Co
     return { fetched: true }
   }
 
+  // make sure we can build the thing
+  if (!hasDockerfile) {
+    throw new ConfigurationError(
+      `Dockerfile not found at ${module.spec.dockerfile || "Dockerfile"} for module ${module.name}`,
+      { spec: module.spec },
+    )
+  }
+
   const identifier = await containerHelpers.getLocalImageId(module)
 
   // build doesn't exist, so we create it
@@ -53,7 +63,7 @@ export async function buildContainerModule({ module, log }: BuildModuleParams<Co
   }
 
   if (module.spec.dockerfile) {
-    cmdOpts.push("--file", containerHelpers.getDockerfilePathFromModule(module))
+    cmdOpts.push("--file", containerHelpers.getDockerfileBuildPath(module))
   }
 
   // TODO: log error if it occurs

garden-service/src/plugins/container/container.ts

@@ -9,7 +9,6 @@
 import dedent = require("dedent")
 
 import { validateWithPath } from "../../config/common"
-import { pathExists } from "fs-extra"
 import { ConfigurationError } from "../../exceptions"
 import { GardenPlugin } from "../../types/plugin/plugin"
 import { ConfigureModuleParams, HotReloadServiceParams, PublishModuleParams } from "../../types/plugin/params"
@@ -120,23 +119,6 @@ export async function configureContainerModule({ ctx, moduleConfig }: ConfigureM
     timeout: t.timeout,
   }))
 
-  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}`,
-      {},
-    )
-  }
-
-  if (!moduleConfig.spec.image && !hasDockerfile) {
-    throw new ConfigurationError(
-      `Module ${moduleConfig.name} neither specifies image nor provides Dockerfile`,
-      { moduleConfig },
-    )
-  }
-
   const provider = <KubernetesProvider>ctx.provider
   const deploymentImageName = await containerHelpers.getDeploymentImageName(
     moduleConfig,

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)
   },
 

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: {

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,15 +241,15 @@ 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"))
     })
 
     it("should return the absolute user specified Dockerfile path", async () => {
       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,

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": {}
-}
+}