chore: refactor EvaluationContext #235
Conversation
- I wanted to make NewEvaluationContext take an options struct, so we could
more easily add parameters without breaking call sites. Since this options
struct ended up being the context itself, I removed `NewEvaluationContext`
for now...
- I tweaked the interface of functions, so all functions have the same
shape.
- Adjusted the visibility of Function/EvaluationContext so in principle
callers can add their own functions
jaspervdj-snyk
requested review from
ricardo-snyk,
craigfurman and
a team
and removed request for
a team
There was a problem hiding this comment.
Approving because I have no objections to this, but I'm not understanding from the context here why these changes are improvements. I'm relatively inexperienced on this project of course, and I'm sure there's a good reason, but it would be good to see that in the commit message or elsewhere! 🙏
| "concat": concatImpl, | ||
| "resourceGroup": resourceGroupImpl, | ||
| }, | ||
| type Function func(e *EvaluationContext, args ...interface{}) (interface{}, error) |
There was a problem hiding this comment.
I know you called this out in the commit message (all functions have the same shape) but why? I'm not quite seeing what it buys us.
This is super speculative, but having one eye on a future multi-pass parser/interpreter, there are some functions we won't be able to evaluate at non-final passes. For example, we could evaluate calls to parameters() and concat() in variable definitions (albeit probably returning a stub from parameters() given that we don't have "real" deployment context), but we can't evaluate resourceId before we've built the resource set.
Having a difference in function signature for these functions that depend on state other than their args made that distinction clear and (this is the speculative part) could possibly help us partition functions into which pass they are available in if/when we build that. This certainly isn't something I'm clinging strongly to, and I'm happy to wait until we actually tackle that work, and will likely come up with a code organisation pattern that suits us then - I just thought it was worth stating now, as a non-blocking possible reason to consider. 🙏
There was a problem hiding this comment.
This is all a bit speculative--
In general, having the same shape for these allows you to manipulate them a bit more easy, e.g. have helpers like func memoize(f Function) Function.
On the other hand, it is useful to see which function depend on additional context. it's a bit of a trade-off though, does a function like variables() depend on EvaluationContext or do we go even more granular and depend on Variables only?
Tying them to the evaluation pass seems sensible, but in that case we'd want to propagate that type to the context, and I'm not really sure if Go's generics will make that nice...
As a middle way, I'll:
- update the function implementations to not take any context if they don't have to
- keep the
Functiontype like this because it accurately describes "everything" a function can do which is useful
There was a problem hiding this comment.
I think since we're both speculating, please feel free to adjust the types to whatever makes our lives easier today. We can always change them when we confront multi-pass!
pkg/input/arm/eval.go
Outdated
| funcs map[string]armFnImpl | ||
| DiscoveredResourceSet map[string]struct{} | ||
| Variables map[string]interface{} | ||
| BuiltinFunctions map[string]Function |
There was a problem hiding this comment.
| BuiltinFunctions map[string]Function | |
| Functions map[string]Function |
If the idea is for callers to extend the function set, wouldn't it just be Functions?
But also, why is it desirable for callers to be able to extend the function set? Is this package not really just an internal dependency of pkg/input?
There was a problem hiding this comment.
I did the rename.
I wasn't necessarily thinking about extending it; more the other way around: restrict functions that aren't available in certain passes or replace them by dummies.
There was a problem hiding this comment.
Ah I think I see. Do you mean that in the parent package, in arm.go, we might in a multi-pass future add functions to the context that can now be meaningfully evaluated after passes? That makes sense if so.
I wanted to make NewEvaluationContext take an options struct, so we could more easily add parameters without breaking call sites. Since this options struct ended up being the context itself, I removed
NewEvaluationContextfor now...I tweaked the interface of functions, so all functions have the same shape.
Adjusted the visibility of Function/EvaluationContext so in principle callers can add their own functions