Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct source mapping in varfiles #6870

Merged
merged 2 commits into from
Feb 20, 2025
Merged

fix: correct source mapping in varfiles #6870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4015a99
fix: correct source mapping in varfiles
stefreak Feb 20, 2025
ebdbac5
fix: allow empty documents
stefreak Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 56 additions & 23 deletions core/src/config/base.ts
View file
Open in desktop
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",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using a different yaml parsing library here than before; It looks like the version of js-yaml we were using for varfiles also used YAML 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
Open in desktop
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