Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

terraform test: move override evaluation into expander #35030

Merged
merged 2 commits into from Apr 24, 2024

Conversation

liamcervante
Copy link
Member

This PR makes use of the new expander logic to handle the terraform test module overrides directly where the modules are expanded instead of later within the group as it was doing previously.

Previously, we just generally let the graph expansion happen and then looked for nodes within overridden modules as the graph executed. We'd then skip nodes within overridden modules to ensure they didn't execute, and then sideload values into the output nodes of the overridden modules.

This was actually broken for the expand nodes, as they aren't keyed by a module instance address which means it is impossible for us to tell during graph execution that they should be skipped. This meant that the expansion was still attempted for anything within an overridden module. If that expansion then required a value from something else in the module such as a local variable, the expansion failed with a crash. This is because the local variable execution was skipped (as it was in an overridden module), while the expansion was still attempted by the expansion nodes are not keyed by module instances.

Now, we pass the override information into the expander. When it is asked for module instances, it excludes instances that have been overridden. This means that concrete execute nodes for overridden modules are not even created as a result of that expansion essentially not even existing.

For output nodes, we actually do include the overridden module instances and we implement the logic for checking if the module instance has been overridden directly within the output node. This means the output nodes can load the values from the override, instead of attempting to look at any references which would then fail since the other nodes in the module were not executed.

The change count here is smaller than it looks. I changed some wide-ranging function arguments, which has led to a lot of diffs that simply set false or nil for the includeOverrides and overrides arguments within the expander and the "get module instances" functions.

Fixes #35019

Target Release

1.8.2

Draft CHANGELOG entry

NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS

  • terraform test: Prevent crash when referencing local variables within overridden modules.

@liamcervante liamcervante added the 1.8-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Apr 19, 2024
@liamcervante liamcervante requested a review from a team April 19, 2024 10:14
// but not if the module is overridden directly.
// GetResourceOverride checks the overrides for the given resource instance.
// This function automatically checks if the containing resource has been
// overridden if the instance is instanced.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand that last sentence

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified!

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a lot, but from what I can tell this is well contained within the testing functionality, so should be OK to backport.

@liamcervante liamcervante merged commit 14418bd into main Apr 24, 2024
6 checks passed
@liamcervante liamcervante deleted the liamcervante/35019 branch April 24, 2024 05:57
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.8-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
2 participants