terraform test: move variable evaluation into the terraform test graph #37205
Conversation
liamcervante
force-pushed
the
liamcervante/specific-dependencies
branch
from
53eb2da to
184d7f5
Compare
liamcervante
force-pushed
the
liamcervante/test/graph-variables
branch
from
3debf19 to
fc892ad
Compare
dsa0x
left a comment
dsa0x
left a comment
There was a problem hiding this comment.
Looks good 👍 , but I have some questions
| requiredValues[addr.Name] = value.Value | ||
| continue | ||
| } | ||
| if value, valueDiags := ctx.EvaluateUnparsedVariable(addr.Name, run.ModuleConfig.Module.Variables[addr.Name]); value != nil { |
There was a problem hiding this comment.
Same question I asked earlier. Shouldn't these references have been resolved before we get here?
I see that the config here is coming from the module configuration. What if we add those variables into the graph as a VariableDefinition?
There was a problem hiding this comment.
Yeah, so the problem with this is that we basically allow users to just provide "external" variables into the testing framework without definitions. I've called them unparsed variables here, as at this point they are essentially just the raw values of variables arriving from either external files, environment variables, or CLI flags.
These external variables are then interpreted differently depending on when they're needed. This makes sense as a particular run block might have a configuration for a variable that it is expecting, and then it's really good that the testing framework just makes the external variables available to that run block without forcing the user to put them in somewhere in the testing file.
But, we did make a (probably bad) decision early on to just let people reference these in the testing file itself, and that's what you're seeing in the place you made the other comment. With the optional variable blocks introduced earlier, we can make people declare what they're using in the test file but we don't want breaking changes by forcing people to do this.
What this means is that we have these unparsed / external variables that are just hanging around and being lazily evaluated. We could add specific nodes into the graph for them, but we'd have to add nodes for the cross product of each run block with each unparsed variable when they're only going to be referenced once anyway.
One short-term solution, could be to reintroduce the variable cache we had before that makes sure these are only evaluated once lazily.
A possible long term solution, might be to add a warning / deprecation notice that says "we're going to stop this behaviour soon, you must add variable definition blocks if you want to do this". Then in 2-3 Terraform versions actually come in and break it.
But, we could do either of those in a follow up PR - what do you think?
There was a problem hiding this comment.
Thanks for the detailed explanation. It makes sense to me to make that decision in another PR. The deprecation notice would be nice, even if it takes longer than 2-3 versions to get it removed.
liamcervante
force-pushed
the
liamcervante/test/graph-variables
branch
from
982ab2b to
403e7b4
Compare
#37205) * terraform test: move variable evaluation into the terraform test graph * make copyrightfix
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
2 participants
This PR moves variable evaluation into the Terraform Test graph. This allows variables to reference other variables and run block outputs for the first time. Variable values are now just held in the evaluation context, and can be retrieved directly instead of needing to use the variable cache at all, so the variable cache has been deleted.
There is one test that must be skipped at the moment, because a follow up PR will add providers into the graph and there is a temporary breakage in behaviour until that PR has been completed.
Fixes #34629