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

Stop the resource validate node transforming the original config #34026

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

liamcervante
Copy link
Member

@liamcervante liamcervante commented Oct 9, 2023

Currently, the validate resource node merges the base resource connection with any connection defined within the provisioner block. It then applies that merged result back into the original config. The value returned by the merge function contains both of the supplied configs. In a single execution this doesn't particularly matter, but in the testing framework this causes severe problems as the original config is shared and reused by all the run blocks. The current behaviour essentially causes the size of the connection config to double every time, so with 20 or 30 or 40 run blocks we getting larger and larger wait times for the validate node to execute, and then all subsequent nodes that use the modified config.

This PR updates the validate node so that it doesn't copy the merged config back into the original config, but just uses the merged config directly. I think this change is okay, as there is nothing downstream that is relying on the config being merged by the validate node.

An alternative solution here would be to do this merge in the configs package as soon as the configs are read initially. I don't think this is a good idea, as we may want to be able to deal with the config exactly as the user defined it, and by merging early we hide the actual written config. A second approach would be to stop the testing framework sharing the same config between the run blocks. In terms of performance, I think the implemented solution is preferable as we'd have to write complex Copy() logic to make duplicates of the original config to be used within the run blocks, or even re-read the config from scratch for each run block. Overall, I think the implemented solution is just much simpler.

Metanote: I don't know if we have lots of hidden places where the config is being transformed within the Terraform graph that makes the chosen approach for the testing framework somewhat dangerous. If it turns out that we regularly transform the supplied config within the graph, then maybe revisiting the approach of copying or reloading the original configuration for each run block would be preferable.

Fixes #34009

Target Release

1.6.1

Draft CHANGELOG entry

BUG FIXES

  • terraform test: Fix performance issues when using provisioners within configs being tested.

@liamcervante liamcervante added the 1.6-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Oct 9, 2023
@liamcervante liamcervante requested a review from a team October 9, 2023 12:53
@liamcervante liamcervante modified the milestone: v1.6.1 Oct 9, 2023
@alisdair alisdair added this to the v1.6.2 milestone Oct 10, 2023
@liamcervante liamcervante merged commit 4451a17 into main Oct 11, 2023
4 checks passed
@github-actions
Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Dec 8, 2023

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.6-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform test performs progressively slower as more test runs execute
2 participants