perf: improve preprocess action perf by only resolving template strings when absolutely needed #6745
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
partially resolving template strings
stefreak
commented
Jan 2, 2025
stefreak
commented
Jan 2, 2025
The yaml highlighting did not work in production builds anyway and removing this functionality made it easier to compare performance between production builds and the dev build for commands like `get graph`.
stefreak
force-pushed
the
preprocess-perf
branch
from
99f72e9 to
cb0081c
Compare
fail early if we access an unresolved template value
Co-authored-by: Vladimir Vagaytsev <vvagaytsev@users.noreply.github.com>
Co-authored-by: Steffen Neubauer <steffen@garden.io>
This was referenced Jan 31, 2025
vvagaytsev
approved these changes
Jan 31, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This was referenced Jan 31, 2025
Closed
This was referenced Feb 12, 2025
stefreak
added a commit
that referenced
this pull request
Feb 24, 2025
…n action configs This commit fixes a regression introduced in #6745 where we stopped throwing an error for unknown keys in action configs.
stefreak
added a commit
that referenced
this pull request
Feb 24, 2025
…n action configs This commit fixes a regression introduced in #6745 where we stopped throwing an error for unknown keys in action configs.
stefreak
added a commit
that referenced
this pull request
Feb 24, 2025
…n action configs This commit fixes a regression introduced in #6745 where we stopped throwing an error for unknown keys in action configs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Improve preprocess action performance
In large monorepo settings, this change improves the performance of the
garden validatecommand (Which processes actions and templates) by about 35%. In smaller projects the performance improvement is not noticeable.We achieve this by fully separating parsing of configs and evaluation of configs, and also dropping partial evaluation of template expressions. Whenever a value needs to be known right now, we fully resolve the template string.
This will fix long-standing issues like #5825 (We were prone to template injection before).
We also keep track of the YAML source for all template strings automatically and don't need to worry about manually keeping track of this anymore. This means that error messages will be improved for all users, even for Modules.
The
ModuleResolveris the only place left where we partially resolve configs, and where we evaluate template strings and then parse them again.Exclude variables and varfile contents from version calculation
This PR also excludes values of variables and varfile contents from version calculation, because
what counts is how variables affect the action or module spec. If a variable is actually used and impacts the spec, it will affect version calculation. This should fix #3473
Which issue(s) this PR fixes:
Fixes #5202, #5442, #5825, #3473 (and possibly more)