From 8d66c36c3dea2833c8b10b3daadac1e3fa891653 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Thu, 15 Oct 2020 16:38:27 +0200 Subject: [PATCH] fix(core): some conditional template strings were not resolved correctly This was introduced in v0.12.7 after some changes in template resolution logic. Added tests to catch this, should all work as expected now, but please test to make sure when reviewing. --- core/src/actions.ts | 2 + core/src/commands/deploy.ts | 2 +- core/src/config/config-context.ts | 51 ++++++++--- core/src/garden.ts | 1 + core/src/resolve-module.ts | 3 +- core/src/tasks/deploy.ts | 2 +- core/src/tasks/resolve-module.ts | 5 +- core/src/template-string-parser.pegjs | 12 ++- core/src/template-string.ts | 14 ++- core/src/util/testing.ts | 4 +- core/test/unit/src/config/config-context.ts | 2 + core/test/unit/src/garden.ts | 98 +++++++++++++++++++++ 12 files changed, 168 insertions(+), 28 deletions(-) diff --git a/core/src/actions.ts b/core/src/actions.ts index b32be86dbc..cddaddfbef 100644 --- a/core/src/actions.ts +++ b/core/src/actions.ts @@ -866,6 +866,7 @@ export class ActionRouter implements TypeGuard { parentName: module.parentName, templateName: module.templateName, inputs: module.inputs, + partialRuntimeResolution: false, }) // Set allowPartial=false to ensure all required strings are resolved. @@ -928,6 +929,7 @@ export class ActionRouter implements TypeGuard { parentName: module.parentName, templateName: module.templateName, inputs: module.inputs, + partialRuntimeResolution: false, }) // Set allowPartial=false to ensure all required strings are resolved. diff --git a/core/src/commands/deploy.ts b/core/src/commands/deploy.ts index febfc385db..e228e01a77 100644 --- a/core/src/commands/deploy.ts +++ b/core/src/commands/deploy.ts @@ -118,7 +118,7 @@ export class DeployCommand extends Command { } const initGraph = await garden.getConfigGraph(log) - let services = await initGraph.getServices({ names: args.services, includeDisabled: true }) + let services = initGraph.getServices({ names: args.services, includeDisabled: true }) const disabled = services.filter((s) => s.disabled).map((s) => s.name) diff --git a/core/src/config/config-context.ts b/core/src/config/config-context.ts index fbeebc3bb3..604358d470 100644 --- a/core/src/config/config-context.ts +++ b/core/src/config/config-context.ts @@ -20,7 +20,11 @@ import { } from "./common" import { Provider, GenericProviderConfig, ProviderMap } from "./provider" import { ConfigurationError } from "../exceptions" -import { resolveTemplateString, TemplateStringMissingKeyError } from "../template-string" +import { + resolveTemplateString, + TemplateStringMissingKeyException, + TemplateStringPassthroughException, +} from "../template-string" import { Garden } from "../garden" import { joi } from "../config/common" import { KeyedSet } from "../util/keyed-set" @@ -64,9 +68,13 @@ export abstract class ConfigContext { private readonly _rootContext: ConfigContext private readonly _resolvedValues: { [path: string]: string } + // This is used for special-casing e.g. runtime.* resolution + protected _alwaysAllowPartial: boolean + constructor(rootContext?: ConfigContext) { this._rootContext = rootContext || this this._resolvedValues = {} + this._alwaysAllowPartial = false } static getSchema() { @@ -185,10 +193,18 @@ export abstract class ConfigContext { } } - if (opts.allowPartial) { - // If we're allowing partial strings, we throw the error immediately to end the resolution flow. The error - // is caught in the surrounding template resolution code. - throw new TemplateStringMissingKeyError(message, { + // If we're allowing partial strings, we throw the error immediately to end the resolution flow. The error + // is caught in the surrounding template resolution code. + if (this._alwaysAllowPartial) { + // We use a separate exception type when contexts are specifically indicating that unresolvable keys should + // be passed through. This is caught in the template parser code. + throw new TemplateStringPassthroughException(message, { + nodePath, + fullPath, + opts, + }) + } else if (opts.allowPartial) { + throw new TemplateStringMissingKeyException(message, { nodePath, fullPath, opts, @@ -716,21 +732,25 @@ class RuntimeConfigContext extends ConfigContext { ) public tasks: Map - constructor(root: ConfigContext, runtimeContext?: RuntimeContext) { + constructor(root: ConfigContext, allowPartial: boolean, runtimeContext?: RuntimeContext) { super(root) this.services = new Map() this.tasks = new Map() - const dependencies = runtimeContext ? runtimeContext.dependencies : [] - - for (const dep of dependencies) { - if (dep.type === "service") { - this.services.set(dep.name, new ServiceRuntimeContext(this, dep.outputs)) - } else if (dep.type === "task") { - this.tasks.set(dep.name, new TaskRuntimeContext(this, dep.outputs)) + if (runtimeContext) { + for (const dep of runtimeContext.dependencies) { + if (dep.type === "service") { + this.services.set(dep.name, new ServiceRuntimeContext(this, dep.outputs)) + } else if (dep.type === "task") { + this.tasks.set(dep.name, new TaskRuntimeContext(this, dep.outputs)) + } } } + + // This ensures that any template string containing runtime.* references is returned unchanged when + // there is no or limited runtimeContext available. + this._alwaysAllowPartial = allowPartial } } @@ -843,6 +863,7 @@ export class ModuleConfigContext extends ProviderConfigContext { parentName, templateName, inputs, + partialRuntimeResolution, }: { garden: Garden resolvedProviders: ProviderMap @@ -854,6 +875,7 @@ export class ModuleConfigContext extends ProviderConfigContext { parentName: string | undefined templateName: string | undefined inputs: DeepPrimitiveMap | undefined + partialRuntimeResolution: boolean }) { super(garden, resolvedProviders) @@ -866,7 +888,7 @@ export class ModuleConfigContext extends ProviderConfigContext { this.modules.set(moduleName, new ErrorContext(`Module ${chalk.white.bold(moduleName)} cannot reference itself.`)) } - this.runtime = new RuntimeConfigContext(this, runtimeContext) + this.runtime = new RuntimeConfigContext(this, partialRuntimeResolution, runtimeContext) if (parentName && templateName) { this.parent = new ParentContext(this, parentName) this.template = new ModuleTemplateContext(this, templateName) @@ -898,6 +920,7 @@ export class OutputConfigContext extends ModuleConfigContext { parentName: undefined, templateName: undefined, inputs: {}, + partialRuntimeResolution: false, }) } } diff --git a/core/src/garden.ts b/core/src/garden.ts index d374c3a368..dee804c215 100644 --- a/core/src/garden.ts +++ b/core/src/garden.ts @@ -843,6 +843,7 @@ export class Garden { parentName: undefined, templateName: undefined, inputs: {}, + partialRuntimeResolution: true, }) // Resolve modules from specs and add to the list diff --git a/core/src/resolve-module.ts b/core/src/resolve-module.ts index de296f44d8..8aa5289d1a 100644 --- a/core/src/resolve-module.ts +++ b/core/src/resolve-module.ts @@ -29,8 +29,7 @@ export const resolveModuleConfig = profileAsync(async function $resolveModuleCon config: ModuleConfig, opts: ModuleConfigResolveOpts ): Promise { - // Allowing partial resolution here, to defer runtime remplate resolution - config = resolveTemplateStrings(cloneDeep(config), opts.configContext, { allowPartial: true, ...opts }) + config = resolveTemplateStrings(cloneDeep(config), opts.configContext, opts) const moduleTypeDefinitions = await garden.getModuleTypes() const description = moduleTypeDefinitions[config.type] diff --git a/core/src/tasks/deploy.ts b/core/src/tasks/deploy.ts index 422261a9cc..c2bb1614f2 100644 --- a/core/src/tasks/deploy.ts +++ b/core/src/tasks/deploy.ts @@ -67,7 +67,7 @@ export class DeployTask extends BaseTask { const dg = this.graph // We filter out service dependencies on services configured for hot reloading (if any) - const deps = await dg.getDependencies({ + const deps = dg.getDependencies({ nodeType: "deploy", name: this.getName(), recursive: false, diff --git a/core/src/tasks/resolve-module.ts b/core/src/tasks/resolve-module.ts index 28726bb2c2..2a0642b3ec 100644 --- a/core/src/tasks/resolve-module.ts +++ b/core/src/tasks/resolve-module.ts @@ -66,6 +66,7 @@ export class ResolveModuleConfigTask extends BaseTask { parentName: this.moduleConfig.parentName, templateName: this.moduleConfig.templateName, inputs: this.moduleConfig.inputs, + partialRuntimeResolution: true, }) const templateRefs = getModuleTemplateReferences(this.moduleConfig, configContext) @@ -116,10 +117,11 @@ export class ResolveModuleConfigTask extends BaseTask { parentName: this.moduleConfig.parentName, templateName: this.moduleConfig.templateName, inputs: this.moduleConfig.inputs, + partialRuntimeResolution: true, }) return resolveModuleConfig(this.garden, this.moduleConfig, { - allowPartial: true, + allowPartial: false, configContext, }) } @@ -221,6 +223,7 @@ export class ResolveModuleTask extends BaseTask { parentName: this.moduleConfig.parentName, templateName: this.moduleConfig.templateName, inputs: this.moduleConfig.inputs, + partialRuntimeResolution: true, }) await Bluebird.map(resolvedConfig.generateFiles || [], async (fileSpec) => { diff --git a/core/src/template-string-parser.pegjs b/core/src/template-string-parser.pegjs index 6d7ac4c606..cfbb3223b4 100644 --- a/core/src/template-string-parser.pegjs +++ b/core/src/template-string-parser.pegjs @@ -15,7 +15,8 @@ isArray, isPrimitive, optionalSuffix, - missingKeyErrorType, + missingKeyExceptionType, + passthroughExceptionType, resolveNested, TemplateStringError, } = options @@ -29,7 +30,8 @@ TemplateString FormatString = FormatStart e:Expression end:FormatEnd { - if (e && e._error && e._error.type !== missingKeyErrorType) { + // Any unexpected error is returned immediately. Certain exceptions have special semantics that are caught below. + if (e && e._error && e._error.type !== missingKeyExceptionType && e._error.type !== passthroughExceptionType) { return e } @@ -37,7 +39,11 @@ FormatString const allowUndefined = end[1] === optionalSuffix if (getValue(e) === undefined) { - if (allowUndefined) { + if (e && e._error && e._error.type === passthroughExceptionType) { + // We allow certain configuration contexts (e.g. placeholders for runtime.*) to indicate that a template + // string should be returned partially resolved even if allowPartial=false. + return text() + } else if (allowUndefined) { if (e && e._error) { return { ...e, _error: undefined } } else { diff --git a/core/src/template-string.ts b/core/src/template-string.ts index 2643108ea3..80e7dc8e08 100644 --- a/core/src/template-string.ts +++ b/core/src/template-string.ts @@ -24,14 +24,19 @@ import { ObjectWithName } from "./util/util" export type StringOrStringPromise = Promise | string -const missingKeyErrorType = "template-string-missing-key" +const missingKeyExceptionType = "template-string-missing-key" +const passthroughExceptionType = "template-string-passthrough" class TemplateStringError extends GardenBaseError { type = "template-string" } -export class TemplateStringMissingKeyError extends GardenBaseError { - type = missingKeyErrorType +export class TemplateStringMissingKeyException extends GardenBaseError { + type = missingKeyExceptionType +} + +export class TemplateStringPassthroughException extends GardenBaseError { + type = passthroughExceptionType } let _parser: any @@ -76,7 +81,8 @@ export function resolveTemplateString(string: string, context: ConfigContext, op isArray, ConfigurationError, TemplateStringError, - missingKeyErrorType, + missingKeyExceptionType, + passthroughExceptionType, allowPartial: !!opts.allowPartial, optionalSuffix: "}?", isPrimitive, diff --git a/core/src/util/testing.ts b/core/src/util/testing.ts index 06270cd314..b4540c06f8 100644 --- a/core/src/util/testing.ts +++ b/core/src/util/testing.ts @@ -109,8 +109,8 @@ export class TestGarden extends Garden { * Helper to get a single module. We don't put this on the Garden class because it is highly inefficient * and not advisable except for testing. */ - async resolveModule(name: string) { - const modules = await this.resolveModules({ log: this.log }) + async resolveModule(name: string, runtimeContext?: RuntimeContext) { + const modules = await this.resolveModules({ log: this.log, runtimeContext }) const config = findByName(modules, name) if (!config) { diff --git a/core/test/unit/src/config/config-context.ts b/core/test/unit/src/config/config-context.ts index 8b5630abd6..b456cdc9ab 100644 --- a/core/test/unit/src/config/config-context.ts +++ b/core/test/unit/src/config/config-context.ts @@ -408,6 +408,7 @@ describe("ModuleConfigContext", () => { parentName: undefined, templateName: undefined, inputs: {}, + partialRuntimeResolution: false, }) }) @@ -517,6 +518,7 @@ describe("ModuleConfigContext", () => { parentName: undefined, templateName: undefined, inputs: {}, + partialRuntimeResolution: false, }) }) diff --git a/core/test/unit/src/garden.ts b/core/test/unit/src/garden.ts index bbc09848ff..0f97a0a6b6 100644 --- a/core/test/unit/src/garden.ts +++ b/core/test/unit/src/garden.ts @@ -2560,6 +2560,104 @@ describe("Garden", () => { expect(module.spec.bla).to.eql({ nested: { key: "my value" } }) }) + it("should pass through runtime template strings when no runtimeContext is provider", async () => { + const test = createGardenPlugin({ + name: "test", + createModuleTypes: [ + { + name: "test", + docs: "test", + schema: joi.object().keys({ bla: joi.string() }), + handlers: {}, + }, + ], + }) + + const garden = await TestGarden.factory(pathFoo, { + plugins: [test], + config: { + apiVersion: DEFAULT_API_VERSION, + kind: "Project", + name: "test", + path: pathFoo, + defaultEnvironment: "default", + dotIgnoreFiles: [], + environments: [{ name: "default", defaultNamespace, variables: {} }], + providers: [{ name: "test" }], + variables: {}, + }, + }) + + garden.setModuleConfigs([ + { + apiVersion: DEFAULT_API_VERSION, + name: "module-a", + type: "test", + allowPublish: false, + build: { dependencies: [] }, + disabled: false, + path: pathFoo, + serviceConfigs: [], + taskConfigs: [], + testConfigs: [], + spec: { bla: "${runtime.services.foo.bar || 'default'}" }, + }, + ]) + + const module = await garden.resolveModule("module-a") + + expect(module.spec.bla).to.equal("${runtime.services.foo.bar || 'default'}") + }) + + it("should resolve conditional strings with missing variables", async () => { + const test = createGardenPlugin({ + name: "test", + createModuleTypes: [ + { + name: "test", + docs: "test", + schema: joi.object().keys({ bla: joi.string() }), + handlers: {}, + }, + ], + }) + + const garden = await TestGarden.factory(pathFoo, { + plugins: [test], + config: { + apiVersion: DEFAULT_API_VERSION, + kind: "Project", + name: "test", + path: pathFoo, + defaultEnvironment: "default", + dotIgnoreFiles: [], + environments: [{ name: "default", defaultNamespace, variables: {} }], + providers: [{ name: "test" }], + variables: {}, + }, + }) + + garden.setModuleConfigs([ + { + apiVersion: DEFAULT_API_VERSION, + name: "module-a", + type: "test", + allowPublish: false, + build: { dependencies: [] }, + disabled: false, + path: pathFoo, + serviceConfigs: [], + taskConfigs: [], + testConfigs: [], + spec: { bla: "${var.foo || 'default'}" }, + }, + ]) + + const module = await garden.resolveModule("module-a") + + expect(module.spec.bla).to.equal("default") + }) + it("should correctly resolve template strings with $merge keys", async () => { const test = createGardenPlugin({ name: "test",