-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat: evaluator v2 #909
base: main
Are you sure you want to change the base?
feat: evaluator v2 #909
Conversation
✅ Deploy Preview for terramate-io-docs canceled.
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #909 +/- ##
==========================================
+ Coverage 71.28% 71.56% +0.28%
==========================================
Files 84 84
Lines 13333 13443 +110
==========================================
+ Hits 9504 9621 +117
+ Misses 3491 3469 -22
- Partials 338 353 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
✅ Deploy Preview for terramate-io-docs canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #909 +/- ##
==========================================
+ Coverage 62.75% 64.19% +1.44%
==========================================
Files 102 100 -2
Lines 16395 16058 -337
==========================================
+ Hits 10288 10309 +21
+ Misses 5683 5326 -357
+ Partials 424 423 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
06c6a55
to
43932f0
Compare
✅ Deploy Preview for docs-terramate-io canceled.
|
43932f0
to
e84e5af
Compare
Signed-off-by: Tiago Natel <[email protected]>
e84e5af
to
798e539
Compare
Some variables are scoped by their expression declaration, that's the case for `[for ...]`, `{for ...}` and `tm_dynamic`. In this case, the `tm_ternary()` implementation must use the provided closure evaluator variables instead of the outer Terramate evaluator. Signed-off-by: Tiago Natel <[email protected]>
Co-authored-by: Sebastian <[email protected]>
## What this PR does / why we need it: Some variables are scoped by their expression declaration, that's the case for `[for ...]`, `{for ...}` and `tm_dynamic`. In this case, the `tm_ternary()` implementation must use the provided closure evaluator variables instead of the outer Terramate evaluator. This fixes an issue where `tm_ternary()` reported the error below: ``` eval expression: evaluating global["val"] from /stack scope: Call to function "tm_ternary" failed: /sandbox/stack/terramate.tm.hcl:5,14-15: partial evaluation failed: evaluating tm_ternary branch: eval expression: There is no variable named "a". ``` ## Which issue(s) this PR fixes: ## Special notes for your reviewer: - This is a fix for PR #909 - This issue was reported by @zied-elouaer and reproduced in a customer repository. - It was fixed in the Hackathon but just now isolated into a separate PR. ## Does this PR introduce a user-facing change? ``` no ```
…bals-v2-impl Signed-off-by: i4k <[email protected]>
evalctx.SetEnv(os.Environ()) | ||
evalctx := eval.New(st.Dir, globalsResolver, runtime.NewResolver(root, st)) | ||
evalctx.SetFunctions(stdlib.Functions(evalctx, st.HostDir(root))) | ||
//evalctx.SetEnv(os.Environ()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be fixed later
What this PR does / why we need it:
This PR introduces lazy evaluation to the
globals
andlets
blocks, then for each stack, not used variables are not evaluated.We need this because for the case of importing lots of files, with thousand stacks, Terramate wastes a lot of time evaluating not used variables.
This implementation gives
60-90%
performance gain but it heavily depends on the use case. For the case where all stacks requires all variables, then it's slightly slower (around 5% in simple tests).Which issue(s) this PR fixes:
Special notes for your reviewer:
TODO
Does this PR introduce a user-facing change?
Checklist