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

always evaluate module outputs during destroy #33462

Merged
merged 1 commit into from Jul 7, 2023

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jun 30, 2023

A module output is generally not used during destroy, however it must be evaluated when its value is used by a provider for configuration, because that configuration is not stored between walks.

There was an oversight in the output expansion node where the output node was not created because the operation was destroy, and module outputs have nothing to destroy. This however skipped evaluation when the output is needed by a provider as mentioned above. Because of the way an implied plan is stored internally when executing terraform destroy, this went unnoticed by the test.

Allowing the output to be evaluated during destroy fixes the issue, and should be acceptable because an output is classified as temporary in the graph, and will be pruned when not actually needed.

Update the existing test to serialize the plan, which triggers the failure.

Fixes #33455

A module output is generally not used during destroy, however it must be
evaluated when its value is used by a provider for configuration,
because that configuration is not stored between walks.

There was an oversight in the output expansion node where the output
node was not created because the operation was destroy, and module
outputs have nothing to destroy. This however skipped evaluation when
the output is needed by a provider as mentioned above. Because of the
way an implied plan is stored internally when executing `terraform
destroy`, this went unnoticed by the test.

Allowing the output to be evaluated during destroy fixes the issue, and
should be acceptable because an output is classified as temporary in the
graph, and will be pruned when not actually needed.

Update the existing test to serialize the plan, which triggers the
failure.
@jbardin jbardin added the 1.5-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Jun 30, 2023
@jbardin jbardin requested a review from a team June 30, 2023 19:33
@jbardin jbardin merged commit c42d3b4 into main Jul 7, 2023
6 checks passed
@jbardin jbardin deleted the jbardin/destroy-output-provider-refs branch July 7, 2023 13:44
@github-actions
Copy link

github-actions bot commented Jul 7, 2023

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

Copy link

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 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.5-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.

module outputs may not evaluated when applying a destroy plan
2 participants