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.

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

perf: optimise template string resolving performance #6685

Merged
merged 43 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
b47e232
perf: optimise template string resolving performance
stefreak Dec 2, 2024
82cc0dd
chore: fix lint issues
vvagaytsev Dec 3, 2024
f4a88a9
test: fix tests
stefreak Dec 3, 2024
c7e0344
chore: fix compilation and lint errors
vvagaytsev Dec 3, 2024
4346ef9
chore: print details on assertion failure in `expectError` function
vvagaytsev Dec 3, 2024
acde6d3
test: fix some assertions
vvagaytsev Dec 3, 2024
8a3e3cb
fix: read raw template string from input
vvagaytsev Dec 3, 2024
08dbd39
test: fix tests for `resolveTemplateString`
vvagaytsev Dec 3, 2024
65a7ae5
chore: return value instead of throwing
vvagaytsev Dec 3, 2024
47b62a5
test: fix remaining resolveTemplateString tests
stefreak Dec 3, 2024
3eb8724
fix: bug fix + unit test
vvagaytsev Dec 3, 2024
56dd850
fix: test failures
vvagaytsev Dec 3, 2024
8aca5ea
fix: always use error message from context if it's available
vvagaytsev Dec 3, 2024
dd1602b
perf: optimize `ConfigContext.resolve`
stefreak Dec 3, 2024
02be20e
perf: don't cache non-template strings and also cache unescape
stefreak Dec 3, 2024
9abba8a
test: fix "exposes arguments and options correctly in command templates"
stefreak Dec 4, 2024
95262d4
fix: throw on critical errors while context resolution
vvagaytsev Dec 4, 2024
8f9daae
test: fix assertions
vvagaytsev Dec 4, 2024
b65c093
test: fix undefined errors in ConfigContext tests
stefreak Dec 4, 2024
dc1205e
test: fix remaining ConfigContext tests
stefreak Dec 4, 2024
1829f26
wip: use static analysis instead of scancontext
stefreak Dec 4, 2024
6fa3fea
chore: lint fixes
vvagaytsev Dec 4, 2024
2cf06d3
chore: replace `ScanContext` with `NoOpContext`
vvagaytsev Dec 4, 2024
d25d761
test: consume generator in tests
vvagaytsev Dec 4, 2024
e122a37
test: fix "throws if action kind is not resolvable"
stefreak Dec 4, 2024
29152fa
feat: allow referencing variables and evaluating complex expressions …
stefreak Dec 4, 2024
fc231be
refactor: get rid of function `collectTemplateReferences`
vvagaytsev Dec 4, 2024
498dc30
refactor: introduce local variables
vvagaytsev Dec 5, 2024
2ba9932
fix: module dependencies resolition
vvagaytsev Dec 5, 2024
4b4b787
fix: restore module name filter
vvagaytsev Dec 5, 2024
b40f5e6
test: update assertion for circular dep error
stefreak Dec 5, 2024
1f7d99c
chore: remove dead code
vvagaytsev Dec 5, 2024
03006d1
fix: handle all kinds of `GardenError` on the AST level
vvagaytsev Dec 5, 2024
4e51588
test: fix partial runtime resolution tests
stefreak Dec 5, 2024
f96b534
fix: make sure that yaml path is included in template string errors
stefreak Dec 5, 2024
77f4e5b
refactor: make code safer and easier to reason about
stefreak Dec 5, 2024
930b04a
improvement: minor improvements
stefreak Dec 5, 2024
3397b0e
test: additional tests
stefreak Dec 5, 2024
e220046
refactor: remove NoopContext
stefreak Dec 5, 2024
3a71187
fix: handle further edge cases in `getContextLookupReferences`
stefreak Dec 6, 2024
f9f9e43
fix: handle optional expressions in brackets correctly
stefreak Dec 6, 2024
2caced0
fix: special edge case in module resolution flow
stefreak Dec 6, 2024
d25be9f
chore: fix lint
stefreak Dec 6, 2024
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
2 changes: 1 addition & 1 deletion core/.mocharc.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require:
- build/test/setup.js
- ../../../../build/test/setup.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a little odd - What prompted this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Vovas IDE needed it to be able to run tests, and it didn't seem to have an effect on CI

Copy link
Member Author

Choose a reason for hiding this comment

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

But I agree it's odd, will have a closer look at this

watch-files:
- build/**/*
ignore:
Expand Down
16 changes: 8 additions & 8 deletions core/src/commands/custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class CustomCommandWrapper extends Command {
const commandVariables = resolveTemplateStrings({
value: this.spec.variables,
context: variablesContext,
source: { yamlDoc, basePath: ["variables"] },
source: { yamlDoc, path: ["variables"] },
})
const variables: any = jsonMerge(cloneDeep(garden.variables), commandVariables)

Expand All @@ -133,18 +133,18 @@ export class CustomCommandWrapper extends Command {
if (this.spec.exec) {
const startedAt = new Date()

const exec = validateWithPath({
const exec = validateWithPath<CommandResource["exec"]>({
config: resolveTemplateStrings({
value: this.spec.exec,
context: commandContext,
source: { yamlDoc, basePath: ["exec"] },
source: { yamlDoc, path: ["exec"] },
}),
schema: customCommandExecSchema(),
path: this.spec.internal.basePath,
projectRoot: garden.projectRoot,
configType: `exec field in custom Command '${this.name}'`,
source: undefined,
})
})!

const command = exec.command
log.debug(`Running exec command: ${command.join(" ")}`)
Expand Down Expand Up @@ -185,18 +185,18 @@ export class CustomCommandWrapper extends Command {
if (this.spec.gardenCommand) {
const startedAt = new Date()

let gardenCommand = validateWithPath({
let gardenCommand = validateWithPath<CommandResource["gardenCommand"]>({
config: resolveTemplateStrings({
value: this.spec.gardenCommand,
context: commandContext,
source: { yamlDoc, basePath: ["gardenCommand"] },
source: { yamlDoc, path: ["gardenCommand"] },
}),
schema: customCommandGardenCommandSchema(),
path: this.spec.internal.basePath,
projectRoot: garden.projectRoot,
configType: `gardenCommand field in custom Command '${this.name}'`,
source: undefined,
})
})!

log.debug(`Running Garden command: ${gardenCommand.join(" ")}`)

Expand Down Expand Up @@ -291,7 +291,7 @@ export async function getCustomCommands(log: Log, projectRoot: string) {
path: (<CommandResource>config).internal.basePath,
projectRoot,
configType: `custom Command '${config.name}'`,
source: { yamlDoc: (<CommandResource>config).internal.yamlDoc },
source: { yamlDoc: (<CommandResource>config).internal.yamlDoc, path: [] },
})
)

Expand Down
2 changes: 1 addition & 1 deletion core/src/commands/get/get-modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ function filterSecrets<T extends object>(object: T, secrets: StringMap): T {
const secretValues = new Set(Object.values(secrets))
const secretNames = Object.keys(secrets)
const sanitized = <T>deepMap(object, (value) => {
if (secretValues.has(value)) {
if (typeof value === "string" && secretValues.has(value)) {
const name = secretNames.find((n) => secrets[n] === value)!
return `[filtered secret: ${name}]`
} else {
Expand Down
6 changes: 3 additions & 3 deletions core/src/commands/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export class WorkflowCommand extends Command<Args, {}> {
const files = resolveTemplateStrings({
value: workflow.files || [],
context: templateContext,
source: { yamlDoc, basePath: ["files"] },
source: { yamlDoc, path: ["files"] },
})

// Write all the configured files for the workflow
Expand Down Expand Up @@ -175,12 +175,12 @@ export class WorkflowCommand extends Command<Args, {}> {
step.command = resolveTemplateStrings({
value: step.command,
context: stepTemplateContext,
source: { yamlDoc, basePath: ["steps", index, "command"] },
source: { yamlDoc, path: ["steps", index, "command"] },
}).filter((arg) => !!arg)

stepResult = await runStepCommand(stepParams)
} else if (step.script) {
step.script = resolveTemplateString({ string: step.script, context: stepTemplateContext })
step.script = resolveTemplateString({ string: step.script, context: stepTemplateContext }) as string
stepResult = await runStepScript(stepParams)
} else {
stepResult = undefined
Expand Down
6 changes: 3 additions & 3 deletions core/src/config/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import type { Document, DocumentOptions } from "yaml"
import { parseAllDocuments } from "yaml"
import { dedent, deline } from "../util/string.js"
import { makeDocsLinkStyled } from "../docs/common.js"
import { profile, profileAsync } from "../util/profiling.js"
import { profileAsync } from "../util/profiling.js"
import { readFile } from "fs/promises"
import { LRUCache } from "lru-cache"

Expand Down Expand Up @@ -557,9 +557,9 @@ const _readFile = profileAsync(async function _readFile(path: string) {
return await readFile(path)
})

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

const loadVarfileCache = new LRUCache<string, Promise<PrimitiveMap>>({
max: 10000,
Expand Down
6 changes: 5 additions & 1 deletion core/src/config/config-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ export async function resolveConfigTemplate(
const loggedIn = garden.isLoggedIn()
const enterpriseDomain = garden.cloudApi?.domain
const context = new ProjectConfigContext({ ...garden, loggedIn, enterpriseDomain })
const resolved = resolveTemplateStrings({ value: partial, context, source: { yamlDoc: resource.internal.yamlDoc } })
const resolved = resolveTemplateStrings({
value: partial,
context,
source: { yamlDoc: resource.internal.yamlDoc, path: [] },
})
const configPath = resource.internal.configFilePath

// Validate the partial config
Expand Down
6 changes: 3 additions & 3 deletions core/src/config/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ export function resolveProjectConfig({
secrets,
commandInfo,
}),
source: { yamlDoc: config.internal.yamlDoc, basePath: [] },
source: { yamlDoc: config.internal.yamlDoc, path: [] },
})
} catch (err) {
log.error("Failed to resolve project configuration.")
Expand Down Expand Up @@ -622,7 +622,7 @@ export const pickEnvironment = profileAsync(async function _pickEnvironment({
})
const projectVariables: DeepPrimitiveMap = <any>merge(projectConfig.variables, projectVarfileVars)

const source = { yamlDoc: projectConfig.internal.yamlDoc, basePath: ["environments", index] }
const source = { yamlDoc: projectConfig.internal.yamlDoc, path: ["environments", index] }

// Resolve template strings in the environment config, except providers
environmentConfig = resolveTemplateStrings({
Expand All @@ -642,7 +642,7 @@ export const pickEnvironment = profileAsync(async function _pickEnvironment({
source,
})

environmentConfig = validateWithPath({
environmentConfig = validateWithPath<EnvironmentConfig>({
config: environmentConfig,
schema: environmentSchema(),
configType: `environment ${environment}`,
Expand Down
65 changes: 45 additions & 20 deletions core/src/config/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,18 @@

import { deline } from "../util/string.js"
import {
joiIdentifier,
joiUserIdentifier,
joiArray,
createSchema,
joi,
joiArray,
joiIdentifier,
joiIdentifierMap,
joiSparseArray,
createSchema,
joiUserIdentifier,
} from "./common.js"
import { collectTemplateReferences } from "../template-string/template-string.js"
import { ConfigurationError } from "../exceptions.js"
import type { ModuleConfig } from "./module.js"
import { moduleConfigSchema } from "./module.js"
import { memoize, uniq } from "lodash-es"
import { isNumber, isString, memoize, uniq } from "lodash-es"
import type { GardenPluginSpec } from "../plugin/plugin.js"
import type { EnvironmentStatus } from "../plugin/handlers/Provider/getEnvironmentStatus.js"
import { environmentStatusSchema } from "./status.js"
Expand All @@ -30,6 +29,8 @@ import type { ActionState } from "../actions/types.js"
import type { ValidResultType } from "../tasks/base.js"
import { uuidv4 } from "../util/random.js"
import { s } from "./zod.js"
import { getContextLookupReferences, visitAll } from "../template-string/static-analysis.js"
import type { ConfigContext } from "./template-contexts/base.js"

// TODO: dedupe from the joi schema below
export const baseProviderConfigSchemaZod = s.object({
Expand Down Expand Up @@ -164,34 +165,58 @@ export function providerFromConfig({
* Given a plugin and its provider config, return a list of dependency names based on declared dependencies,
* as well as implicit dependencies based on template strings.
*/
export function getAllProviderDependencyNames(plugin: GardenPluginSpec, config: GenericProviderConfig) {
export function getAllProviderDependencyNames(
plugin: GardenPluginSpec,
config: GenericProviderConfig,
context: ConfigContext
) {
return uniq([
...(plugin.dependencies || []).map((d) => d.name),
...(config.dependencies || []),
...getProviderTemplateReferences(config),
...getProviderTemplateReferences(config, context),
]).sort()
}

/**
* Given a provider config, return implicit dependencies based on template strings.
*/
export function getProviderTemplateReferences(config: GenericProviderConfig) {
const references = collectTemplateReferences(config)
export function getProviderTemplateReferences(config: GenericProviderConfig, context: ConfigContext) {
const deps: string[] = []

for (const key of references) {
if (key[0] === "providers") {
const providerName = key[1] as string
if (!providerName) {
throw new ConfigurationError({
message: deline`
Invalid template key '${key.join(".")}' in configuration for provider '${config.name}'. You must
const generator = getContextLookupReferences(
visitAll({
value: config,
parseTemplateStrings: true,
// TODO: get proper source
source: {
path: [],
},
}),
context
)
for (const finding of generator) {
const keyPath = finding.keyPath
if (keyPath[0] !== "providers") {
continue
}

const providerName = keyPath[1]
if (!providerName || isNumber(providerName)) {
throw new ConfigurationError({
message: deline`s
Invalid template key '${keyPath.join(".")}' in configuration for provider '${config.name}'. You must
specify a provider name as well (e.g. \${providers.my-provider}).
`,
})
}
deps.push(providerName)
})
}
if (!isString(providerName)) {
const err = providerName.getError()
throw new ConfigurationError({
message: `Found invalid provider reference: ${err.message}`,
})
}

deps.push(providerName)
}

return uniq(deps).sort()
Expand Down
19 changes: 13 additions & 6 deletions core/src/config/render-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,15 @@ export async function renderConfigTemplate({
const resolvedWithoutInputs = resolveTemplateStrings({
value: { ...omit(config, "inputs") },
context: templateContext,
source: { yamlDoc },
source: { yamlDoc, path: [] },
})
const partiallyResolvedInputs = resolveTemplateStrings({
value: config.inputs || {},
context: templateContext,
contextOpts: {
allowPartial: true,
},
source: { yamlDoc, basePath: ["inputs"] },
source: { yamlDoc, path: ["inputs"] },
})
let resolved: RenderTemplateConfig = {
...resolvedWithoutInputs,
Expand Down Expand Up @@ -214,7 +214,7 @@ async function renderModules({
value: m,
context,
contextOpts: { allowPartial: true },
source: { yamlDoc, basePath: ["modules", i] },
source: { yamlDoc, path: ["modules", i] },
})
const renderConfigPath = renderConfig.internal.configFilePath || renderConfig.internal.basePath

Expand Down Expand Up @@ -273,22 +273,29 @@ async function renderConfigs({
context: RenderTemplateConfigContext
renderConfig: RenderTemplateConfig
}): Promise<TemplatableConfig[]> {
const source = { yamlDoc: template.internal.yamlDoc, path: ["configs"] }
vvagaytsev marked this conversation as resolved.
Show resolved Hide resolved

const templateDescription = `${configTemplateKind} '${template.name}'`
const templateConfigs = template.configs || []
const partiallyResolvedTemplateConfigs = resolveTemplateStrings({
value: templateConfigs,
context,
contextOpts: { allowPartial: true },
source: { yamlDoc: template.internal.yamlDoc, basePath: ["inputs"] },
source,
})

return Promise.all(
partiallyResolvedTemplateConfigs.map(async (m) => {
partiallyResolvedTemplateConfigs.map(async (m, index) => {
// Resolve just the name, which must be immediately resolvable
let resolvedName = m.name

try {
resolvedName = resolveTemplateString({ string: m.name, context, contextOpts: { allowPartial: false } })
resolvedName = resolveTemplateString({
string: m.name,
context,
contextOpts: { allowPartial: false },
source: { ...source, path: [...source.path, index, "name"] },
}) as string
} catch (error) {
throw new ConfigurationError({
message: `Could not resolve the \`name\` field (${m.name}) for a config in ${templateDescription}: ${error}\n\nNote that template strings in config names in must be fully resolvable at the time of scanning. This means that e.g. references to other actions, modules or runtime outputs cannot be used.`,
Expand Down
Loading