Skip to content

Commit

Permalink
fix(config): recursion error with invalid template strings
Browse files Browse the repository at this point in the history 
BREAKING CHANGE:

This removes the previously deprecated ability to do nested formatting
strings. It's not a helpful feature for most cases, and just complicates
the parser logic. Also wasn't documented really, so it should be safe to
remove without a minor version bump.
  • Loading branch information
@edvald @thsig
edvald authored and thsig committed Apr 30, 2019
1 parent e669408 commit 0cbcb98
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 64 deletions.
20 changes: 18 additions & 2 deletions garden-service/src/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,8 @@ export class ActionHelper implements TypeGuard {
//===========================================================================

async getStatus({ log, serviceNames }: { log: LogEntry, serviceNames?: string[] }): Promise<EnvironmentStatus> {
log.verbose(`Getting environment status (${this.garden.projectName})`)

const envStatus: EnvironmentStatusMap = await this.getEnvironmentStatus({ log })
const graph = await this.garden.getConfigGraph()
const services = keyBy(await graph.getServices(serviceNames), "name")
Expand Down Expand Up @@ -450,7 +452,10 @@ export class ActionHelper implements TypeGuard {
{ params: ModuleActionHelperParams<ModuleActionParams[T]>, actionType: T, defaultHandler?: ModuleActions[T] },
): Promise<ModuleActionOutputs[T]> {
// the type system is messing me up here, not sure why I need the any cast... - j.e.
const { module, pluginName } = <any>params
const { module, pluginName, log } = <any>params

log.verbose(`Getting ${actionType} handler for module ${module.name}`)

const handler = await this.getModuleActionHandler({
moduleType: module.type,
actionType,
Expand All @@ -463,6 +468,9 @@ export class ActionHelper implements TypeGuard {
...<object>params,
module: omit(module, ["_ConfigType"]),
}

log.verbose(`Calling ${actionType} handler for module ${module.name}`)

// TODO: figure out why this doesn't compile without the function cast
return (<Function>handler)(handlerParams)
}
Expand All @@ -474,6 +482,8 @@ export class ActionHelper implements TypeGuard {
const { log, service, runtimeContext } = <any>params
const module = service.module

log.verbose(`Getting ${actionType} handler for service ${service.name}`)

const handler = await this.getModuleActionHandler({
moduleType: module.type,
actionType,
Expand All @@ -488,6 +498,8 @@ export class ActionHelper implements TypeGuard {
runtimeContext,
}

log.verbose(`Calling ${actionType} handler for service ${service.name}`)

return (<Function>handler)(handlerParams)
}

Expand All @@ -499,9 +511,11 @@ export class ActionHelper implements TypeGuard {
},
): Promise<TaskActionOutputs[T]> {

const { task } = <any>params
const { task, log } = <any>params
const module = task.module

log.verbose(`Getting ${actionType} handler for task ${module.name}.${task.name}`)

const handler = await this.getModuleActionHandler({
moduleType: module.type,
actionType,
Expand All @@ -516,6 +530,8 @@ export class ActionHelper implements TypeGuard {
task,
}

log.verbose(`Calling ${actionType} handler for task ${module.name}.${task.name}`)

return (<Function>handler)(handlerParams)
}

Expand Down
8 changes: 7 additions & 1 deletion garden-service/src/plugins/kubernetes/status.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ export async function compareDeployedObjects(
!err.detail || !err.detail.result
|| (!!err.detail.result.stderr && err.detail.result.stderr.trim() !== "exit status 1")
) {
log.verbose(`kubectl diff failed: ${err.message}`)
log.verbose(`kubectl diff failed: ${err.message}\n${err.stderr}`)
} else {
log.verbose(`kubectl diff indicates one or more resources are outdated.`)
log.silly(err.detail.result.stdout)
Expand All @@ -489,6 +489,8 @@ export async function compareDeployedObjects(

// Using kubectl diff didn't work, so we fall back to our own comparison check, which works in _most_ cases,
// but doesn't exhaustively handle normalization issues.
log.verbose(`Getting currently deployed resources...`)

const deployedObjectStatuses: WorkloadStatus[] = await Bluebird.map(
deployedObjects,
async (obj) => checkResourceStatus(api, namespace, obj, log, undefined))
Expand All @@ -509,6 +511,8 @@ export async function compareDeployedObjects(
return result
}

log.verbose(`Comparing expected and deployed resources...`)

for (let [newSpec, existingSpec] of zip(resources, deployedObjects) as KubernetesResource[][]) {
// the API version may implicitly change when deploying
existingSpec.apiVersion = newSpec.apiVersion
Expand Down Expand Up @@ -561,6 +565,8 @@ export async function compareDeployedObjects(
}
}

log.verbose(`All resources match. Environment is ready.`)

result.state = "ready"
return result
}
Expand Down
13 changes: 0 additions & 13 deletions garden-service/src/template-string-parser.pegjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,6 @@ TemplateString
/ InvalidFormatString
/ $(.*) { return [text()] }

NestedTemplateString
= a:(FormatString)+ b:NestedTemplateString? {
return [...a, ...(b || [])]
}
/ a:Prefix b:(FormatString)+ c:NestedTemplateString? {
return [a, ...b, ...(c || [])]
}
/ InvalidFormatString
/ Suffix { return [text()] }

FormatString
= FormatStart key:Key FormatEnd {
return options.getKey(key)
Expand All @@ -45,9 +35,6 @@ FormatString
/ FormatStart a:StringLiteral FormatEnd {
return a
}
/ FormatStart s:NestedTemplateString FormatEnd {
return options.resolve(s)
}

InvalidFormatString
= Prefix? FormatStart .* {
Expand Down
101 changes: 53 additions & 48 deletions garden-service/test/unit/src/template-string.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,33 +43,6 @@ describe("resolveTemplateString", async () => {
expect(res).to.equal("value")
})

// it("should resolve a key via a resolver function", async () => {
// const resolver = (parts) => {
// expect(parts).to.eql(["nested", "key"])
// return "value"
// }
// const res = await resolveTemplateString("${some.nested.key}", new TestContext({ some: resolver }))
// expect(res).to.equal("value")
// })

// it("should resolve a key via a resolver function that returns a promise", async () => {
// const resolver = async (parts) => {
// expect(parts).to.eql(["nested", "key"])
// return "value"
// }
// const res = await resolveTemplateString("${some.nested.key}", { some: resolver }))
// expect(res).to.equal("value")
// })

// it("should resolve a key via a resolver function in a nested key", async () => {
// const resolver = (parts) => {
// expect(parts).to.eql(["key"])
// return "value"
// }
// const res = await resolveTemplateString("${some.nested.key}", { some: { nested: resolver } }))
// expect(res).to.equal("value")
// })

it("should handle multiple format strings", async () => {
const res = await resolveTemplateString("prefix-${a}-${b}-suffix", new TestContext({ a: "value", b: "other" }))
expect(res).to.equal("prefix-value-other-suffix")
Expand Down Expand Up @@ -134,28 +107,14 @@ describe("resolveTemplateString", async () => {
throw new Error("Expected error")
})

it("should handle nested format strings", async () => {
const res = await resolveTemplateString("${resol${part}ed}", new TestContext({ resolved: 123, part: "v" }))
expect(res).to.equal("123")
})

it("should handle nested format strings with nested keys", async () => {
const res = await resolveTemplateString(
"${resol${part}ed.nested}", new TestContext({ resolved: { nested: 123 }, part: "v" }),
)
expect(res).to.equal("123")
})

it("should handle nested format strings with format string at end", async () => {
const res = await resolveTemplateString("${resolv${part}}", new TestContext({ resolved: 123, part: "ed" }))
expect(res).to.equal("123")
})

it("should handle deeply nested format strings", async () => {
const res = await resolveTemplateString(
"${resol${pa${deep}t}ed}", new TestContext({ resolved: 123, deep: "r", part: "v" }),
it("should throw on nested format strings", async () => {
return expectError(
() => resolveTemplateString(
"${resol${part}ed}",
new TestContext({}),
),
(err) => (expect(err.message).to.equal("Invalid template string: ${resol${part}ed}")),
)
expect(res).to.equal("123")
})

it("should handle a single-quoted string", async () => {
Expand All @@ -174,6 +133,26 @@ describe("resolveTemplateString", async () => {
expect(res).to.equal("foo")
})

it("should throw on invalid single-quoted string", async () => {
return expectError(
() => resolveTemplateString(
"${'foo}",
new TestContext({}),
),
(err) => (expect(err.message).to.equal("Invalid template string: ${'foo}")),
)
})

it("should throw on invalid double-quoted string", async () => {
return expectError(
() => resolveTemplateString(
"${\"foo}",
new TestContext({}),
),
(err) => (expect(err.message).to.equal("Invalid template string: ${\"foo}")),
)
})

it("should handle a conditional between two identifiers", async () => {
const res = await resolveTemplateString(
"${a || b}",
Expand Down Expand Up @@ -212,6 +191,22 @@ describe("resolveTemplateString", async () => {
expect(res).to.equal("123")
})

it("should handle a conditional between two identifiers with first value undefined and string fallback", async () => {
const res = await resolveTemplateString(
"${a || \"foo\"}",
new TestContext({ a: undefined }),
)
expect(res).to.equal("foo")
})

it("should handle a conditional with undefined nested value and string fallback", async () => {
const res = await resolveTemplateString(
"${a.b || 'foo'}",
new TestContext({ a: {} }),
)
expect(res).to.equal("foo")
})

it("should handle a conditional between two identifiers without spaces with first value set", async () => {
const res = await resolveTemplateString(
"${a||b}",
Expand All @@ -230,6 +225,16 @@ describe("resolveTemplateString", async () => {
)
})

it("should throw on invalid conditional string", async () => {
return expectError(
() => resolveTemplateString(
"${a || 'b}",
new TestContext({}),
),
(err) => (expect(err.message).to.equal("Invalid template string: ${a || 'b}")),
)
})

it("should handle a conditional between an identifier and a string", async () => {
const res = await resolveTemplateString(
"${a || 'b'}",
Expand Down

0 comments on commit 0cbcb98

Please sign in to comment.