From 31b2936f96ead7c5e8266afe021ac7feb4f307ab Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Mon, 25 Mar 2019 18:59:07 +0100 Subject: [PATCH] fix: include resolved config in module version Module versions now also include a serialized version of the resolved module config (hashed together with versions / dirty timestamps). This fixes an issue where changes to project variables used in a module's config would not result in the module's version changing when recomputed. This was the case because the version was based only on the latest commit and dirty timestamp of the module in question, and those of its dependencies, which doesn't factor in changes to project-level variables. --- garden-service/package-lock.json | 4 +- garden-service/package.json | 1 + garden-service/src/config/module.ts | 5 + garden-service/src/vcs/base.ts | 104 +++++------ .../garden.yml | 9 + .../module-a/garden.yml | 8 + .../module-b/garden.yml | 8 + garden-service/test/unit/src/vcs/base.ts | 162 ++++++++++++++++-- 8 files changed, 218 insertions(+), 83 deletions(-) create mode 100644 garden-service/test/unit/data/test-project-variable-versioning/garden.yml create mode 100644 garden-service/test/unit/data/test-project-variable-versioning/module-a/garden.yml create mode 100644 garden-service/test/unit/data/test-project-variable-versioning/module-b/garden.yml diff --git a/garden-service/package-lock.json b/garden-service/package-lock.json index 21b4d914ac..e0fc942bae 100644 --- a/garden-service/package-lock.json +++ b/garden-service/package-lock.json @@ -6016,7 +6016,6 @@ "version": "1.0.1", "resolved": "https://registry.npmjs.org/json-stable-stringify/-/json-stable-stringify-1.0.1.tgz", "integrity": "sha1-mnWdOcXy/1A/1TAGRu1EX4jE+a8=", - "dev": true, "requires": { "jsonify": "~0.0.0" } @@ -6037,8 +6036,7 @@ "jsonify": { "version": "0.0.0", "resolved": "https://registry.npmjs.org/jsonify/-/jsonify-0.0.0.tgz", - "integrity": "sha1-LHS27kHZPKUbe1qu6PUDYx0lKnM=", - "dev": true + "integrity": "sha1-LHS27kHZPKUbe1qu6PUDYx0lKnM=" }, "jsonparse": { "version": "1.3.1", diff --git a/garden-service/package.json b/garden-service/package.json index 523e124003..b1a68f133d 100644 --- a/garden-service/package.json +++ b/garden-service/package.json @@ -58,6 +58,7 @@ "joi": "^14.3.0", "js-yaml": "^3.12.0", "json-merge-patch": "^0.2.3", + "json-stable-stringify": "^1.0.1", "json-stringify-safe": "^5.0.1", "klaw": "^3.0.0", "koa": "^2.6.2", diff --git a/garden-service/src/config/module.ts b/garden-service/src/config/module.ts index a2967a3159..3c3bd38ecd 100644 --- a/garden-service/src/config/module.ts +++ b/garden-service/src/config/module.ts @@ -7,6 +7,7 @@ */ import dedent = require("dedent") +import stableStringify = require("json-stable-stringify") import * as Joi from "joi" import { ServiceConfig, ServiceSpec } from "./service" import { @@ -146,3 +147,7 @@ export const moduleConfigSchema = baseModuleSpecSchema .description("The module spec, as defined by the provider plugin."), }) .description("The configuration for a module.") + +export function serializeConfig(moduleConfig: ModuleConfig) { + return stableStringify(moduleConfig) +} diff --git a/garden-service/src/vcs/base.ts b/garden-service/src/vcs/base.ts index 4509be9bc9..ccbaa28eef 100644 --- a/garden-service/src/vcs/base.ts +++ b/garden-service/src/vcs/base.ts @@ -7,7 +7,7 @@ */ import * as Bluebird from "bluebird" -import { mapValues, keyBy, sortBy, orderBy, omit } from "lodash" +import { mapValues, keyBy, last, sortBy, omit } from "lodash" import { createHash } from "crypto" import * as Joi from "joi" import { validate } from "../config/common" @@ -20,7 +20,7 @@ import { getRemoteSourcesDirname, getRemoteSourcePath, } from "../util/ext-source-util" -import { ModuleConfig } from "../config/module" +import { ModuleConfig, serializeConfig } from "../config/module" import { LogNode } from "../logger/log-node" export const NEW_MODULE_VERSION = "0000000000" @@ -128,8 +128,12 @@ export abstract class VcsHandler { }) if (dependencies.length === 0) { + const versionString = getVersionString( + moduleConfig, + [{ ...treeVersion, name: moduleConfig.name }], + treeVersion.dirtyTimestamp) return { - versionString: getVersionString(treeVersion), + versionString, dirtyTimestamp: treeVersion.dirtyTimestamp, dependencyVersions: {}, } @@ -142,51 +146,14 @@ export abstract class VcsHandler { const dependencyVersions = mapValues(keyBy(namedDependencyVersions, "name"), v => omit(v, "name")) // keep the module at the top of the chain, dependencies sorted by name - const sortedDependencies = sortBy(namedDependencyVersions, "name") - const allVersions: NamedTreeVersion[] = [{ name: moduleConfig.name, ...treeVersion }].concat(sortedDependencies) - - const dirtyVersions = allVersions.filter(v => !!v.dirtyTimestamp) - - if (dirtyVersions.length > 0) { - // if any modules are dirty, we resolve with the one(s) with the most recent timestamp - const latestDirty: NamedTreeVersion[] = [] - - for (const v of orderBy(dirtyVersions, "dirtyTimestamp", "desc")) { - if (latestDirty.length === 0 || v.dirtyTimestamp === latestDirty[0].dirtyTimestamp) { - latestDirty.push(v) - } else { - break - } - } - - const dirtyTimestamp = latestDirty[0].dirtyTimestamp - - if (latestDirty.length > 1) { - // if the last modified timestamp is common across multiple modules, hash their versions - const versionString = addVersionPrefix(`${hashVersions(latestDirty)}-${dirtyTimestamp}`) - - return { - versionString, - dirtyTimestamp, - dependencyVersions, - } - } else { - // if there's just one module that was most recently modified, return that version - return { - versionString: getVersionString(latestDirty[0]), - dirtyTimestamp, - dependencyVersions, - } - } - } else { - // otherwise derive the version from all the modules - const versionString = addVersionPrefix(hashVersions(allVersions)) + const allVersions: NamedTreeVersion[] = [{ name: moduleConfig.name, ...treeVersion }] + .concat(namedDependencyVersions) + const dirtyTimestamp = getLatestDirty(allVersions) - return { - versionString, - dirtyTimestamp: null, - dependencyVersions, - } + return { + dirtyTimestamp, + dependencyVersions, + versionString: getVersionString(moduleConfig, allVersions, dirtyTimestamp), } } @@ -199,12 +166,6 @@ export abstract class VcsHandler { } } -function hashVersions(versions: NamedTreeVersion[]) { - const versionHash = createHash("sha256") - versionHash.update(versions.map(v => `${v.name}_${v.latestCommit}`).join(".")) - return versionHash.digest("hex").slice(0, 10) -} - async function readVersionFile(path: string, schema): Promise { if (!(await pathExists(path))) { return null @@ -247,18 +208,37 @@ export async function writeModuleVersionFile(path: string, version: ModuleVersio await writeFile(path, JSON.stringify(version)) } -export function getVersionString(treeVersion: TreeVersion) { - const rawVersion = treeVersion.dirtyTimestamp - ? `${treeVersion.latestCommit}-${treeVersion.dirtyTimestamp}` - : treeVersion.latestCommit - return addVersionPrefix(rawVersion) -} - /** * We prefix with "v-" to prevent this.version from being read as a number when only a prefix of the * commit hash is used, and that prefix consists of only numbers. This can cause errors in certain contexts * when the version string is used in template variables in configuration files. */ -export function addVersionPrefix(versionString: string) { - return `v-${versionString}` +export function getVersionString( + moduleConfig: ModuleConfig, treeVersions: NamedTreeVersion[], dirtyTimestamp: number | null, +) { + const hashed = `v-${hashVersions(moduleConfig, treeVersions)}` + return dirtyTimestamp ? `${hashed}-${dirtyTimestamp}` : hashed +} + +/** + * Returns the latest (i.e. numerically largest) dirty timestamp found in versions, or null if none of versions + * has a dirty timestamp. + */ +export function getLatestDirty(versions: TreeVersion[]): number | null { + const latest = last(sortBy( + versions.filter(v => !!v.dirtyTimestamp), v => v.dirtyTimestamp) + .map(v => v.dirtyTimestamp)) + return latest || null +} + +/** + * The versions argument should consist of moduleConfig's tree version, and the tree versions of its dependencies. + */ +export function hashVersions(moduleConfig: ModuleConfig, versions: NamedTreeVersion[]) { + const versionHash = createHash("sha256") + const configString = serializeConfig(moduleConfig) + const versionStrings = sortBy(versions, "name") + .map(v => `${v.name}_${v.latestCommit}`) + versionHash.update([configString, ...versionStrings].join(".")) + return versionHash.digest("hex").slice(0, 10) } diff --git a/garden-service/test/unit/data/test-project-variable-versioning/garden.yml b/garden-service/test/unit/data/test-project-variable-versioning/garden.yml new file mode 100644 index 0000000000..2498d72baf --- /dev/null +++ b/garden-service/test/unit/data/test-project-variable-versioning/garden.yml @@ -0,0 +1,9 @@ +project: + name: test-project-variable-versioning + environmentDefaults: + variables: + echo-string: OK + environments: + - name: local + providers: + - name: test-plugin diff --git a/garden-service/test/unit/data/test-project-variable-versioning/module-a/garden.yml b/garden-service/test/unit/data/test-project-variable-versioning/module-a/garden.yml new file mode 100644 index 0000000000..21ad52cfa3 --- /dev/null +++ b/garden-service/test/unit/data/test-project-variable-versioning/module-a/garden.yml @@ -0,0 +1,8 @@ +module: + name: module-a + type: test + build: + command: [echo, "${variables.echo-string}"] + tests: + - name: unit + command: [echo, OK] diff --git a/garden-service/test/unit/data/test-project-variable-versioning/module-b/garden.yml b/garden-service/test/unit/data/test-project-variable-versioning/module-b/garden.yml new file mode 100644 index 0000000000..35cdb69f72 --- /dev/null +++ b/garden-service/test/unit/data/test-project-variable-versioning/module-b/garden.yml @@ -0,0 +1,8 @@ +module: + name: module-b + type: test + build: + command: [echo, beans] + tests: + - name: unit + command: [echo, OK] diff --git a/garden-service/test/unit/src/vcs/base.ts b/garden-service/test/unit/src/vcs/base.ts index acd415c2e3..af9d15342b 100644 --- a/garden-service/test/unit/src/vcs/base.ts +++ b/garden-service/test/unit/src/vcs/base.ts @@ -1,7 +1,16 @@ -import { VcsHandler, NEW_MODULE_VERSION, TreeVersions, TreeVersion } from "../../../../src/vcs/base" -import { projectRootA, makeTestGardenA } from "../../../helpers" +import { + VcsHandler, + NEW_MODULE_VERSION, + TreeVersions, + TreeVersion, + getVersionString, + getLatestDirty, +} from "../../../../src/vcs/base" +import { projectRootA, makeTestGardenA, makeTestGarden, getDataDir } from "../../../helpers" import { expect } from "chai" +import { cloneDeep } from "lodash" import { Garden } from "../../../../src/garden" +import { ModuleConfigContext } from "../../../../src/config/config-context" class TestVcsHandler extends VcsHandler { name = "test" @@ -75,14 +84,108 @@ describe("VcsHandler", () => { }) }) + describe("getVersionString", () => { + let moduleABefore + let moduleAAfter + let moduleBBefore + let moduleBAfter + + before(async () => { + const templateGarden = await makeTestGarden(getDataDir("test-project-variable-versioning")) + + moduleABefore = await templateGarden.resolveModuleConfig("module-a") // uses the echo-string variable + moduleBBefore = await templateGarden.resolveModuleConfig("module-b") // does not use the echo-string variable + + const moduleAAfterEnv = cloneDeep(templateGarden.environment) + moduleAAfterEnv.variables["echo-string"] = "something else" + const changedModuleConfigContext = new ModuleConfigContext( + templateGarden, moduleAAfterEnv, await templateGarden.getRawModuleConfigs()) + + moduleAAfter = await templateGarden.resolveModuleConfig("module-a", changedModuleConfigContext) + moduleBAfter = await templateGarden.resolveModuleConfig("module-b", changedModuleConfigContext) + }) + + it("should return a different version for a module when a variable used by it changes", async () => { + const moduleABeforeVersion = getVersionString(moduleABefore, [], null) + const moduleAAfterVersion = getVersionString(moduleAAfter, [], null) + + expect(moduleABeforeVersion).to.not.eql(moduleAAfterVersion) + }) + + it("should return the same version for a module when a variable not used by it changes", async () => { + const moduleBBeforeVersion = getVersionString(moduleBBefore, [], null) + const moduleBAfterVersion = getVersionString(moduleBAfter, [], null) + + expect(moduleBBeforeVersion).to.eql(moduleBAfterVersion) + }) + + }) + + context("internal helpers", () => { + + const namedVersionA = { + name: "module-a", + latestCommit: "qwerty", + dirtyTimestamp: null, + } + + const namedVersionB = { + name: "module-b", + latestCommit: "qwerty", + dirtyTimestamp: 123, + } + + const namedVersionC = { + name: "module-c", + latestCommit: "qwerty", + dirtyTimestamp: 456, + } + + const namedVersions = [namedVersionA, namedVersionB, namedVersionC] + + describe("hashVersions", () => { + + it("is stable with respect to key order in moduleConfig", async () => { + const originalConfig = await garden.resolveModuleConfig("module-a") + const stirredConfig = cloneDeep(originalConfig) + delete stirredConfig.name + stirredConfig.name = originalConfig.name + + expect(getVersionString(originalConfig, namedVersions, null)) + .to.eql(getVersionString(stirredConfig, namedVersions, null)) + }) + + it("is stable with respect to named version order", async () => { + const config = await garden.resolveModuleConfig("module-a") + + expect(getVersionString(config, [namedVersionA, namedVersionB, namedVersionC], null)) + .to.eql(getVersionString(config, [namedVersionB, namedVersionA, namedVersionC], null)) + }) + + }) + + describe("getLatestDirty", () => { + + it("returns the latest dirty timestamp if one or more versions provided have one", () => { + expect(getLatestDirty(namedVersions)).to.eql(456) + }) + + it("returns null if none of the versions provided has a dirty has one", () => { + expect(getLatestDirty([namedVersionA])).to.eql(null) + }) + + }) + + }) + describe("resolveVersion", () => { + it("should return module version if there are no dependencies", async () => { const module = await garden.resolveModuleConfig("module-a") - const result = await handler.resolveVersion(module, []) expect(result).to.eql({ - versionString: `v-${versionA.latestCommit}`, + versionString: getVersionString(module, [{ ...versionA, name: "module-a" }], null), dirtyTimestamp: null, dependencyVersions: {}, }) @@ -91,9 +194,10 @@ describe("VcsHandler", () => { it("should return module version if there are no dependencies and properly handle a dirty timestamp", async () => { const module = await garden.resolveModuleConfig("module-b") const latestCommit = "abcdef" + const dirtyTimestamp = 1234 const version = { latestCommit, - dirtyTimestamp: 1234, + dirtyTimestamp, } handler.setTestVersion(module.path, version) @@ -101,8 +205,8 @@ describe("VcsHandler", () => { const result = await handler.resolveVersion(module, []) expect(result).to.eql({ - versionString: "v-abcdef-1234", - dirtyTimestamp: 1234, + dirtyTimestamp, + versionString: getVersionString(module, [{ ...version, name: "module-b" }], dirtyTimestamp), dependencyVersions: {}, }) }) @@ -110,6 +214,8 @@ describe("VcsHandler", () => { it("should return the dirty version if there is a single one", async () => { const [moduleA, moduleB, moduleC] = await garden.resolveModuleConfigs(["module-a", "module-b", "module-c"]) + const dirtyTimestamp = 123 + const versionB = { latestCommit: "qwerty", dirtyTimestamp: null, @@ -118,14 +224,18 @@ describe("VcsHandler", () => { const versionStringC = "asdfgh" const versionC = { + dirtyTimestamp, latestCommit: versionStringC, - dirtyTimestamp: 123, } handler.setTestVersion(moduleC.path, versionC) expect(await handler.resolveVersion(moduleC, [moduleA, moduleB])).to.eql({ - versionString: "v-asdfgh-123", - dirtyTimestamp: 123, + dirtyTimestamp, + versionString: getVersionString(moduleC, [ + { ...versionA, name: "module-a" }, + { ...versionB, name: "module-b" }, + { ...versionC, name: "module-c" }, + ], 123), dependencyVersions: { "module-a": versionA, "module-b": versionB, @@ -136,9 +246,11 @@ describe("VcsHandler", () => { it("should return the latest dirty version if there are multiple", async () => { const [moduleA, moduleB, moduleC] = await garden.resolveModuleConfigs(["module-a", "module-b", "module-c"]) + const latestDirty = 456 + const versionB = { latestCommit: "qwerty", - dirtyTimestamp: 456, + dirtyTimestamp: latestDirty, } handler.setTestVersion(moduleB.path, versionB) @@ -150,8 +262,12 @@ describe("VcsHandler", () => { handler.setTestVersion(moduleC.path, versionC) expect(await handler.resolveVersion(moduleC, [moduleA, moduleB])).to.eql({ - versionString: "v-qwerty-456", - dirtyTimestamp: 456, + versionString: getVersionString(moduleC, [ + { ...versionA, name: "module-a" }, + { ...versionB, name: "module-b" }, + { ...versionC, name: "module-c" }, + ], latestDirty), + dirtyTimestamp: latestDirty, dependencyVersions: { "module-a": versionA, "module-b": versionB, @@ -177,7 +293,11 @@ describe("VcsHandler", () => { handler.setTestVersion(moduleC.path, versionC) expect(await handler.resolveVersion(moduleC, [moduleA, moduleB])).to.eql({ - versionString: "v-5ff3a146d9", + versionString: getVersionString(moduleC, [ + { ...versionA, name: "module-a" }, + { ...versionB, name: "module-b" }, + { ...versionC, name: "module-c" }, + ], null), dirtyTimestamp: null, dependencyVersions: { "module-a": versionA, @@ -192,23 +312,29 @@ describe("VcsHandler", () => { const [moduleA, moduleB, moduleC] = await garden.resolveModuleConfigs(["module-a", "module-b", "module-c"]) + const dirtyTimestamp = 1234 + const versionStringB = "qwerty" const versionB = { + dirtyTimestamp, latestCommit: versionStringB, - dirtyTimestamp: 1234, } handler.setTestVersion(moduleB.path, versionB) const versionStringC = "asdfgh" const versionC = { + dirtyTimestamp, latestCommit: versionStringC, - dirtyTimestamp: 1234, } handler.setTestVersion(moduleC.path, versionC) expect(await handler.resolveVersion(moduleC, [moduleA, moduleB])).to.eql({ - versionString: "v-cfa6d28ec5-1234", - dirtyTimestamp: 1234, + versionString: getVersionString(moduleC, [ + { ...versionA, name: "module-a" }, + { ...versionB, name: "module-b" }, + { ...versionC, name: "module-c" }, + ], dirtyTimestamp), + dirtyTimestamp, dependencyVersions: { "module-a": versionA, "module-b": versionB,