Skip to content

Commit

Permalink
fix(templates): fix regression with multiple if statements introduced…
Browse files Browse the repository at this point in the history
… in 0.13.45 (#6714)

fix(templates): fix regression with multiple if statements introduced in
0.13.45

In `buildConditionalTree`, we did not properly reset state and thus we
got a wrong AST if multiple if statements existed in the same nesting
level.
  • Loading branch information
stefreak authored Dec 12, 2024
1 parent 424b392 commit 7fbe717
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 21 deletions.
10 changes: 5 additions & 5 deletions core/src/template-string/parser.pegjs
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,9 @@
let currentCondition = undefined
let consequent = []
let alternate = []
let nestingLevel = 0
let encounteredElse = false
let nestingLevel = 0

const pushElement = (e) => {
if (!currentCondition) {
return rootExpressions.push(e)
Expand Down Expand Up @@ -147,6 +148,9 @@
currentCondition.endIf = e
rootExpressions.push(currentCondition)
currentCondition = undefined
consequent = []
alternate = []
encounteredElse = false
} else {
nestingLevel--
pushElement(e)
Expand All @@ -173,10 +177,6 @@
return new ast.BlockExpression(...rootExpressions)
}

function isBlockExpression(e) {
return e instanceof ast.IfBlockExpression || e instanceof ast.ElseBlockExpression || e instanceof ast.EndIfBlockExpression
}

function optionalList(value) {
return value !== null ? value : [];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,13 @@ name: legacypartial-ifblock-false
spec:
files:
- ./k8s/ifblock-false.yaml

---

kind: Deploy
type: kubernetes
name: legacypartial-ifblock-indentation
spec:
files:
- ./k8s/ifblock-indentation.yaml

Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: someapplication
labels:
app: someapplication
tier: web
spec:
replicas: 1
selector:
matchLabels:
app: someapplication
tier: web
strategy:
type: Recreate
template:
metadata:
labels:
app: someapplication
tier: web
spec:
containers:
- name: someapplication
image: ${actions.build.test-image.outputs.deployment-image-id}
command: ${jsonEncode(var[var.env-type].command)}
resources:
${if environment.name != 'local'}
requests:
memory: 1G
cpu: 1
limits:
memory: 5G
cpu: 1
# memory: 300M
${endif}
env:
- name: TEST_APP
value: someapplication
- name: TEST_DOMAIN
value: ${var.hostname}
volumeMounts:
- name: google-cloud-key
mountPath: /path/to/secret
${if true}
- name: someapplication-flower
image: ${actions.build.test-image.outputs.deployment-image-id}
resources:
requests:
memory: 300M
cpu: 500m
limits:
memory: 600M
cpu: 1
env:
- name: TEST_APP
value: someapplication-flower
ports:
- containerPort: 1234

volumeMounts:
- name: some-volume
mountPath: /path/to/secret
${endif}
volumes:
- name: some-volume
secret:
secretName: some-secret-name
11 changes: 11 additions & 0 deletions core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,17 @@ describe("getManifests", () => {
})
})

it("should not crash due to indentation with if block statement", async () => {
action = await garden.resolveAction<KubernetesDeployAction>({
action: cloneDeep(graph.getDeploy("legacypartial-ifblock-indentation")),
log: garden.log,
graph,
})

const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace })
expect(result.length).to.eq(2) // due to metadata configmap
})

it("partially resolves the consequent branch of ${if true} block", async () => {
action = await garden.resolveAction<KubernetesDeployAction>({
action: cloneDeep(graph.getDeploy("legacypartial-ifblock-true")),
Expand Down
65 changes: 49 additions & 16 deletions core/test/unit/src/template-string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,26 @@
*/

import { expect } from "chai"
import {
resolveTemplateString,
resolveTemplateStrings,
throwOnMissingSecretKeys,
getActionTemplateReferences,
} from "../../../src/template-string/template-string.js"
import { CONTEXT_RESOLVE_KEY_AVAILABLE_LATER, GenericContext } from "../../../src/config/template-contexts/base.js"
import type { TestGarden } from "../../helpers.js"
import { expectError, expectFuzzyMatch, getDataDir, makeTestGarden } from "../../helpers.js"
import { dedent } from "../../../src/util/string.js"
import repeat from "lodash-es/repeat.js"
import stripAnsi from "strip-ansi"
import { loadAndValidateYaml } from "../../../src/config/base.js"
import { CONTEXT_RESOLVE_KEY_AVAILABLE_LATER, GenericContext } from "../../../src/config/template-contexts/base.js"
import { TemplateStringError } from "../../../src/exceptions.js"
import repeat from "lodash-es/repeat.js"
import type { ContextLookupReferenceFinding } from "../../../src/template-string/static-analysis.js"
import {
getContextLookupReferences,
UnresolvableValue,
visitAll,
} from "../../../src/template-string/static-analysis.js"
import { loadAndValidateYaml } from "../../../src/config/base.js"
import {
getActionTemplateReferences,
resolveTemplateString,
resolveTemplateStrings,
throwOnMissingSecretKeys,
} from "../../../src/template-string/template-string.js"
import { dedent } from "../../../src/util/string.js"
import type { TestGarden } from "../../helpers.js"
import { expectError, expectFuzzyMatch, getDataDir, makeTestGarden } from "../../helpers.js"

describe("resolveTemplateString", () => {
it("should return a non-templated string unchanged", () => {
Expand Down Expand Up @@ -1436,38 +1436,71 @@ describe("resolveTemplateString", () => {
})

context("conditional blocks", () => {
it("single-line if block (positive)", () => {
it("One single-line if block (positive)", () => {
const res = resolveTemplateString({
string: "prefix ${if a}content ${endif}suffix",
context: new GenericContext({ a: true }),
})
expect(res).to.equal("prefix content suffix")
})

it("single-line if block (negative)", () => {
it("One single-line if block (negative)", () => {
const res = resolveTemplateString({
string: "prefix ${if a}content ${endif}suffix",
context: new GenericContext({ a: false }),
})
expect(res).to.equal("prefix suffix")
})

it("single-line if/else statement (positive)", () => {
it("One single-line if/else statement (positive)", () => {
const res = resolveTemplateString({
string: "prefix ${if a == 123}content ${else}other ${endif}suffix",
context: new GenericContext({ a: 123 }),
})
expect(res).to.equal("prefix content suffix")
})

it("single-line if/else statement (negative)", () => {
it("One single-line if/else statement (negative)", () => {
const res = resolveTemplateString({
string: "prefix ${if a}content ${else}other ${endif}suffix",
context: new GenericContext({ a: false }),
})
expect(res).to.equal("prefix other suffix")
})

it("Two single-line if block (positive)", () => {
const res = resolveTemplateString({
string: "prefix ${if a}content ${endif}suffixprefix ${if a}content ${endif}suffix",
context: new GenericContext({ a: true }),
})
expect(res).to.equal("prefix content suffixprefix content suffix")
})

it("Two single-line if block (negative)", () => {
const res = resolveTemplateString({
string: "prefix ${if a}content ${endif}suffixprefix ${if a}content ${endif}suffix",
context: new GenericContext({ a: false }),
})
expect(res).to.equal("prefix suffixprefix suffix")
})

it("Two single-line if/else statement (positive)", () => {
const res = resolveTemplateString({
string:
"prefix ${if a == 123}content ${else}other ${endif}suffixprefix ${if a == 123}content ${else}other ${endif}suffix",
context: new GenericContext({ a: 123 }),
})
expect(res).to.equal("prefix content suffixprefix content suffix")
})

it("Two single-line if/else statement (negative)", () => {
const res = resolveTemplateString({
string: "prefix ${if a}content ${else}other ${endif}suffixprefix ${if a}content ${else}other ${endif}suffix",
context: new GenericContext({ a: false }),
})
expect(res).to.equal("prefix other suffixprefix other suffix")
})

it("multi-line if block (positive)", () => {
const res = resolveTemplateString({
string: "prefix\n${if a}content\n${endif}suffix",
Expand Down

0 comments on commit 7fbe717

Please sign in to comment.