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

[#TF-722] Allow safe and force delete through the tfe provider #675

Merged
merged 5 commits into from
Nov 17, 2022

Conversation

emlanctot
Copy link
Contributor

@emlanctot emlanctot commented Oct 26, 2022

Description

Adds the force_delete attribute to a workspace provider resource.

External links

Output from acceptance tests

Please run applicable acceptance tests locally and include the output here. See TESTS.md to learn how to run acceptance tests.

If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.

/opt/homebrew/Cellar/go/1.19.2/libexec/bin/go tool test2json -t /private/var/folders/2z/33zp571x44v_ndv_hk8cyy1w0000gp/T/GoLand/___TestTFEWorkspace_delete_withoutCanForceDeletePermission_in_github_com_hashicorp_terraform_provider_tfe_tfe.test -test.v -test.paniconexit0 -test.run ^\QTestTFEWorkspace_delete_withoutCanForceDeletePermission\E$
=== RUN   TestTFEWorkspace_delete_withoutCanForceDeletePermission
2022/11/08 09:43:52 [DEBUG] Delete workspace ws-testing
2022/11/08 09:43:52 [DEBUG] Delete workspace ws-testing
--- PASS: TestTFEWorkspace_delete_withoutCanForceDeletePermission (0.37s)
PASS

Process finished with the exit code 0

...


@emlanctot emlanctot changed the title El/tf 722 [#TF-722] Allow safe and force delete through the tfe provider Oct 27, 2022
@emlanctot emlanctot force-pushed the el/tf-722 branch 2 times, most recently from 0c53b4f to 321be76 Compare October 31, 2022 22:46
@emlanctot emlanctot marked this pull request as ready for review October 31, 2022 22:49
@emlanctot emlanctot requested a review from a team as a code owner October 31, 2022 22:49
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Not smoke tested yet but this is looking pretty good!

go.mod Outdated Show resolved Hide resolved
tfe/resource_tfe_workspace.go Outdated Show resolved Hide resolved
@emlanctot emlanctot force-pushed the el/tf-722 branch 8 times, most recently from e658690 to c137aa9 Compare November 9, 2022 22:12
@emlanctot emlanctot force-pushed the el/tf-722 branch 4 times, most recently from 2cbd1cf to 7dbdcc9 Compare November 9, 2022 23:22
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

I really appreciate the unit testing you added to delete because it can be difficult to set up the prerequisites for a conflict

I feel like the behavior of force_delete is unique enough to warrant mentioning in the error message because it's a meta argument. What do you think?

This is a great contribution, thanks!

tfe/resource_tfe_workspace.go Outdated Show resolved Hide resolved
tfe/resource_tfe_workspace_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@SwiftEngineer SwiftEngineer left a comment

Choose a reason for hiding this comment

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

I only had one suggestion about ways of unnesting some if statements, the actual logic in this PR is probably the only thing I feel confident in checking and it seems like you nailed it to me 💯 !

tfe/resource_tfe_workspace.go Show resolved Hide resolved
tfe/resource_tfe_workspace.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Great! Thanks. I think the linter issue looks straightforward as well.

tfe/resource_tfe_workspace.go Outdated Show resolved Hide resolved
@ktham
Copy link

ktham commented Nov 17, 2022

This is great, if we can cut a release soon, that would be super awesome!

Comment on lines +809 to +816
func errWorkspaceSafeDeleteWithPermission(workspaceID string, err error) error {
if err != nil {
if strings.HasPrefix(err.Error(), "conflict") {
return fmt.Errorf("Error deleting workspace %s: %w\nTo delete this workspace without destroying the managed resources, add force_delete = true to the resource config.", workspaceID, err)
}
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the logic here seems wrong; if there's an error, and it's not a conflict, this function should still return the err value. i saw this behavior today when i destroyed a terraform cloud workspace, and the workspace wasn't actually deleted but the apply succeeded and the resource was removed from state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants