Skip to content

Commit

Permalink
fix: correct source mapping in varfiles (#6870)
Browse files Browse the repository at this point in the history 
* fix: correct source mapping in varfiles

Ensures that template error messages in YAML varfiles will be correct

* fix: allow empty documents
  • Loading branch information
@stefreak
stefreak authored Feb 20, 2025
1 parent f2f80ca commit 994d78f
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 41 deletions.
79 changes: 56 additions & 23 deletions core/src/config/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@

import dotenv from "dotenv"
import { sep, resolve, relative, basename, dirname, join } from "path"
import { load } from "js-yaml"
import { lint } from "yaml-lint"
import { omit, isPlainObject } from "lodash-es"
import type { BuildDependencyConfig, ModuleConfig } from "./module.js"
import { coreModuleSpecSchema, baseModuleSchemaKeys } from "./module.js"
import { ConfigurationError, FilesystemError, isErrnoException, ParameterError } from "../exceptions.js"
import { DEFAULT_BUILD_TIMEOUT_SEC, defaultGardenApiVersion, GardenApiVersion } from "../constants.js"
import type { ProjectConfig } from "../config/project.js"
import type { ConfigSource } from "./validation.js"
import { validateWithPath } from "./validation.js"
import { defaultDotIgnoreFile, listDirectory } from "../util/fs.js"
import { isConfigFilename } from "../util/fs.js"
Expand Down Expand Up @@ -588,15 +588,8 @@ export async function findProjectConfig({
return
}

const _readFile = profileAsync(async function _readFile(path: string) {
return await readFile(path)
})

function _loadYaml(data: Buffer) {
return load(data.toString()) as PrimitiveMap
}

const loadVarfileCache = new LRUCache<string, Promise<PrimitiveMap>>({
type LoadedVarfile = { data: PrimitiveMap; source: ConfigSource }
const loadVarfileCache = new LRUCache<string, Promise<LoadedVarfile>>({
max: 10000,
ttl: 30000,
ttlAutopurge: true,
Expand Down Expand Up @@ -628,46 +621,81 @@ export const loadVarfile = profileAsync(async function loadVarfile({
}
const resolvedPath = resolve(configRoot, pathOrDefault)

let promise: Promise<PrimitiveMap> | undefined = loadVarfileCache.get(resolvedPath)
let promise: Promise<LoadedVarfile> | undefined = loadVarfileCache.get(resolvedPath)
if (!promise) {
promise = loadVarfileInner()
loadVarfileCache.set(resolvedPath, promise)
}

return await promise

async function loadVarfileInner(): Promise<PrimitiveMap> {
async function loadVarfileInner(): Promise<LoadedVarfile> {
try {
const data = await _readFile(resolvedPath)
log?.silly(() => `Loaded ${data.length} bytes from varfile ${resolvedPath}`)
const fileContents = await readFile(resolvedPath)
log?.silly(() => `Loaded ${fileContents.length} bytes from varfile ${resolvedPath}`)
const relPath = relative(configRoot, resolvedPath)
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())
const parsed = JSON.parse(fileContents.toString())
if (!isPlainObject(parsed)) {
throw new ConfigurationError({
message: `Configured variable file ${relPath} must be a valid plain JSON object. Got: ${typeof parsed}`,
})
}
return parsed as PrimitiveMap
return {
data: parsed as PrimitiveMap,
// source mapping to JSON has not been implemented at this point
source: { path: [] },
}
} else if (filename.endsWith(".yml") || filename.endsWith(".yaml")) {
const loaded = await loadAndValidateYaml({
content: fileContents.toString("utf-8"),
filename: resolvedPath,
version: "1.2",
sourceDescription: `varfile at ${relPath}`,
})
if (loaded.length === 0) {
// We treat empty documents as an empty object mapping
return {
data: {},
source: {
path: [],
},
}
}
if (loaded.length > 1) {
throw new ConfigurationError({
message: `Configured variable file ${relPath} must be a single YAML document. Got multiple (${loaded.length}) YAML documents`,
})
}
const yamlDoc = loaded[0]
// YAML parser returns `undefined` for empty files, we interpret that as an empty object.
const parsed = _loadYaml(data) || {}
if (!isPlainObject(parsed)) {
const data = yamlDoc.toJS() || {}
if (!isPlainObject(data)) {
throw new ConfigurationError({
message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof parsed}`,
message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof data}`,
})
}
return parsed as PrimitiveMap
return {
data,
source: {
path: [],
yamlDoc,
},
}
} else {
// 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
const parsed = dotenv.parse(fileContents)
return {
data: parsed as PrimitiveMap,
// source mapping to dotenv files has not been implemented at this point
source: { path: [] },
}
}
} catch (error) {
if (error instanceof ConfigurationError) {
Expand All @@ -687,7 +715,12 @@ export const loadVarfile = profileAsync(async function loadVarfile({
})
} else {
// The default var file did not exist. In that case we return empty object.
return {}
return {
data: {},
source: {
path: [],
},
}
}
}

Expand Down
36 changes: 18 additions & 18 deletions core/src/config/template-contexts/variables.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@ export class VariablesContext extends LayeredContext {
defaultPath: defaultProjectVarfilePath,
})
const projectVarfileVars = parseTemplateCollection({
value: rawProjectVarfileVars,
source: { path: ["varfile"], yamlDoc: projectConfig.internal.yamlDoc },
value: rawProjectVarfileVars.data,
source: rawProjectVarfileVars.source,
})

return new this(`project ${projectConfig.name}`, {
Expand All @@ -155,8 +155,8 @@ export class VariablesContext extends LayeredContext {
defaultPath: defaultEnvVarfilePath(environment),
})
const envVarfileVars = parseTemplateCollection({
value: rawEnvVarfileVars,
source: { path: ["varfile"], yamlDoc: projectConfig.internal.yamlDoc },
value: rawEnvVarfileVars.data,
source: rawEnvVarfileVars.source,
})

return new this(`environment ${environmentConfig.name}`, {
Expand All @@ -172,7 +172,7 @@ export class VariablesContext extends LayeredContext {
* garden.variableOverrides > module varfile > config.variables
*/
public static async forModule(garden: Garden, config: ModuleConfig, context: ModuleConfigContext) {
let varfileVars: DeepPrimitiveMap = {}
let varfileVars: ParsedTemplate = {}
if (config.varfile) {
const varfilePath = deepEvaluate(config.varfile, {
context,
Expand All @@ -183,12 +183,16 @@ export class VariablesContext extends LayeredContext {
message: `Expected varfile template expression in module configuration ${config.name} to resolve to string, actually got ${typeof varfilePath}`,
})
}
varfileVars = await loadVarfile({
const rawVarfileVars = await loadVarfile({
configRoot: config.path,
path: varfilePath,
defaultPath: undefined,
log: garden.log,
})
varfileVars = parseTemplateCollection({
value: rawVarfileVars.data,
source: rawVarfileVars.source,
})
}

return new this(describeConfig(config), {
Expand All @@ -205,21 +209,13 @@ export class VariablesContext extends LayeredContext {
group?: GroupConfig
) {
const effectiveConfigFileLocation = getEffectiveConfigFileLocation(config)
const rawActionVarfileVars = await loadVarfiles(garden, effectiveConfigFileLocation, config.varfiles || [])
const actionVarfileVars = parseTemplateCollection({
value: rawActionVarfileVars,
source: { path: ["varfiles"], yamlDoc: config.internal.yamlDoc },
})
const actionVarfileVars = await loadVarfiles(garden, effectiveConfigFileLocation, config.varfiles || [])
const actionVariables = [config.variables, ...actionVarfileVars]

let groupVarfileVars: ParsedTemplate[] = []
let groupVariables: ParsedTemplate[] = []
if (group) {
const rawGroupVarfileVars = await loadVarfiles(garden, group.path, group.varfiles || [])
groupVarfileVars = parseTemplateCollection({
value: rawGroupVarfileVars,
source: { path: [] },
})
groupVarfileVars = await loadVarfiles(garden, group.path, group.varfiles || [])
groupVariables = [group.variables, ...groupVarfileVars]
}

Expand Down Expand Up @@ -294,15 +290,19 @@ async function loadVarfiles(garden: Garden, configRoot: string, varfiles: Varfil
)

const varsByFile = await Promise.all(
(resolvedVarFiles || []).map((varfile) => {
(resolvedVarFiles || []).map(async (varfile) => {
const { path, optional } = getVarfileData(varfile)
return loadVarfile({
const loaded = await loadVarfile({
configRoot,
path,
defaultPath: undefined,
optional,
log: garden.log,
})
return parseTemplateCollection({
value: loaded.data,
source: loaded.source,
})
})
)

Expand Down

0 comments on commit 994d78f

Please sign in to comment.