Skip to content

Consider Automatically Calling State.RemoveResource() After Successful Resource Deletion #100

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

Closed
bflad opened this issue Aug 13, 2021 · 5 comments · Fixed by #301
Closed
Assignees
Labels
enhancement New feature or request sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Aug 13, 2021

Module version

v0.2.0

Use-cases

Currently the framework requires all resource implementations to include this type of logic in the Delete() method:

func (r exampleResource) Delete(ctx context.Context, req tfsdk.DeleteResourceRequest, resp *tfsdk.DeleteResourceResponse) {
	// logic to delete resource

	resp.State.RemoveResource(ctx)
}

Is there ever a time this would not be desired on successful deletion? Maybe we can save a few keystrokes.

Proposal

Implement State.RemoveResource(ctx) in the calling code on Delete() success so it is not necessary for all implementations. From the previous framework:

https://github.com/hashicorp/terraform-plugin-sdk/blob/c455248338544ae42b6449eae44279c0c82d7987/helper/schema/resource.go#L428-L434

@bflad bflad added the enhancement New feature or request label Aug 13, 2021
@bflad
Copy link
Contributor Author

bflad commented Aug 13, 2021

Thinking further, if for some reason the behavior wasn't desired (such as certain warning-only situations maybe?), we could implement an additional field in the response, e.g.

KeepResourceState bool

@paddycarver
Copy link
Contributor

My only hesitation, personally, is that this was always (in my experience) surprising behavior in SDKv2. People had no idea this would happen, and so kept removing things from state manually. I don't know that that disqualifies it, but it gives me pause, at least.

@bflad
Copy link
Contributor Author

bflad commented Aug 18, 2021

I think we'd much rather have folks potentially doing it unnecessarily, rather than not at all though, right? Seems like we'd need to introduce error handling to catch it if it was forgotten, or something.

@paddycarver
Copy link
Contributor

I'm not sure I would. I think not doing it at all would be caught during acceptance testing, or should be caught during acceptance testing. And then at least people would be sure about what the code was doing. Doing it unnecessarily smells to me like people not knowing what their responsibilities in the framework are, and I think clear expectations are better than We Know What You Meant.

I don't know that I'm strictly against the idea, or that the convenience tradeoff of having it done automatically isn't worth the clarity tradeoff of not knowing if you need to do it or not. But I do feel like there's a tradeoff at play here and that Just Doing The Right Thing isn't free.

@paddycarver paddycarver added sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it. breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. labels Sep 9, 2021
@bflad bflad added this to the v1.0.0 milestone Apr 21, 2022
@bflad bflad removed the breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. label Apr 22, 2022
@bflad bflad modified the milestones: v1.0.0, v0.7.0 Apr 22, 2022
@bflad bflad self-assigned this Apr 22, 2022
bflad added a commit that referenced this issue Apr 22, 2022
…te method execution

Reference: #100

Due to provider developer feedback, the framework will now perform automatic removal of state on successful (no error diagnostic) `Resource` type `Delete` executions. This matches the behavior of terraform-plugin-sdk.

Providers can still explicitly call `(State).RemoveResource()` or leave existing implementations, however it is extraneous for most `Delete` logic now.
bflad added a commit that referenced this issue Apr 22, 2022
…te method execution (#301)

Reference: #100

Due to provider developer feedback, the framework will now perform automatic removal of state on successful (no error diagnostic) `Resource` type `Delete` executions. This matches the behavior of terraform-plugin-sdk.

Providers can still explicitly call `(State).RemoveResource()` or leave existing implementations, however it is extraneous for most `Delete` logic now.
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Projects
None yet
2 participants