Skip to content

Commit

Permalink
fix: do not fail on empty YAML varfiles (#5759)
Browse files Browse the repository at this point in the history
**What this PR does / why we need it**:

The `js-yaml` parser returns `undefined` instead of an empty object
(`{}`) for empty input files.
So, we handle empty YAML varfiles as empty variable dictionaries to
avoid runtime errors.

This PR also converts `module-varfiles` test project to action-based
configuration.

**Which issue(s) this PR fixes**:

Fixes #5741
  • Loading branch information
vvagaytsev committed Feb 21, 2024
1 parent ccb0dd7 commit e5732aa
Show file tree
Hide file tree
Showing 11 changed files with 71 additions and 34 deletions.
15 changes: 10 additions & 5 deletions core/src/config/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -583,25 +583,30 @@ export async function loadVarfile({
const filename = basename(resolvedPath.toLowerCase())

if (filename.endsWith(".json")) {
// JSON parser throws a JSON syntax error on completely empty input file,
// and returns an empty object for an empty JSON.
const parsed = JSON.parse(data.toString())
if (!isPlainObject(parsed)) {
throw new ConfigurationError({
message: `Configured variable file ${relPath} must be a valid plain JSON object. Got: ${typeof parsed}`,
})
}
return parsed
return parsed as PrimitiveMap
} else if (filename.endsWith(".yml") || filename.endsWith(".yaml")) {
const parsed = load(data.toString())
// YAML parser returns `undefined` for empty files, we interpret that as an empty object.
const parsed = load(data.toString()) || {}
if (!isPlainObject(parsed)) {
throw new ConfigurationError({
message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof parsed}`,
})
}
return parsed as PrimitiveMap
} else {
// Note: For backwards-compatibility we fall back on using .env as a default format, and don't specifically
// validate the extension for that.
return dotenv.parse(await readFile(resolvedPath))
// Note: For backwards-compatibility we fall back on using .env as a default format,
// and don't specifically validate the extension for that.
// The dotenv parser returns an empty object for invalid or empty input file.
const parsed = dotenv.parse(data)
return parsed as PrimitiveMap
}
} catch (error) {
throw new ConfigurationError({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
c=from-action-varfile
d=from-action-varfile
22 changes: 22 additions & 0 deletions core/test/data/test-projects/action-varfiles/garden.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
apiVersion: garden.io/v1
kind: Project
name: action-varfiles
varfile: garden.project.env
environments:
- name: default
providers:
- name: test-plugin

---

kind: Run
name: run-a
type: test
varfiles: ["garden.run-a.${environment.name}.env"]
variables:
b: from-action-vars
c: from-action-vars # should be overridden by action-level varfile
d: from-action-vars # should be overridden by var passed as a CLI option

spec:
command: [echo, A]
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Empty file.
20 changes: 20 additions & 0 deletions core/test/data/test-projects/empty-varfiles/garden.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
apiVersion: garden.io/v1
kind: Project
name: empty-varfiles
environments:
- name: default
providers:
- name: test-plugin

---

kind: Run
name: run-a
type: test
varfiles:
- "empty-varfile.env"
- "empty-varfile.json"
- "empty-varfile.yml"

spec:
command: [echo, A]

This file was deleted.

21 changes: 0 additions & 21 deletions core/test/data/test-projects/module-varfiles/garden.yml

This file was deleted.

22 changes: 16 additions & 6 deletions core/test/unit/src/garden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import { keyBy, set, mapValues, omit, cloneDeep } from "lodash-es"
import { joi } from "../../../src/config/common.js"
import { defaultDotIgnoreFile, makeTempDir } from "../../../src/util/fs.js"
import fsExtra from "fs-extra"

const { realpath, writeFile, readFile, remove, pathExists, mkdirp, copy } = fsExtra
import { dedent, deline, randomString, wordWrap } from "../../../src/util/string.js"
import { getLinkedSources, addLinkedSources } from "../../../src/util/ext-source-util.js"
Expand Down Expand Up @@ -384,22 +385,31 @@ describe("Garden", () => {
})
})

it("should respect the module variables < module varfile < CLI var precedence order", async () => {
const projectRoot = getDataDir("test-projects", "module-varfiles")
it("should respect the action variables < action varfile < CLI var precedence order", async () => {
const projectRoot = getDataDir("test-projects", "action-varfiles")

const garden = await makeTestGarden(projectRoot)
// In the normal flow, `garden.variableOverrides` is populated with variables passed via the `--var` CLI option.
garden.variableOverrides["d"] = "from-cli-var"
const graph = await garden.getConfigGraph({ log: garden.log, emit: false })
const module = graph.getModule("module-a")
expect({ ...garden.variables, ...module.variables }).to.eql({
const runAction = graph.getRun("run-a")
expect({ ...garden.variables, ...runAction.getVariables() }).to.eql({
a: "from-project-varfile",
b: "from-module-vars",
c: "from-module-varfile",
b: "from-action-vars",
c: "from-action-varfile",
d: "from-cli-var",
})
})

it("should allow empty varfiles", async () => {
const projectRoot = getDataDir("test-projects", "empty-varfiles")

const garden = await makeTestGarden(projectRoot)
const graph = await garden.getConfigGraph({ log: garden.log, emit: false })
const runAction = graph.getRun("run-a")
expect(runAction.getVariables()).to.eql({})
})

it("should throw if project root is not in a git repo root", async () => {
const dir = await tmp.dir({ unsafeCleanup: true })

Expand Down

0 comments on commit e5732aa

Please sign in to comment.