From dbf2df4a46cf0ce75eef5e69d78dac7dcd300546 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Mon, 2 Jan 2023 18:28:40 +0100 Subject: [PATCH 1/6] fix: re-initialize providers changing environments When users change environments, we need to re-initialize providers. Let's consider the following example: User has a provider config like so: ``` providers: - name: exec initScript: echo ${environment.name} > someFile ``` If user runs a garden command with --env foo, and then with --env bar, and then again with --env foo we would expect `someFile` to contain `foo`. But the file actually contains bar. Reason for this is that garden skipped executing the initScript in the foo env, because it has already been executed. But the cache logic is not aware that the last command that ran was in the bar env. This commit fixes that problem. co-authored-by: srihas-g --- core/src/tasks/resolve-provider.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/core/src/tasks/resolve-provider.ts b/core/src/tasks/resolve-provider.ts index c726344856..4ed727beaa 100644 --- a/core/src/tasks/resolve-provider.ts +++ b/core/src/tasks/resolve-provider.ts @@ -45,11 +45,13 @@ interface Params extends TaskParams { interface CachedStatus extends EnvironmentStatus { configHash: string + environmentName: string resolvedAt: Date } const cachedStatusSchema = environmentStatusSchema().keys({ configHash: joi.string().required(), + environmentName: joi.string().required(), resolvedAt: joi.date().required(), }) @@ -253,7 +255,6 @@ export class ResolveProviderTask extends BaseTask { return getProviderStatusCachePath({ gardenDirPath: this.garden.gardenDirPath, pluginName: this.plugin.name, - environmentName: this.garden.environmentName, }) } @@ -282,6 +283,11 @@ export class ResolveProviderTask extends BaseTask { return null } + if (cachedStatus.environmentName !== this.garden.environmentName) { + this.log.silly(`Cached environment name at ${cachePath} does not match the current environmnent name`) + return null + } + const configHash = this.hashConfig(config) if (cachedStatus.configHash !== configHash) { @@ -297,7 +303,7 @@ export class ResolveProviderTask extends BaseTask { return null } - return omit(cachedStatus, ["configHash", "resolvedAt"]) + return omit(cachedStatus, ["configHash", "resolvedAt", "environmentName"]) } private async setCachedStatus(config: GenericProviderConfig, status: EnvironmentStatus) { @@ -381,11 +387,9 @@ export class ResolveProviderTask extends BaseTask { export function getProviderStatusCachePath({ gardenDirPath, pluginName, - environmentName, }: { gardenDirPath: string pluginName: string - environmentName: string }) { - return join(gardenDirPath, "cache", "provider-statuses", `${pluginName}.${environmentName}.json`) + return join(gardenDirPath, "cache", "provider-statuses", `${pluginName}.json`) } From 9a5f82d1d84cae044f9943709c8d0d7ebfe11cd8 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 3 Jan 2023 11:15:20 +0100 Subject: [PATCH 2/6] improvement: remove unnecessary check for env name Reasoning is that when the env name changes and that is relevant, the provider config changes too (assuming that we do not implicitly pass additional information like the environment name using env variables) --- core/src/tasks/resolve-provider.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/core/src/tasks/resolve-provider.ts b/core/src/tasks/resolve-provider.ts index 4ed727beaa..34e490495c 100644 --- a/core/src/tasks/resolve-provider.ts +++ b/core/src/tasks/resolve-provider.ts @@ -45,13 +45,11 @@ interface Params extends TaskParams { interface CachedStatus extends EnvironmentStatus { configHash: string - environmentName: string resolvedAt: Date } const cachedStatusSchema = environmentStatusSchema().keys({ configHash: joi.string().required(), - environmentName: joi.string().required(), resolvedAt: joi.date().required(), }) @@ -283,11 +281,6 @@ export class ResolveProviderTask extends BaseTask { return null } - if (cachedStatus.environmentName !== this.garden.environmentName) { - this.log.silly(`Cached environment name at ${cachePath} does not match the current environmnent name`) - return null - } - const configHash = this.hashConfig(config) if (cachedStatus.configHash !== configHash) { @@ -303,7 +296,7 @@ export class ResolveProviderTask extends BaseTask { return null } - return omit(cachedStatus, ["configHash", "resolvedAt", "environmentName"]) + return omit(cachedStatus, ["configHash", "resolvedAt"]) } private async setCachedStatus(config: GenericProviderConfig, status: EnvironmentStatus) { From 86e3293670fe3d094ae71fb03eb5183432e965e5 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 3 Jan 2023 11:21:36 +0100 Subject: [PATCH 3/6] fix: compilation --- plugins/terraform/commands.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/plugins/terraform/commands.ts b/plugins/terraform/commands.ts index fb2a0c3e2d..66ec48f0de 100644 --- a/plugins/terraform/commands.ts +++ b/plugins/terraform/commands.ts @@ -44,8 +44,7 @@ function makeRootCommand(commandName: string) { // Clear the provider status cache, to avoid any user confusion const cachePath = getProviderStatusCachePath({ gardenDirPath: ctx.gardenDirPath, - pluginName: provider.name, - environmentName: ctx.environmentName, + pluginName: provider.name }) await remove(cachePath) From d0032f00518a8c7f26e450926bd26e19d818145d Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 3 Jan 2023 15:52:14 +0100 Subject: [PATCH 4/6] tests: add an integration test co-authored-by: Walther --- core/test/data/exec-provider-cache/.gitignore | 1 + .../exec-provider-cache/project.garden.yml | 8 +++ core/test/integ/src/plugins/exec/resolve.ts | 55 +++++++++++++++++++ 3 files changed, 64 insertions(+) create mode 100644 core/test/data/exec-provider-cache/.gitignore create mode 100644 core/test/data/exec-provider-cache/project.garden.yml create mode 100644 core/test/integ/src/plugins/exec/resolve.ts diff --git a/core/test/data/exec-provider-cache/.gitignore b/core/test/data/exec-provider-cache/.gitignore new file mode 100644 index 0000000000..f8c3214fdc --- /dev/null +++ b/core/test/data/exec-provider-cache/.gitignore @@ -0,0 +1 @@ +theFile diff --git a/core/test/data/exec-provider-cache/project.garden.yml b/core/test/data/exec-provider-cache/project.garden.yml new file mode 100644 index 0000000000..d5117eccd3 --- /dev/null +++ b/core/test/data/exec-provider-cache/project.garden.yml @@ -0,0 +1,8 @@ +kind: Project +name: incident-repro +environments: + - name: one + - name: two +providers: + - name: exec + initScript: "echo '${environment.name}' > theFile" diff --git a/core/test/integ/src/plugins/exec/resolve.ts b/core/test/integ/src/plugins/exec/resolve.ts new file mode 100644 index 0000000000..43f3ebca2b --- /dev/null +++ b/core/test/integ/src/plugins/exec/resolve.ts @@ -0,0 +1,55 @@ +import { expect } from "chai" +import execa from "execa" +import { remove, readFile } from "fs-extra" +import { join } from "node:path" +import { getDataDir, makeTestGarden, TestGarden } from "../../../../helpers" + +describe("exec provider initialization cache behaviour", () => { + let gardenOne: TestGarden + let tmpDir: string + + beforeEach(async () => { + gardenOne = await makeTestGarden(getDataDir("exec-provider-cache"), { environmentName: "one" }) + tmpDir = await gardenOne.getRepoRoot() + }) + + it("writes the environment name to theFile as configured in the initScript", async () => { + await gardenOne.resolveProvider(gardenOne.log, "exec") + + const contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + + expect(contents).equal("one\n") + }) + + it("overwrites theFile when changing environments", async () => { + await gardenOne.resolveProvider(gardenOne.log, "exec") + + let contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + expect(contents).equal("one\n") + + const gardenTwo = await makeTestGarden(tmpDir, { environmentName: "two", noTempDir: true }) + await gardenTwo.resolveProvider(gardenTwo.log, "exec") + + contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + expect(contents).equal("two\n") + }) + + it("still overwrites theFile when changing environments back", async () => { + await gardenOne.resolveProvider(gardenOne.log, "exec") + + let contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + expect(contents).equal("one\n") + + const gardenTwo = await makeTestGarden(tmpDir, { environmentName: "two", noTempDir: true }) + await gardenTwo.resolveProvider(gardenTwo.log, "exec") + + contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + expect(contents).equal("two\n") + + const gardenOneAgain = await makeTestGarden(tmpDir, { environmentName: "one", noTempDir: true }) + await gardenOneAgain.resolveProvider(gardenOneAgain.log, "exec") + + contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + expect(contents).equal("one\n") + }) +}) From f8cbe7abbd8dba1795c0de9eb781fdd86dcfe5e1 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 3 Jan 2023 16:05:55 +0100 Subject: [PATCH 5/6] improvment: satisfy linter --- core/test/integ/src/plugins/exec/resolve.ts | 11 +++++++++-- plugins/terraform/commands.ts | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/core/test/integ/src/plugins/exec/resolve.ts b/core/test/integ/src/plugins/exec/resolve.ts index 43f3ebca2b..901141e0f8 100644 --- a/core/test/integ/src/plugins/exec/resolve.ts +++ b/core/test/integ/src/plugins/exec/resolve.ts @@ -1,6 +1,13 @@ +/* + * Copyright (C) 2018-2022 Garden Technologies, Inc. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + import { expect } from "chai" -import execa from "execa" -import { remove, readFile } from "fs-extra" +import { readFile } from "fs-extra" import { join } from "node:path" import { getDataDir, makeTestGarden, TestGarden } from "../../../../helpers" diff --git a/plugins/terraform/commands.ts b/plugins/terraform/commands.ts index 66ec48f0de..146c8f1799 100644 --- a/plugins/terraform/commands.ts +++ b/plugins/terraform/commands.ts @@ -44,7 +44,7 @@ function makeRootCommand(commandName: string) { // Clear the provider status cache, to avoid any user confusion const cachePath = getProviderStatusCachePath({ gardenDirPath: ctx.gardenDirPath, - pluginName: provider.name + pluginName: provider.name, }) await remove(cachePath) From 8e05e1e704a68266d1dbe75ea8ee553b972c5bd8 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 3 Jan 2023 16:24:40 +0100 Subject: [PATCH 6/6] improvement: reduce code duplication --- core/test/integ/src/plugins/exec/resolve.ts | 25 ++++++++++----------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/core/test/integ/src/plugins/exec/resolve.ts b/core/test/integ/src/plugins/exec/resolve.ts index 901141e0f8..650434df47 100644 --- a/core/test/integ/src/plugins/exec/resolve.ts +++ b/core/test/integ/src/plugins/exec/resolve.ts @@ -14,49 +14,48 @@ import { getDataDir, makeTestGarden, TestGarden } from "../../../../helpers" describe("exec provider initialization cache behaviour", () => { let gardenOne: TestGarden let tmpDir: string + let fileLocation: string beforeEach(async () => { gardenOne = await makeTestGarden(getDataDir("exec-provider-cache"), { environmentName: "one" }) - tmpDir = await gardenOne.getRepoRoot() - }) - it("writes the environment name to theFile as configured in the initScript", async () => { + tmpDir = join(await gardenOne.getRepoRoot(), "project") + fileLocation = join(tmpDir, "theFile") + await gardenOne.resolveProvider(gardenOne.log, "exec") + }) - const contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + it("writes the environment name to theFile as configured in the initScript", async () => { + const contents = await readFile(fileLocation, { encoding: "utf-8" }) expect(contents).equal("one\n") }) it("overwrites theFile when changing environments", async () => { - await gardenOne.resolveProvider(gardenOne.log, "exec") - - let contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + let contents = await readFile(fileLocation, { encoding: "utf-8" }) expect(contents).equal("one\n") const gardenTwo = await makeTestGarden(tmpDir, { environmentName: "two", noTempDir: true }) await gardenTwo.resolveProvider(gardenTwo.log, "exec") - contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + contents = await readFile(fileLocation, { encoding: "utf-8" }) expect(contents).equal("two\n") }) it("still overwrites theFile when changing environments back", async () => { - await gardenOne.resolveProvider(gardenOne.log, "exec") - - let contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + let contents = await readFile(fileLocation, { encoding: "utf-8" }) expect(contents).equal("one\n") const gardenTwo = await makeTestGarden(tmpDir, { environmentName: "two", noTempDir: true }) await gardenTwo.resolveProvider(gardenTwo.log, "exec") - contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + contents = await readFile(fileLocation, { encoding: "utf-8" }) expect(contents).equal("two\n") const gardenOneAgain = await makeTestGarden(tmpDir, { environmentName: "one", noTempDir: true }) await gardenOneAgain.resolveProvider(gardenOneAgain.log, "exec") - contents = await readFile(join(tmpDir, "theFile"), { encoding: "utf-8" }) + contents = await readFile(fileLocation, { encoding: "utf-8" }) expect(contents).equal("one\n") }) })