core: Generate placeholder results for named values under partially-expanded module prefixes #34578
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
apparentlymart
added
enhancement
core
unknown-values
alisdair
approved these changes
Jan 29, 2024
apparentlymart
changed the base branch from
f-deferred-actions-placeholder-eval
to
main
Since local value evaluation is largely identical between the fully- and partially-expanded cases, this factors out that handling into a shared function and makes the two graph node Execute functions just thin wrappers around it. The error-handling behavior is intentionally marginally different: it now always registers the local value result in the named values state, even on error, but uses cty.DynamicVal as a placeholder when an error prevents returning anything more precise than that. Nothing depends on that detail because downstream nodes don't get to execute when an upstream returns an error, but this approach reduces the number of possible outcomes and thus makes this marginally easier to follow. This also includes some modernization of the unit tests for TestNodeLocalExecute, so that it no longer relies on shimming functions from the Terraform v0.12 transition.
Since input variables are defined in the parent of the module where they are declared, this particular case ends up straddling over a module boundary: the EvalContext belongs to the parent while the node's own absolute address belongs to the child. The DynamicExpand implementation of the expander node already knows how to wire that up (the same way as for the fully-expanded equivalent) and so we don't need to do anything particularly special in this function, but it does mean that we need to provide some placeholder repetition data to use in case the definition expression refers to each.key, each.value, or count.index. For now we're using the "totally-unknown" repetition data that just sets all three symbols to unknown values. In future it would be nice to improve the precision here so that e.g. each.value can be treated as invalid when used in a call that doesn't set for_each, but that would require propagating the call configuration into this graph node and we don't yet have the needed information in the code that generates this node, so we'll save the more disruptive rework to allow that for a later change.
apparentlymart
force-pushed
the
f-deferred-actions-named-value-placeholders
branch
from
6f490cf to
d0c22a0
Compare
apparentlymart
deleted the
f-deferred-actions-named-value-placeholders
branch
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This is a further iteration towards #30937, and follows on from yesterday's #34561 (which needs to land first).
These commits implement the
Executemethod for the three graph node types representing input variables, local values, and output values under partially-expanded module prefixes. In all three cases the goal is twofold:Raise any errors where can be confident that they will definitely happen once the expansion becomes fully-known, to shorten the iterative journey as much as we can.
Produce a placeholder value that represents what all instances of the relevant object have in common.
For example, if the expression that defines the value is one that will always return a string but the exact string might vary between instances then
cty.UnknownVal(cty.String)would be a good placeholder, since it will allow us to catch problems downstream if the result is used in a location where a string is not acceptable.For the moment my focus is on getting this new mechanism implemented at all and so I'm intentionally accepting some compromises on the above where the placeholder value might be less precise than it really needs to be, or we might miss an error that we could potentially catch, because we can always improve the level of precision later if real-world experience shows that it would be valuable to do so, at the expense of poking some additional data through some of these layers so we'll have the information needed to make those decisions.
As usual with this series of changes, reviewing on a commit-by-commit basis might make this easier to digest, although this time the overall pattern is the same for all the commits so this one is probably more approachable as a flattened diff if you'd rather that.
Also as usual, this isn't intended to change any externally-observable behavior for a configuration that doesn't have any modules enabling the language experiment. Enabling the language experiment does allow executing these codepaths in principle, but since only named values are handled at this point there's not really anything useful to be gained by doing that. More to come in future PRs!
I did hope that this changeset would be the first milestone where it was possible to write a useful integration test involving a module that doesn't include any resources, but alas I have now found that (obviously, in retrospect) the external Terraform Core API that our integration tests use cannot directly observe the special behavior going on here, because by the time it gets to the external API it is at best just a few more unknown values.
Therefore my new plan is to wait until we have the basics of dealing with resources under partially-expanded module prefixes, which will then allow using the mock provider API to give an integration test better signals about what's going on so we can have a more useful test than just whether the result of evaluation was unknown (which it could be for various reasons) or whether the operation returned a specific error message that actually lives in some other part of the system.