fix(module-conversion): skip omitted build deps (#5727)

<!--  Thanks for sending a pull request! Here are some tips for you:

1. If this is your first pull request, please read our contributor
guidelines in the
https://github.com/garden-io/garden/blob/main/CONTRIBUTING.md file.
2. Please label this pull request according to what type of issue you
are addressing (see "What type of PR is this?" below)
3. Ensure you have added or run the appropriate tests for your PR.
4. If the PR is unfinished, add `WIP:` at the beginning of the title or
use the GitHub Draft PR feature.
5. Please add at least two reviewers to the PR. Currently active
maintainers are: @edvald, @thsig, @eysi09, @shumailxyz, @stefreak,
@TimBeyer, @mkhq, and @vvagaytsev.
-->

**What this PR does / why we need it**:

When a module has a build dependency on another module that doesn't have
a corresponding Build after being converted into actions (e.g. a build
dependency on a `terraform` module), we now log a warning and remove the
dependency from the generated actions instead of erroring out.

In 0.12, some users had build dependencies on modules with no-op build
steps (e.g. `terraform`, `kubernetes` and `helm` modules)

While this was harmless in 0.12, this resulted in noisy errors before
this fix.

core/src/resolve-module.ts

@@ -26,7 +26,7 @@ import {
   PluginError,
   toGardenError,
 } from "./exceptions.js"
-import { dedent } from "./util/string.js"
+import { dedent, deline } from "./util/string.js"
 import type { GardenModule, ModuleConfigMap, ModuleMap, ModuleTypeMap } from "./types/module.js"
 import { getModuleTypeBases, moduleFromConfig } from "./types/module.js"
 import type { BuildDependencyConfig, ModuleConfig } from "./config/module.js"
@@ -58,6 +58,7 @@ import type { GraphResults } from "./graph/results.js"
 import type { ExecBuildConfig } from "./plugins/exec/build.js"
 import { pMemoizeDecorator } from "./lib/p-memoize.js"
 import { styles } from "./logger/styles.js"
+import { actionReferenceToString } from "./actions/base.js"
 
 // This limit is fairly arbitrary, but we need to have some cap on concurrent processing.
 export const moduleResolutionConcurrencyLimit = 50
@@ -838,6 +839,47 @@ export const convertModules = profileAsync(async function convertModules(
     })
   )
 
+  const allActions = [...actions, ...groups.flatMap((g) => g.actions)]
+  // Not all conversion handlers return a Build action for the module, so we need to check for references to
+  // build steps for modules that aren't represented by a Build in the post-conversion set of actions.
+  // We warn the user to remove the dangling references, but don't throw an exception and instead simply remove
+  // the dependencies from the relevant actions.
+  const convertedBuildNames = new Set(allActions.filter((a) => isBuildActionConfig(a)).map((a) => a.name))
+  const missingBuildNames = new Set(
+    graph
+      .getModules()
+      .map((m) => m.name)
+      .filter((name) => !convertedBuildNames.has(name))
+  )
+
+  for (const action of allActions) {
+    action.dependencies = (action.dependencies || []).filter((dep) => {
+      let keep = true
+      if (dep.kind === "Build" && missingBuildNames.has(dep.name)) {
+        keep = false
+        const moduleName = action.internal.moduleName
+        const depType = graph.getModule(dep.name)?.type
+        if (moduleName && depType) {
+          log.warn(
+            deline`
+            Action ${styles.highlight(actionReferenceToString(action))} depends on
+            ${styles.highlight("build." + dep.name)} (from module ${styles.highlight(dep.name)} of type ${depType}),
+            which doesn't exist. This is probably because there's no need for a Build action when converting modules
+            of type ${depType} to actions. Skipping this dependency.
+          `
+          )
+          log.warn(
+            deline`
+            Please remove the build dependency on ${styles.highlight(dep.name)} from the module
+            ${styles.highlight(moduleName)}'s configuration.
+          `
+          )
+        }
+      }
+      return keep
+    })
+  }
+
   return { groups, actions }
 })
 

core/test/unit/src/plugins/kubernetes/container/module-conversion.ts

@@ -36,7 +36,7 @@ describe("kubernetes container module conversion", () => {
     tmpDir.cleanup()
   })
 
-  it("should ", async () => {
+  it("should include build dependencies among converted runtime actions' dependencies", async () => {
     garden.setModuleConfigs([
       makeModuleConfig<ContainerModuleConfig>(garden.projectRoot, {
         name: "test-image",

core/test/unit/src/resolve-module.ts

@@ -9,10 +9,22 @@
 import { expect } from "chai"
 import { join, dirname } from "path"
 import type { TestGarden } from "../../helpers.js"
-import { getDataDir, makeTestGarden, makeTestGardenA } from "../../helpers.js"
+import {
+  customizedTestPlugin,
+  expectError,
+  getDataDir,
+  makeTestGarden,
+  makeTestGardenA,
+  projectRootA,
+} from "../../helpers.js"
 import { DEFAULT_BUILD_TIMEOUT_SEC } from "../../../src/constants.js"
 import type { ConfigGraph } from "../../../src/graph/config-graph.js"
 import { loadYamlFile } from "../../../src/util/serialization.js"
+import type { DeployActionConfig } from "../../../src/actions/deploy.js"
+import type { BaseActionConfig } from "../../../src/actions/types.js"
+import { resolveMsg } from "../../../src/logger/log-entry.js"
+import { deline } from "../../../src/util/string.js"
+import stripAnsi from "strip-ansi"
 
 describe("ModuleResolver", () => {
   // Note: We test the ModuleResolver via the TestGarden.resolveModule method, for convenience.
@@ -160,3 +172,99 @@ describe("ModuleResolver", () => {
     })
   })
 })
+
+describe("convertModules", () => {
+  context("when an action has a build dependency on a module whose conversion didn't result in a build ation", () => {
+    it("should remove the build dependency and log an informative warning message", async () => {
+      // it("should always include a dummy build, even when the convert handler doesn't doesn't return a build", async () => {
+      const testPlugin = customizedTestPlugin({
+        name: "test-plugin",
+        createModuleTypes: [
+          {
+            name: "test",
+            docs: "I are documentation, yes",
+            needsBuild: false,
+            handlers: {
+              convert: async (params) => {
+                const { module, services, dummyBuild } = params
+                const actions: BaseActionConfig[] = []
+                for (const service of services) {
+                  const deployAction: DeployActionConfig = {
+                    kind: "Deploy",
+                    type: "test",
+                    name: service.name,
+                    ...params.baseFields,
+
+                    dependencies: params.prepareRuntimeDependencies(service.spec.dependencies, dummyBuild),
+                    timeout: service.spec.timeout,
+
+                    spec: {},
+                  }
+                  actions.push(deployAction)
+                }
+                dummyBuild && actions.push(dummyBuild)
+                return {
+                  group: {
+                    kind: <const>"Group",
+                    name: module.name,
+                    path: module.path,
+                    actions,
+                  },
+                }
+              },
+            },
+          },
+        ],
+      })
+      const garden = await makeTestGarden(projectRootA, { plugins: [testPlugin] })
+      const log = garden.log
+
+      garden.setModuleConfigs([
+        {
+          name: "module-a",
+          type: "test",
+          path: join(garden.projectRoot, "module-a"),
+          spec: {
+            services: [{ name: "service-a", deployCommand: ["echo", "ok"], dependencies: [] }],
+            tests: [],
+            tasks: [],
+            build: { dependencies: [], timeout: DEFAULT_BUILD_TIMEOUT_SEC },
+          },
+          build: { dependencies: [], timeout: DEFAULT_BUILD_TIMEOUT_SEC },
+        },
+        {
+          name: "module-b",
+          type: "test",
+          path: join(garden.projectRoot, "module-b"),
+          spec: {
+            services: [{ name: "service-b", deployCommand: ["echo", "ok"], dependencies: ["service-a"] }],
+            tests: [],
+            tasks: [],
+            build: { dependencies: ["module-a"], timeout: DEFAULT_BUILD_TIMEOUT_SEC },
+          },
+          build: { dependencies: [{ name: "module-a", copy: [] }], timeout: DEFAULT_BUILD_TIMEOUT_SEC },
+        },
+      ])
+
+      const graph = await garden.getConfigGraph({ log, emit: false })
+
+      expectError(() => graph.getBuild("module-a"), { contains: "Could not find Build action module-a" })
+      expectError(() => graph.getBuild("module-b"), { contains: "Could not find Build action module-b" })
+
+      const deployActionDeps = graph
+        .getDeploy("service-b")
+        .getDependencies()
+        .map((dep) => dep.key())
+        .sort()
+
+      expect(deployActionDeps).to.eql(["deploy.service-a"])
+      const warningMsgSnippet = deline`
+        Action deploy.service-b depends on build.module-a (from module module-a of type test), which doesn't exist.
+      `
+      const expectedLog = log.root
+        .getLogEntries()
+        .filter((l) => stripAnsi(resolveMsg(l) || "").includes(warningMsgSnippet))
+      expect(expectedLog.length).to.be.greaterThan(0)
+    })
+  })
+})