hcl2template: recursively evaluate local variables #13039
Merged
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
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
from
80e6729 to
69615d8
Compare
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
from
69615d8 to
11815a1
Compare
lbajolet-hashicorp
changed the base branch from
main
to
split_packer_test_suites
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
from
11815a1 to
dcd89dd
Compare
nywilken
reviewed
Jun 13, 2024
There was a problem hiding this comment.
I think this is a good approach to this issue and not something users would expect given that Terraform locals honor the dependencies. That said we need to update the documentation for our locals as the current documentation reads
The locals block is read as a map. Maps are not sorted, and therefore the evaluation time is not deterministic.
To avoid that, singular local blocks should be used instead. These will be evaluated in the order they are defined, and the evaluation order and time will always be the same.
dcd89dd to
1553088
Compare
lbajolet-hashicorp
changed the base branch from
split_packer_test_suites
to
main
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
from
0a86dbd to
26f2887
Compare
I'm not sure I see where those docs are in the repo, where did you see this? |
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
from
bf9163a to
26f2887
Compare
nywilken
approved these changes
Jun 17, 2024
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
3 times, most recently
from
ddcf937 to
1fcfafa
Compare
When a Packer command is created for testing the tool, we generally run it once, then the command is essentially nooping. This change allows us to run Packer multiple times with the same parameters, and make sure all runs conform to a specific list of checks. This allows us to more reliably test non-deterministic behaviours.
Some commands need to have an input in order to work. For those, we add the capability for the packerCommand struct to have their stdin defined from a string, which is then fed to the command being executed.
When a test fails to exert its assertions on the command-line output, a test fails, but we don't necessarily can troubleshoot what happened, especially when this happens in a CI environment. Therefore, for convenience, we add the faculty for packerCommand.Assert to automatically dump a command's output (both stdout and stderr) if a test fails.
The SkipNoAcc function on PackerTestSuite allows to mark a test run as not to be run every time we run `make test`, but only when PACKER_ACC=1 is set in the environment. This allows us to skip executing tests that are either long-running, or that depend on external dependencies (typically Github), which have a higher potential to fail on a normal run of Packer tests.
GetVarsByType is a function that gets a list of Traversals from a hcl Block. This approach works when what we are visiting is indeed one, however when we can get an immediate list of Traversals, but want to filter them based on their roots, we have to reimplement parts of that function. Therefore, we split this function in two, GetVarsByType still keeps its current behaviour, but the filtering step is exposed as another function now: FilterTraversalsByType, so we can reuse it elsewhere.
The logic for evaluating local variables used to rely on their definition order, with some edge cases. Typically `locals` blocks define multiple local variables, which don't necessarily appear in the same order at evaluation as within the template, leading to inconsistent behaviour, as the order in which those are added to the list of local variables is non-deterministic. To avoid this problem, we change how local variables are evaluated, and we're adopting a workflow similar to datasources, where the local variables first build a list of direct dependencies. Then when evaluation happens, we evaluate all the dependencies recursively for each local variable, which takes care of this issue. As with Datasources, we add a cap to the recursion: 10. I.e. if the evaluation of a single variable provokes an infinite recursion, we stop at that point and return an error to the user, telling them to fix their template.
Previously duplicate detection for local variables happened during `Initialise`, through a call to `checkForDuplicateLocalDefinition`. This works in a majority of cases, but for commands like `console`, this was not detected as the return diagnostics for `Initialise` are ignored. That check can be done as early as during parsing however, as the names of blocks are not dynamic in the slightest (no interpolation possible), so we move that detection logic into `Parse`, so that the behaviour is coherent between all commands.
When a command is run, it is the expectation that no test should make Packer panic. If it did, something is wrong and Packer should be fixed so it doesn't panic anymore in that situation. The way we did the check before was adding a PanicCheck after the command ran, so we could make sure of that during `Assert`. However, since we introduced the possibility to have multiple runs, having this addition as part of the run loop meant that the PanicCheck would be run as many times as there were runs. While this worked, this implied that we'd do the same check multiple times on a single command output, which is not optimal. Instead, this commit moves the check to within the `Run` function, this way for each run of the command we do the check once, and then we can assert the results of the command on what output it produced.
lbajolet-hashicorp
force-pushed
the
fix_locals_eval_order
branch
from
1fcfafa to
7823d15
Compare
nywilken
approved these changes
Jun 17, 2024
lbajolet-hashicorp
added
the
backport/1.11.x
|
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 issues. |
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.
The logic for evaluating local variables used to rely on their definition order, with some edge cases.
Typically
localsblocks define multiple local variables, which don't necessarily appear in the same order at evaluation as within the template, leading to inconsistent behaviour, as the order in which those are added to the list of local variables is non-deterministic.To avoid this problem, we change how local variables are evaluated, and we're adopting a workflow similar to datasources, where the local variables first build a list of direct dependencies.
Then when evaluation happens, we evaluate all the dependencies recursively for each local variable, which takes care of this issue.
As with Datasources, we add a cap to the recursion: 10. I.e. if the evaluation of a single variable provokes an infinite recursion, we stop at that point and return an error to the user, telling them to fix their template.
Closes: #13018