fix(core): missing dep detection in ConfigGraph

Before this fix, we were throwing missing dependency errors in ConfigGraph's constructor after relations were added, which could result in null errors in the case of missing runtime dependencies. This was fixed by throwing missing dependency errors before adding relations in the constructor.
garden-io · Apr 8, 2020 · 006ad98 · 006ad98
1 parent aad5023
commit 006ad98
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 54 deletions.
diff --git a/garden-service/src/config-graph.ts b/garden-service/src/config-graph.ts
@@ -16,11 +16,7 @@ import { TestConfig } from "./config/test"
 import { uniqByName, pickKeys } from "./util/util"
 import { ConfigurationError } from "./exceptions"
 import { deline } from "./util/string"
-import {
-  detectMissingDependencies,
-  handleDependencyErrors,
-  DependencyValidationGraph,
-} from "./util/validate-dependencies"
+import { detectMissingDependencies, DependencyValidationGraph } from "./util/validate-dependencies"
 import { ServiceConfig } from "./config/service"
 import { TaskConfig } from "./config/task"
 import { makeTestTaskName } from "./tasks/helpers"
@@ -167,7 +163,7 @@ export class ConfigGraph {
       }
     }
 
-    const missingDepsError = detectMissingDependencies(Object.values(this.modules))
+    detectMissingDependencies(Object.values(this.modules))
 
     // Add relations between nodes
     for (const module of modules) {
@@ -267,17 +263,11 @@ export class ConfigGraph {
     const validationGraph = DependencyValidationGraph.fromDependencyGraph(this.dependencyGraph)
     const cycles = validationGraph.detectCircularDependencies()
 
-    let circularDepsError
     if (cycles.length > 0) {
       const description = validationGraph.cyclesToString(cycles)
       const errMsg = `\nCircular dependencies detected: \n\n${description}\n`
-      circularDepsError = new ConfigurationError(errMsg, { "circular-dependencies": description })
-    } else {
-      circularDepsError = null
+      throw new ConfigurationError(errMsg, { "circular-dependencies": description })
     }
-
-    // Throw an error if one or both of these errors is non-null.
-    handleDependencyErrors(missingDepsError, circularDepsError)
   }
 
   // Convenience method used in the constructor above.

diff --git a/garden-service/src/util/validate-dependencies.ts b/garden-service/src/util/validate-dependencies.ts
@@ -7,7 +7,7 @@
  */
 
 import { DepGraph } from "dependency-graph"
-import { merge, flatten, uniq } from "lodash"
+import { flatten, uniq } from "lodash"
 import indentString from "indent-string"
 import { get, isEqual, join, set, uniqWith } from "lodash"
 import { getModuleKey } from "../types/module"
@@ -17,33 +17,11 @@ import { deline } from "./string"
 import { DependencyGraph, DependencyGraphNode, nodeKey as configGraphNodeKey } from "../config-graph"
 import { Profile } from "./profiling"
 
-export function handleDependencyErrors(
-  missingDepsError: ConfigurationError | null,
-  circularDepsError: ConfigurationError | null
-) {
-  let errMsg = ""
-  let detail = {}
-
-  if (missingDepsError) {
-    errMsg += missingDepsError.message
-    detail = merge(detail, missingDepsError.detail)
-  }
-
-  if (circularDepsError) {
-    errMsg += "\n" + circularDepsError.message
-    detail = merge(detail, circularDepsError.detail)
-  }
-
-  if (missingDepsError || circularDepsError) {
-    throw new ConfigurationError(errMsg, detail)
-  }
-}
-
 /**
- * Looks for dependencies on non-existent modules, services or tasks, and returns an error
+ * Looks for dependencies on non-existent modules, services or tasks, and throws a ConfigurationError
  * if any were found.
  */
-export function detectMissingDependencies(moduleConfigs: ModuleConfig[]): ConfigurationError | null {
+export function detectMissingDependencies(moduleConfigs: ModuleConfig[]) {
   const moduleNames: Set<string> = new Set(moduleConfigs.map((m) => m.name))
   const serviceNames = moduleConfigs.flatMap((m) => m.serviceConfigs.map((s) => s.name))
   const taskNames = moduleConfigs.flatMap((m) => m.taskConfigs.map((t) => t.name))
@@ -79,13 +57,11 @@ export function detectMissingDependencies(moduleConfigs: ModuleConfig[]): Config
   if (missingDepDescriptions.length > 0) {
     const errMsg = "Unknown dependencies detected.\n\n" + indentString(missingDepDescriptions.join("\n\n"), 2) + "\n"
 
-    return new ConfigurationError(errMsg, {
+    throw new ConfigurationError(errMsg, {
       unknownDependencies: missingDepDescriptions,
       availableModules: Array.from(moduleNames),
       availableServicesAndTasks: Array.from(runtimeNames),
     })
-  } else {
-    return null
   }
 }
 

diff --git a/garden-service/test/helpers.ts b/garden-service/test/helpers.ts
@@ -28,7 +28,7 @@ import { Garden, GardenParams, GardenOpts } from "../src/garden"
 import { ModuleConfig } from "../src/config/module"
 import { mapValues, fromPairs } from "lodash"
 import { ModuleVersion } from "../src/vcs/vcs"
-import { GARDEN_SERVICE_ROOT, LOCAL_CONFIG_FILENAME } from "../src/constants"
+import { GARDEN_SERVICE_ROOT, LOCAL_CONFIG_FILENAME, DEFAULT_API_VERSION } from "../src/constants"
 import { EventBus, Events } from "../src/events"
 import { ValueOf, exec, findByName, getNames } from "../src/util/util"
 import { LogEntry } from "../src/logger/log-entry"
@@ -276,7 +276,7 @@ export const testPluginC = createGardenPlugin({
 })
 
 const defaultModuleConfig: ModuleConfig = {
-  apiVersion: "garden.io/v0",
+  apiVersion: DEFAULT_API_VERSION,
   type: "test",
   name: "test",
   path: "bla",

diff --git a/garden-service/test/unit/src/config-graph.ts b/garden-service/test/unit/src/config-graph.ts
@@ -9,7 +9,8 @@
 import { resolve, join } from "path"
 import { expect } from "chai"
 import { ensureDir } from "fs-extra"
-import { makeTestGardenA, makeTestGarden, dataDir, expectError } from "../../helpers"
+import stripAnsi from "strip-ansi"
+import { makeTestGardenA, makeTestGarden, dataDir, expectError, makeTestModule } from "../../helpers"
 import { getNames } from "../../../src/util/util"
 import { ConfigGraph, DependencyGraphNode } from "../../../src/config-graph"
 import { Garden } from "../../../src/garden"
@@ -132,6 +133,58 @@ describe("ConfigGraph", () => {
 
       throw new Error("Expected error")
     })
+
+    it("should throw if a build dependency is missing", async () => {
+      const garden = await makeTestGardenA()
+
+      garden.setModuleConfigs([
+        makeTestModule({
+          name: "test",
+          path: tmpPath,
+          build: {
+            dependencies: [{ name: "missing-build-dep", copy: [] }],
+          },
+        }),
+      ])
+
+      await expectError(
+        () => garden.getConfigGraph(garden.log),
+        (err) =>
+          expect(stripAnsi(err.message)).to.match(
+            /Could not find build dependency missing-build-dep, configured in module test/
+          )
+      )
+    })
+
+    it("should throw if a runtime dependency is missing", async () => {
+      const garden = await makeTestGardenA()
+
+      garden.setModuleConfigs([
+        makeTestModule({
+          name: "test",
+          path: tmpPath,
+          spec: {
+            services: [
+              {
+                name: "test-service",
+                dependencies: ["missing-runtime-dep"],
+                disabled: false,
+                hotReloadable: false,
+                spec: {},
+              },
+            ],
+          },
+        }),
+      ])
+
+      await expectError(
+        () => garden.getConfigGraph(garden.log),
+        (err) =>
+          expect(stripAnsi(err.message)).to.match(
+            /Unknown service or task 'missing-runtime-dep' referenced in dependencies/
+          )
+      )
+    })
   })
 
   describe("getServices", () => {

diff --git a/garden-service/test/unit/src/util/validate-dependencies.ts b/garden-service/test/unit/src/util/validate-dependencies.ts
@@ -37,8 +37,7 @@ describe("validate-dependencies", () => {
           spec: {},
         },
       ]
-      const err = detectMissingDependencies(moduleConfigs)
-      expect(err).to.be.an.instanceOf(ConfigurationError)
+      expect(() => detectMissingDependencies(moduleConfigs)).to.throw()
     })
 
     it("should return an error when a service dependency is missing", async () => {
@@ -66,8 +65,7 @@ describe("validate-dependencies", () => {
           spec: {},
         },
       ]
-      const err = detectMissingDependencies(moduleConfigs)
-      expect(err).to.be.an.instanceOf(ConfigurationError)
+      expect(() => detectMissingDependencies(moduleConfigs)).to.throw()
     })
 
     it("should return an error when a task dependency is missing", async () => {
@@ -96,8 +94,7 @@ describe("validate-dependencies", () => {
           spec: {},
         },
       ]
-      const err = detectMissingDependencies(moduleConfigs)
-      expect(err).to.be.an.instanceOf(ConfigurationError)
+      expect(() => detectMissingDependencies(moduleConfigs)).to.throw()
     })
 
     it("should return an error when a test dependency is missing", async () => {
@@ -125,8 +122,7 @@ describe("validate-dependencies", () => {
           spec: {},
         },
       ]
-      const err = detectMissingDependencies(moduleConfigs)
-      expect(err).to.be.an.instanceOf(ConfigurationError)
+      expect(() => detectMissingDependencies(moduleConfigs)).to.throw()
     })
 
     it("should return null when no dependencies are missing", async () => {
@@ -146,8 +142,7 @@ describe("validate-dependencies", () => {
           spec: {},
         },
       ]
-      const result = detectMissingDependencies(moduleConfigs)
-      expect(result).to.be.null
+      expect(() => detectMissingDependencies(moduleConfigs))
     })
   })