Use safe or force workspace delete for cloud backend #31949
Conversation
JarrettSpiker
requested review from
jrhouston,
alexsomesan and
a team
as code owners
| @@ -109,7 +109,7 @@ type Backend interface { | |||
| // DeleteWorkspace cannot prevent deleting a state that is in use. It is | |||
| // the responsibility of the caller to hold a Lock for the state manager | |||
| // belonging to this workspace before calling this method. | |||
| DeleteWorkspace(name string) error | |||
| DeleteWorkspace(name string, force bool) error | |||
There was a problem hiding this comment.
I dont love adding magic boolean arguments to interfaces, especially when it is only used in one of the backends. I can justify it here because "force deleting" is an existing concept for workspace deleting, which just hasnt been handled by the backends up until now.
However, any suggestions on how to pass this force vs safe deletion boolean to the cloud backend are welcome
There was a problem hiding this comment.
I think this is what we like to see. All backends should share the same interface, even if they don't all behave exactly the same.
| @@ -582,7 +582,7 @@ func (b *Remote) WorkspaceNamePattern() string { | |||
| } | |||
|
|
|||
| // DeleteWorkspace implements backend.Enhanced. | |||
| func (b *Remote) DeleteWorkspace(name string) error { | |||
| func (b *Remote) DeleteWorkspace(name string, _ bool) error { | |||
There was a problem hiding this comment.
I am not 100% sure if it makes sense to also add usage of the Safe Delete API to the remote backend (instead of just the cloud backend). I have not in this PR so far, but input is appreciated
There was a problem hiding this comment.
I don't think it's strictly necessary to backport your changes from cloud to backend "remote" but it depends on how many terraform users you'd like to reach with this feature.
There was a problem hiding this comment.
My instinct then is to leave remote as it is, since:
- we recommend
cloudinstead - the CLI already does some safe vs force delete checks, this change is just some additional guards. So having the
remotebackend continue to fallback to the existing force-delete APIs seems 👌
|
|
||
| var err error | ||
|
|
||
| isSafeDeleteSupported := s.workspace.Permissions.CanForceDelete != nil |
There was a problem hiding this comment.
You can temporarily update go.mod with the branch version of go-tfe to test this while it's a PR. Just be sure to get go-tfe merged and released first and reset the go.mod dependency to the new version before merging this PR.
| } else { | ||
| err = s.tfeClient.Workspaces.SafeDelete(context.Background(), s.organization, s.workspace.Name) | ||
| } | ||
|
|
||
| if err != nil && err != tfe.ErrResourceNotFound { |
There was a problem hiding this comment.
You can see here that we ignore 404 errors when deleting the workspace, and I'm wondering if we should do the same thing when reading the workspace before deleting it in internal/cloud/backend.go...
There was a problem hiding this comment.
That is very interesting...I guess if the workspace cant be read, we would have failed earlier in the command code here
Im not really sure what makes the most sense for this interface in isolation, but for consistency I am happy to change it to return successfully if the workspace cant be found once we are attempting the delete
JarrettSpiker
force-pushed
the
jspiker/safe-delete-workspace
branch
from
cf761dd
to
0a940f8
Compare
JarrettSpiker
force-pushed
the
jspiker/safe-delete-workspace
branch
2 times, most recently
from
946a213
to
4c6ddc1
Compare
JarrettSpiker
force-pushed
the
jspiker/safe-delete-workspace
branch
from
4c6ddc1
to
c28aaa2
Compare
JarrettSpiker
force-pushed
the
jspiker/safe-delete-workspace
branch
from
c28aaa2
to
0ae6eff
Compare
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
Updates the cloud backend to call either workspace safe-delete or workspace force-delete
These are new concepts to the Terraform Cloud workspace deletion APIs (proposed documentation here). Workspace safe delete will only remove a workspace if it does not have resources under management, to help prevent users from accidentally leaving managed resources orphaned from Terraform.
If a CLI user does not pass the
-forceoption toterraform workspace delete, the cloud backend will attempt to use the new workspace safe-delete API (if we can detect that the TFC/E version contains this API). Otherwise it will continue using the existing API, which functions as a force delete.The CLI already has this concept of force workspace deletion, and will not remove a workspace if it detects it is managing resources (unless the user passes the
-forceflag). So this just acts as an extra set of guardrails around workspace deletion without the-forceflag, since the safe delete API can make some additional checks around whether the workspace in question is locked, or if its state is in the process of being updated.Relevant RFC
This depends on the go-tfe change here
Draft CHANGELOG entry
ENHANCEMENTS
When using the cloud backend
terraform workspace deletewill now perform additional checks to ensure that the workspace being deleted is not managing state, such as checking if the workspace is locked or if its state is in the process of being updated, unless the-forceoption is passed.