fix(dev): fix reload error when using templates (#5329)

* fix(dev): fix reload error when using templates

Before this fix, changing a Garden config during a `dev` command session
and running two subsequent commands would result in the first command
succeeding but the second failing with an error like:

"Build foo references template <some-template> which cannot be found.
Vailable templates: <None>"

This was because the config templates scanned by the first subcommand
after the config was changed weren't propagated to the parent Garden
instance, which was then cloned and passed to the second command (which
then had action configs, but no templates).

This was fixed by making the relevant instance variables on the Garden
class readonly (since reassignments in a child instance prevent newly
scanned configs from propagating to the parent via the shared data
structure) and emptying the maps before rescanning instead.

Also added a unit test case that checks this whole flow.

* chore(test): disable config clearing by default

The call to `this.clearConfig()` in `Garden#scanAndAddConfigs` had
started breaking certain test cases that manually set action configs
before calling `garden.getConfigGraph` (which would scan and add
configs, thus discarding the manually set configs).

While it's not great that we're adding more tweaks to the `TestGarden`
class, we're almost never interested in explicitly testing the post-edit
config reloading (outside of tests that test interactive / dev command
funcftionality).

core/src/garden.ts

@@ -11,8 +11,9 @@ const { ensureDir } = fsExtra
 import { platform, arch } from "os"
 import { relative, resolve } from "path"
 import cloneDeep from "fast-copy"
-import { flatten, sortBy, keyBy, mapValues, groupBy, set } from "lodash-es"
 import AsyncLock from "async-lock"
+import { flatten, groupBy, keyBy, mapValues, set, sortBy } from "lodash-es"
+import { username } from "username"
 
 import { TreeCache } from "./cache.js"
 import { getBuiltinPlugins } from "./plugins/plugins.js"
@@ -28,8 +29,15 @@ import {
   defaultNamespace,
   defaultEnvironment,
 } from "./config/project.js"
-import { getCloudDistributionName, getCloudLogSectionName } from "./util/cloud.js"
-import { findByName, pickKeys, getPackageVersion, getNames, findByNames, duplicatesByKey } from "./util/util.js"
+import {
+  findByName,
+  pickKeys,
+  getPackageVersion,
+  getNames,
+  findByNames,
+  duplicatesByKey,
+  clearObject,
+} from "./util/util.js"
 import type { GardenError } from "./exceptions.js"
 import {
   ConfigurationError,
@@ -96,7 +104,6 @@ import {
 import { dedent, deline, naturalList, wordWrap } from "./util/string.js"
 import { DependencyGraph } from "./graph/common.js"
 import { Profile, profileAsync } from "./util/profiling.js"
-import { username } from "username"
 import {
   throwOnMissingSecretKeys,
   resolveTemplateString,
@@ -160,6 +167,7 @@ import { detectModuleOverlap, makeOverlapErrors } from "./util/module-overlap.js
 import { GotHttpError } from "./util/http.js"
 import { styles } from "./logger/styles.js"
 import { renderDuration } from "./logger/util.js"
+import { getCloudDistributionName, getCloudLogSectionName } from "./util/cloud.js"
 
 const defaultLocalAddress = "localhost"
 
@@ -235,9 +243,9 @@ export class Garden {
   public log: Log
   private gardenInitLog?: Log
   private loadedPlugins?: GardenPluginSpec[]
-  protected actionConfigs: ActionConfigMap
-  protected moduleConfigs: ModuleConfigMap
-  protected workflowConfigs: WorkflowConfigMap
+  protected readonly actionConfigs: ActionConfigMap
+  protected readonly moduleConfigs: ModuleConfigMap
+  protected readonly workflowConfigs: WorkflowConfigMap
   protected configPaths: Set<string>
   private resolvedProviders: { [key: string]: Provider }
   protected readonly state: GardenInstanceState
@@ -253,7 +261,7 @@ export class Garden {
   public readonly treeCache: TreeCache
   public events: EventBus
   private tools?: { [key: string]: PluginTool }
-  public configTemplates: { [name: string]: ConfigTemplateConfig }
+  public readonly configTemplates: { [name: string]: ConfigTemplateConfig }
   private actionTypeBases: ActionTypeMap<ActionTypeDefinition<any>[]>
   private emittedWarnings: Set<string>
   public cloudApi: CloudApi | null
@@ -491,6 +499,7 @@ export class Garden {
   needsReload(v?: true) {
     if (v) {
       this.state.needsReload = true
+      this.state.configsScanned = false
     }
     return this.state.needsReload
   }
@@ -1270,6 +1279,23 @@ export class Garden {
     return version
   }
 
+  /**
+   * We clear these objects without reassigning them. If this Garden instance was cloned from a parent Garden instance,
+   * this will also clear the configs of the parent instance (which is what we want e.g. when rescanning configs during
+   * a subcommand after a reload is necessary during a dev command session).
+   *
+   * We need to clear before rescanning to make sure old/outdated configs are cleared away, and to avoid duplicate
+   * key errors when adding the newly scanned ones (and those generated from newly scanned config templates).
+   */
+  protected clearConfigs() {
+    for (const kind of Object.getOwnPropertyNames(this.actionConfigs)) {
+      clearObject(this.actionConfigs[kind])
+    }
+    clearObject(this.moduleConfigs)
+    clearObject(this.workflowConfigs)
+    clearObject(this.configTemplates)
+  }
+
   /**
    * Scans the specified directories for Garden config files and returns a list of paths.
    */
@@ -1305,6 +1331,7 @@ export class Garden {
       }
 
       this.log.silly(() => `Scanning for configs (force=${force})`)
+      this.clearConfigs()
 
       // Add external sources that are defined at the project level. External sources are either kept in
       // the .garden/sources dir (and cloned there if needed), or they're linked to a local path via the link command.
@@ -1425,7 +1452,10 @@ export class Garden {
       )
 
       this.state.configsScanned = true
-      this.configTemplates = { ...this.configTemplates, ...keyBy(configTemplates, "name") }
+
+      for (const template of configTemplates) {
+        this.configTemplates[template.name] = template
+      }
 
       this.events.emit("configsScanned", {})
     })

core/src/util/testing.ts

@@ -14,9 +14,9 @@ import type { GardenOpts, GardenParams, GetConfigGraphParams } from "../garden.j
 import { Garden, resolveGardenParams } from "../garden.js"
 import type { DeepPrimitiveMap, StringMap } from "../config/common.js"
 import type { ModuleConfig } from "../config/module.js"
-import type { WorkflowConfig } from "../config/workflow.js"
+import type { WorkflowConfig, WorkflowConfigMap } from "../config/workflow.js"
 import { resolveMsg, type Log, type LogEntry } from "../logger/log-entry.js"
-import type { GardenModule } from "../types/module.js"
+import type { GardenModule, ModuleConfigMap } from "../types/module.js"
 import { findByName, getNames } from "./util.js"
 import { GardenError, InternalError } from "../exceptions.js"
 import type { EventName, Events } from "../events/events.js"
@@ -32,7 +32,7 @@ import type { ConfigGraph } from "../graph/config-graph.js"
 import type { SolveParams } from "../graph/solver.js"
 import type { GraphResults } from "../graph/results.js"
 import { expect } from "chai"
-import type { ActionConfig, ActionKind, ActionStatus } from "../actions/types.js"
+import type { ActionConfig, ActionConfigMap, ActionKind, ActionStatus } from "../actions/types.js"
 import type { WrappedActionRouterHandlers } from "../router/base.js"
 import type {
   BuiltinArgs,
@@ -48,6 +48,7 @@ const { mkdirp, remove } = fsExtra
 import { GlobalConfigStore } from "../config-store/global.js"
 import { isPromise } from "./objects.js"
 import { styles } from "../logger/styles.js"
+import type { ConfigTemplateConfig } from "../config/config-template.js"
 
 export class TestError extends GardenError {
   type = "_test"
@@ -164,15 +165,22 @@ export type TestGardenOpts = Partial<GardenOpts> & {
   noTempDir?: boolean
   onlySpecifiedPlugins?: boolean
   remoteContainerAuth?: boolean
+  clearConfigsOnScan?: boolean
 }
 
 export class TestGarden extends Garden {
   override events: TestEventBus
-  public declare vcs: VcsHandler // Not readonly, to allow overriding with a mocked handler in tests
-  public declare secrets: StringMap // Not readonly, to allow setting secrets in tests
-  public declare variables: DeepPrimitiveMap // Not readonly, to allow setting variables in tests
+  // Overriding the type declarations of a few instance variables to allow reassignment in test code.
+  public declare actionConfigs: ActionConfigMap
+  public declare moduleConfigs: ModuleConfigMap
+  public declare workflowConfigs: WorkflowConfigMap
+  public declare configTemplates: { [name: string]: ConfigTemplateConfig }
+  public declare vcs: VcsHandler
+  public declare secrets: StringMap
+  public declare variables: DeepPrimitiveMap
   private repoRoot!: string
   public cacheKey!: string
+  public clearConfigsOnScan = false
 
   constructor(params: GardenParams) {
     super(params)
@@ -222,6 +230,14 @@ export class TestGarden extends Garden {
     return garden
   }
 
+  protected override clearConfigs() {
+    if (this.clearConfigsOnScan) {
+      super.clearConfigs()
+    } else {
+      // No-op: We need to disable this method, because it breaks test cases that manually set configs.
+    }
+  }
+
   override async processTasks(params: Omit<SolveParams, "log"> & { log?: Log }) {
     return super.processTasks({ ...params, log: params.log || this.log })
   }

core/src/util/util.ts

@@ -481,6 +481,17 @@ export function getEnvVarName(identifier: string) {
   return identifier.replace(/-/g, "_").toUpperCase()
 }
 
+/**
+ * Removes all keys from `obj` (except those inherited from the object's prototype).
+ *
+ * Essentially a vanilla object analog of `map.clear()` for ES5 Maps.
+ */
+export function clearObject<T extends object>(obj: T) {
+  for (const key of Object.getOwnPropertyNames(obj)) {
+    delete obj[key]
+  }
+}
+
 /**
  * Picks the specified keys from the given object, and throws an error if one or more keys are not found.
  */

core/test/data/test-projects/config-template-reloading/actions.garden.yml

@@ -0,0 +1,9 @@
+kind: RenderTemplate
+template: combo
+name: foo
+inputs:
+  name: test
+  # Resolvable at parse time
+  envName: ${environment.name}
+  # Resolvable later
+  providerKey: ${providers.test-plugin.outputs.testKey}

core/test/data/test-projects/config-template-reloading/project.garden.yml

@@ -0,0 +1,7 @@
+apiVersion: garden.io/v1
+kind: Project
+name: config-templates
+environments:
+  - name: local
+providers:
+  - name: test-plugin

core/test/data/test-projects/config-template-reloading/schema.json

@@ -0,0 +1,14 @@
+{
+  "type": "object",
+  "properties": {
+    "name": {
+      "type": "string"
+    },
+    "envName": {
+      "type": "string"
+    },
+    "providerKey": {
+      "type": "string"
+    }
+  }
+}

core/test/data/test-projects/config-template-reloading/templates.garden.yml

@@ -0,0 +1,30 @@
+kind: ConfigTemplate
+name: combo
+inputsSchemaPath: schema.json
+configs:
+  - kind: Build
+    type: test
+    name: ${parent.name}-${inputs.name}
+    include: []
+    spec:
+      command: [echo, "echo-prefix", "${inputs.name}"]
+  - kind: Deploy
+    type: test
+    name: ${parent.name}-${inputs.name}
+    build: ${parent.name}-${inputs.name}
+  - kind: Test
+    type: test
+    name: ${parent.name}-${inputs.name}
+    dependencies: ["build.${parent.name}-${inputs.name}"]
+    spec:
+      command: [echo, "${inputs.envName}", "${inputs.providerKey}"]
+
+---
+
+kind: ConfigTemplate
+name: workflows
+configs:
+  - kind: Workflow
+    name: ${parent.name}-${inputs.name}
+    steps:
+      - script: echo "${inputs.envName}"

core/test/data/test-projects/config-template-reloading/workflows.garden.yml

@@ -0,0 +1,6 @@
+kind: RenderTemplate
+template: workflows
+name: foo
+inputs:
+  name: test
+  envName: ${environment.name}

core/test/data/test-projects/config-templates/templates.garden.yml

@@ -7,7 +7,7 @@ configs:
     name: ${parent.name}-${inputs.name}
     include: []
     spec:
-      command: ["${inputs.value}"]
+      command: [echo, "echo-prefix", "${inputs.name}"]
   - kind: Deploy
     type: test
     name: ${parent.name}-${inputs.name}

core/test/unit/src/commands/base.ts

@@ -10,8 +10,14 @@ import { expect } from "chai"
 import { Command, CommandGroup } from "../../../../src/commands/base.js"
 import { StringsParameter } from "../../../../src/cli/params.js"
 import stripAnsi from "strip-ansi"
-import { dedent } from "../../../../src/util/string.js"
-import { trimLineEnds } from "../../../helpers.js"
+import replaceInFile from "replace-in-file"
+import { join } from "path"
+import dedent from "dedent"
+import { BuildCommand } from "../../../../src/commands/build.js"
+import { DevCommand } from "../../../../src/commands/dev.js"
+import { ValidateCommand } from "../../../../src/commands/validate.js"
+import { uuidv4 } from "../../../../src/util/random.js"
+import { trimLineEnds, getDataDir, makeTestGarden, withDefaultGlobalOpts } from "../../../helpers.js"
 
 describe("Command", () => {
   describe("renderHelp", () => {
@@ -212,6 +218,83 @@ describe("Command", () => {
       ])
     })
   })
+
+  describe("run", () => {
+    // This applies e.g. when running a command on the dev command line.
+    context("when called with a parentSessionId and a reload is needed after a config change", () => {
+      const projectRoot = getDataDir("test-projects", "config-template-reloading")
+
+      it("passes on changed configs and templates to the parent and subsequent child instances", async () => {
+        const garden = await makeTestGarden(projectRoot)
+        // Here, we're testing the config clearing logic, so we turn this behavior on (which is off by default for
+        // TestGarden instances).
+        garden.clearConfigsOnScan = true
+        // `makeTestGarden` does some trickery to copy the project root into a temp directory and work from there
+        // (which is nice, since it avoids the need for cleanup).
+        // Therefore, we need to make our find & replace substitutions inside the temp dir here.
+        const tmpRoot = garden.projectRoot
+        const log = garden.log
+
+        const devCmd = new DevCommand()
+        const devCmdSessionId = uuidv4()
+
+        const validateCmd = new ValidateCommand()
+        const buildCmd = new BuildCommand()
+
+        // We run this command to trigger the initial scan for configs
+        await validateCmd.run({
+          log,
+          args: {},
+          opts: withDefaultGlobalOpts({}),
+          garden,
+          sessionId: uuidv4(),
+          parentSessionId: devCmdSessionId,
+          parentCommand: devCmd,
+        })
+
+        await replaceInFile.replaceInFile({
+          files: join(tmpRoot, "templates.garden.yml"),
+          from: new RegExp("echo-prefix"),
+          to: "reloaded-prefix",
+        })
+        await replaceInFile.replaceInFile({
+          files: join(tmpRoot, "actions.garden.yml"),
+          from: new RegExp("name: test"),
+          to: "name: reloaded-name",
+        })
+
+        // The modification to `templates.garden.yml` above would normally trigger a `configChanged` event which would
+        // result in this call, but we're doing it manually here to simplify the test setup.
+        garden.needsReload(true)
+        await validateCmd.run({
+          log,
+          args: {},
+          opts: withDefaultGlobalOpts({}),
+          garden,
+          sessionId: uuidv4(),
+          parentSessionId: devCmdSessionId,
+          parentCommand: devCmd,
+        })
+
+        const { result } = await buildCmd.run({
+          log,
+          args: { names: undefined },
+          opts: withDefaultGlobalOpts({ "watch": false, "force": false, "with-dependants": false }),
+          garden,
+          sessionId: uuidv4(),
+          parentSessionId: devCmdSessionId,
+          parentCommand: devCmd,
+        })
+
+        // This means that when the config was rescanned during the second `validate` command run:
+        // * The action, module and workflow configs and config templates were also set on the parent Garden instance
+        //   when the first validate command run rescanned & added configs.
+        //  * The build command run that came immediately after received updated config templates, and the appropriately
+        //    re-generated action configs from the updated template.
+        expect(result.build["foo-reloaded-name"].buildLog).to.eql("reloaded-prefix reloaded-name")
+      })
+    })
+  })
 })
 
 describe("CommandGroup", () => {

core/test/unit/src/garden.ts

@@ -2903,7 +2903,7 @@ describe("Garden", () => {
       expect(test).to.exist
 
       expect(build.type).to.equal("test")
-      expect(build.spec.command).to.eql(["${inputs.value}"]) // <- resolved later
+      expect(build.spec.command).to.include("${inputs.name}") // <- resolved later
       expect(omit(build.internal, "yamlDoc")).to.eql(internal)
 
       expect(deploy["build"]).to.equal("${parent.name}-${inputs.name}") // <- resolved later