Skip to content

don't refresh deposed instances during the destroy plan#29720

Merged
jbardin merged 4 commits intomainfrom
jbardin/destroy-plan-deposed
Oct 8, 2021
Merged

don't refresh deposed instances during the destroy plan#29720
jbardin merged 4 commits intomainfrom
jbardin/destroy-plan-deposed

Conversation

@jbardin
Copy link
Copy Markdown
Member

@jbardin jbardin commented Oct 8, 2021

The NodePlanDeposedResourceInstanceObject is used in both a regular plan, and in a destroy plan, because the only action needed for a deposed instance is to destroy it so the functionality is mostly identical.

A major difference between these two plans however is that a regular plan can refresh instances, while a destroy plan is done without any interaction from the provider. Since NodePlanDeposedResourceInstanceObject is also used for a normal plan, it contains the code to refresh the instance, which needs to be skipped during a destroy plan.

The other instance types don't have this problem because they have different plan node types for plan destroy. We could create a new type for NodePlanDestroyDeposedResourceInstanceObject, but the only difference would be missing refresh call, so for now we can just add another conditional based on the walk operation.

In order to try and prevent similar issues, the MockProvider is updated to check that the provider is configured for all calls that may require it, and the *Called fields are zeroed out when the provider is reused.

Fixes #29712

Have the MockProvider ensure that Configure is always called before any
methods that may require a configured provider.

Ensure the MockProvider *Called fields are zeroed out when re-using the
provider instance.
The destroy plan should not require a configured provider (the complete
configuration is not evaluated, so they cannot be configured).

Deposed instances were being refreshed during the destroy plan, because
this instance type is only ever destroyed and shares the same
implementation between plan and walkPlanDestroy. Skip refreshing during
walkPlanDestroy.
Resetting the *Called fields and enforcing configuration broke a few
tests.
Copy link
Copy Markdown
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

I can confirm terraform destroy on 1.1.0-alpha20211006 does not work in this scenario, while this branch does. 🎉

@jbardin jbardin requested a review from a team October 8, 2021 19:24
@jbardin
Copy link
Copy Markdown
Member Author

jbardin commented Oct 8, 2021

This will require a manual backport of the NodePlanDeposedResourceInstanceObject change and associated test for v1.0 due to major refactoring of the codebase.

@jbardin
Copy link
Copy Markdown
Member Author

jbardin commented Oct 8, 2021

go1.17.2 made a change that updates the go.mod format. I'll address that in a separate PR, but the go-checks test will fail for now.

@jbardin jbardin changed the title don't refresh instances during the destroy plan don't refresh deposed instances during the destroy plan Oct 8, 2021
@jbardin jbardin merged commit 3c942ee into main Oct 8, 2021
@jbardin jbardin deleted the jbardin/destroy-plan-deposed branch October 8, 2021 20:24
jbardin added a commit that referenced this pull request Oct 12, 2021
v1.0 backport of #29720: skip refreshing deposed during destroy plan
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 8, 2021

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 Nov 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NodeApplyableProvider Does Not Call ConfigureProvider for Destroy Plan With Deposed Instances

3 participants