: core: Fix destroy w/module vars used in counts #6753
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
A new problem was introduced by the prior fixes for destroy interpolation messages when resources depend on module variables with a _count_ attribute, this makes the variable crucial for properly building the graph - even in destroys. So removing all module variables from the graph as noops was overzealous. By borrowing the logic in `DestroyEdgeInclude` we are able to determine if we need to keep a given module variable relatively easily. I'd like to overhaul the `Destroy: true` implementation so that it does not depend on config at all, but I want to continue for now with the targeted fixes that we can backport into the 0.6 series.
Building on b10564a, adding tweaks that allow the module var count search to act recursively, ensuring that a sitaution where something like var.top gets passed to module middle, as var.middle, and then to module bottom, as var.bottom, which is then used in a resource count.
|
LGTM |
phinze
added a commit
that referenced
this pull request
Jun 12, 2016
For `terraform destroy`, we currently build up the same graph we do for `plan` and `apply` and we do a walk with a special Diff that says "destroy everything". We have fought the interpolation subsystem time and again through this code path. Beginning in #2775 we gained a new feature to selectively prune out problematic graph nodes. The past chain of destroy fixes I have been involved with (#6557, #6599, #6753) have attempted to massage the "noop" definitions to properly handle the edge cases reported. "Variable is depended on by provider config" is another edge case we add here and try to fix. This dive only makes me more convinced that the whole `terraform destroy` code path needs to be reworked. For now, I went with a "surgical strike" approach to the problem expressed in #7047. I found a couple of issues with the existing Noop and DestroyEdgeInclude logic, especially with regards to flattening, but I'm explicitly ignoring these for now so we can get this particular bug fixed ahead of the 0.7 release. My hope is that we can circle around with a fully specced initiative to refactor `terraform destroy`'s graph to be more state-derived than config-derived. Until then, this fixes #7407
|
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
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 the same PR as #6723 but targeted at the 0.7 release rather than 0.6.17.
This PR combines the work of @phinze in #6704 and @vancluever on the paybyphone fork of Terraform.
First from @phinze:
A new problem was introduced by the prior fixes for destroy interpolation messages when resources depend on module variables with a count attribute, this makes the variable crucial for properly building the graph - even in destroys. So removing all module variables from the graph as noops was overzealous.
By borrowing the logic in DestroyEdgeInclude we are able to determine if we need to keep a given module variable relatively easily.
I'd like to overhaul the Destroy: true implementation so that it does not depend on config at all, but I want to continue for now with the targeted fixes that we can backport into the 0.6 series.
Then from @vancluever in paybyphone/terraform@c975906:
Building on b10564a, adding tweaks that allow the module var count search to act recursively, ensuring that a sitaution where something like var.top gets passed to module middle, as var.middle, and then to
module bottom, as var.bottom, which is then used in a resource count.