From 9752310329f7779e1614a72653469d954c6085fb Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Fri, 1 Dec 2023 13:37:01 +0100 Subject: [PATCH] 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 which cannot be found. Vailable templates: " 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 | 48 ++++++++-- core/src/util/testing.ts | 28 ++++-- core/src/util/util.ts | 11 +++ .../actions.garden.yml | 9 ++ .../project.garden.yml | 7 ++ .../config-template-reloading/schema.json | 14 +++ .../templates.garden.yml | 30 +++++++ .../workflows.garden.yml | 6 ++ .../config-templates/templates.garden.yml | 2 +- core/test/unit/src/commands/base.ts | 87 ++++++++++++++++++- core/test/unit/src/garden.ts | 2 +- 11 files changed, 225 insertions(+), 19 deletions(-) create mode 100644 core/test/data/test-projects/config-template-reloading/actions.garden.yml create mode 100644 core/test/data/test-projects/config-template-reloading/project.garden.yml create mode 100644 core/test/data/test-projects/config-template-reloading/schema.json create mode 100644 core/test/data/test-projects/config-template-reloading/templates.garden.yml create mode 100644 core/test/data/test-projects/config-template-reloading/workflows.garden.yml diff --git a/core/src/garden.ts b/core/src/garden.ts index 362746d043..686427a1df 100644 --- a/core/src/garden.ts +++ b/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 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[]> private emittedWarnings: Set 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", {}) }) diff --git a/core/src/util/testing.ts b/core/src/util/testing.ts index c581de7a6a..7e3d7aa419 100644 --- a/core/src/util/testing.ts +++ b/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 & { 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 & { log?: Log }) { return super.processTasks({ ...params, log: params.log || this.log }) } diff --git a/core/src/util/util.ts b/core/src/util/util.ts index 0a69024fa5..a9e1ade995 100644 --- a/core/src/util/util.ts +++ b/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(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. */ diff --git a/core/test/data/test-projects/config-template-reloading/actions.garden.yml b/core/test/data/test-projects/config-template-reloading/actions.garden.yml new file mode 100644 index 0000000000..cc4f7e1a56 --- /dev/null +++ b/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} diff --git a/core/test/data/test-projects/config-template-reloading/project.garden.yml b/core/test/data/test-projects/config-template-reloading/project.garden.yml new file mode 100644 index 0000000000..5b75835ce6 --- /dev/null +++ b/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 diff --git a/core/test/data/test-projects/config-template-reloading/schema.json b/core/test/data/test-projects/config-template-reloading/schema.json new file mode 100644 index 0000000000..e337ba56f5 --- /dev/null +++ b/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" + } + } +} diff --git a/core/test/data/test-projects/config-template-reloading/templates.garden.yml b/core/test/data/test-projects/config-template-reloading/templates.garden.yml new file mode 100644 index 0000000000..b456a7b2d3 --- /dev/null +++ b/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}" diff --git a/core/test/data/test-projects/config-template-reloading/workflows.garden.yml b/core/test/data/test-projects/config-template-reloading/workflows.garden.yml new file mode 100644 index 0000000000..95e2719001 --- /dev/null +++ b/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} diff --git a/core/test/data/test-projects/config-templates/templates.garden.yml b/core/test/data/test-projects/config-templates/templates.garden.yml index 54c09948a1..b456a7b2d3 100644 --- a/core/test/data/test-projects/config-templates/templates.garden.yml +++ b/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} diff --git a/core/test/unit/src/commands/base.ts b/core/test/unit/src/commands/base.ts index 5ebf6107e8..85d5257050 100644 --- a/core/test/unit/src/commands/base.ts +++ b/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", () => { diff --git a/core/test/unit/src/garden.ts b/core/test/unit/src/garden.ts index a8fa2d4817..1b1651e0af 100644 --- a/core/test/unit/src/garden.ts +++ b/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