Always return all module instances during evaluation#24697
Merged
Conversation
In order to be able to use module values, and handle operations like possibly invalid module indexes in conditional statements, whole modules must always be returned during evaluation.
Module references, like resource references, need to always return the and object containing all instances in order to handle modules as single values, and to postpone index evaluation to when the expression as whole is evaluated.
Since modules need to be evaluated as whole objects, yet the outputs are all handled individually, we need a method to collect and return all output changes for a module from the plan, including all known module instances.
The evaluationStateData needs the change to the GetModule method to work with the new evaluator. This is using a deep copy of module instances, which we will clean up after some changes to the states package.
We need all module instance outputs to build the objects for evaluation, but there is no need to copy all the resource instances along with that. This allows us to only return the output states, with enough information to connect them with their module instances.
In order to efficiently build the module objects for evaluation, we need to collect the outputs from a set of module instances. The ModuleOutputs method will return a copy of the state outputs, while not requiring the unnecessary copying of each entire module.
This way we don't need the extra copy of the entire module.
There is no codepath that can use this any longer, since we need to evaluate the modules as whole objects. This means we're going to have to live for now with invalid module output references returning "object" errors rather that "module".
The module error is unfortunately less specific at the moment, but change the error text here to match.
ghost
added
the
sdkv1
jbardin
changed the title
There is no expansion during validation, so in order for module references to work we need to ensure that the returned values are unknown.
jbardin
force-pushed
the
jbardin/get-module-data
branch
from
1390fec to
92837e6
Compare
Codecov Report
|
pselle
approved these changes
Apr 20, 2020
Contributor
pselle
left a comment
pselle
left a comment
There was a problem hiding this comment.
General approval, one question on a comment that looks out of date
| var diags tfdiags.Diagnostics | ||
|
|
||
| // Output results live in the module that declares them, which is one of | ||
| // the child module instances of our current module path. |
Contributor
There was a problem hiding this comment.
Is this comment still correct? It mentions instances, and looks lime moduleAddr is an addrs.Module
Member
Author
There was a problem hiding this comment.
Yes, that's fine here, it's just describing the existing relationship in the state, not the resulting output of the function.
|
|
||
| parentCfg := d.Evaluator.Config.DescendentForInstance(d.ModulePath) | ||
| callConfig, ok := parentCfg.Module.ModuleCalls[addr.Name] | ||
| if !ok { |
Merged
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
In order to be able to use module values, and handle operations like possibly invalid module indexes in conditional statements; whole, expanded modules must always be returned during evaluation. This is the same methodology applied to resources in #22846.
This also requires some new methods on the Changes and State, since the evaluation is happening from within a known
addrs.ModuleInstancepath, but we need to retrieve all instances of the child module being referenced. We need to collect all outputs from all possible module instances, as module outputs are handled individually (as opposed to resources which are stored as objects containing all their attributes), so this allows us to compile a data structure with all the outputs for each module in order to build thecty.Objectvalues that evaluation expects without copying all module resources as well.