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

Add safe-delete workspace API, and org setting to restrict force-delete #539

Merged
merged 10 commits into from Oct 27, 2022
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,9 @@
* Add `Query` and `Status` fields to `OrganizationMembershipListOptions` to allow filtering memberships by status or username by @sebasslash [#550](https://github.com/hashicorp/go-tfe/pull/550)
* Add `ListForWorkspace` method to `VariableSets` interface to enable fetching variable sets associated with a workspace by @tstapler [#552](https://github.com/hashicorp/go-tfe/pull/552)
* Add `NotificationTriggerAssessmentDrifted` and `NotificationTriggerAssessmentFailed` notification trigger types by @lawliet89 [#542](https://github.com/hashicorp/go-tfe/pull/542)
* Add `AllowForceDeleteWorkspaces` setting to `Organizations` by @JarrettSpiker [#539](https://github.com/hashicorp/go-tfe/pull/539)
* Add `SafeDelete` and `SafeDeleteID` APIs to `Workspaces` by @JarrettSpiker [#539](https://github.com/hashicorp/go-tfe/pull/539)


## Bug Fixes
* Fix marshalling of run variables in `RunCreateOptions`. The `Variables` field type in `Run` struct has changed from `[]*RunVariable` to `[]*RunVariableAttr` by @Uk1288 [#531](https://github.com/hashicorp/go-tfe/pull/531)
Expand Down
6 changes: 6 additions & 0 deletions helper_test.go
Expand Up @@ -1599,6 +1599,12 @@ func createWorkspaceWithOptions(t *testing.T, client *Client, org *Organization,

return w, func() {
if err := client.Workspaces.Delete(ctx, org.Name, w.Name); err != nil {

// if the workspace has already been cleaned up, or destroyed in the testing process, no need to raise the alarm
if err == ErrResourceNotFound {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, a recurring pain point for us. I tried introducing this in #527 but it was ultimately rejected considering it could obfuscate an authorization bug. So unfortunately will have to reject it here -- I think we can omit the cleanup for now if an "already deleted" workspace is causing this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting...I think I would lean the other direction on that, since a test creating a workspace and then being unable to "see" it when it goes for a delete seems unlikely, but I definitely see the logic

I removed the ErrResourceNotFound check, and switched a lot of the tests to using t.Cleanup instead of defer for the cleanup callbacks, and that does seem to have cleared up most of the cleanup errors~

I also changed this to delete the workspace by ID instead of Name, because we had one test which changes the workspace name (causing this cleanup to fail)


t.Errorf("Error destroying workspace! WARNING: Dangling resources\n"+
"may exist! The full error is shown below.\n\n"+
"Workspace: %s\nError: %s", w.Name, err)
Expand Down
28 changes: 28 additions & 0 deletions mocks/workspace_mocks.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions organization.go
Expand Up @@ -78,6 +78,9 @@ type Organization struct {
TrialExpiresAt time.Time `jsonapi:"attr,trial-expires-at,iso8601"`
TwoFactorConformant bool `jsonapi:"attr,two-factor-conformant"`
SendPassingStatusesForUntriggeredSpeculativePlans bool `jsonapi:"attr,send-passing-statuses-for-untriggered-speculative-plans"`
// Note: This will be false for TFE versions older than v202211, where the setting was introduced.
// On those TFE versions, safe delete does not exist, so ALL deletes will be force deletes.
AllowForceDeleteWorkspaces bool `jsonapi:"attr,allow-force-delete-workspaces"`
}

// Capacity represents the current run capacity of an organization.
Expand Down Expand Up @@ -166,6 +169,9 @@ type OrganizationCreateOptions struct {

// Optional: SendPassingStatusesForUntriggeredSpeculativePlans toggles behavior of untriggered speculative plans to send status updates to version control systems like GitHub.
SendPassingStatusesForUntriggeredSpeculativePlans *bool `jsonapi:"attr,send-passing-statuses-for-untriggered-speculative-plans,omitempty"`

// Optional: AllowForceDeleteWorkspaces toggles behavior of allowing workspace admins to delete workspaces with resources under management.
AllowForceDeleteWorkspaces *bool `jsonapi:"attr,allow-force-delete-workspaces,omitempty"`
}

// OrganizationUpdateOptions represents the options for updating an organization.
Expand Down Expand Up @@ -202,6 +208,9 @@ type OrganizationUpdateOptions struct {

// SendPassingStatusesForUntriggeredSpeculativePlans toggles behavior of untriggered speculative plans to send status updates to version control systems like GitHub.
SendPassingStatusesForUntriggeredSpeculativePlans *bool `jsonapi:"attr,send-passing-statuses-for-untriggered-speculative-plans,omitempty"`

// Optional: AllowForceDeleteWorkspaces toggles behavior of allowing workspace admins to delete workspaces with resources under management.
AllowForceDeleteWorkspaces *bool `jsonapi:"attr,allow-force-delete-workspaces,omitempty"`
}

// ReadRunQueueOptions represents the options for showing the queue.
Expand Down
36 changes: 36 additions & 0 deletions organization_integration_test.go
Expand Up @@ -565,7 +565,43 @@ func TestOrganizationsReadRunTasksEntitlement(t *testing.T) {
assert.NotEmpty(t, entitlements.ID)
assert.True(t, entitlements.RunTasks)
})
}

func TestOrganizationsAllowForceDeleteSetting(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our docs should be much more explicit, but we've recently introduced test splitting in our CI and we're requiring all top-level tests to call skipIfNotCINode(t) as the first check. This will ensure the test runs only once on the correct test node instead of on every single node. You do not need to do this for subtests.

skipIfNotCINode(t)

client := testClient(t)
ctx := context.Background()

t.Run("creates and updates allow force delete", func(t *testing.T) {
options := OrganizationCreateOptions{
Name: String(randomString(t)),
Email: String(randomString(t) + "@tfe.local"),
AllowForceDeleteWorkspaces: Bool(true),
}

org, err := client.Organizations.Create(ctx, options)
require.NoError(t, err)

t.Cleanup(func() {
err := client.Organizations.Delete(ctx, org.Name)
if err != nil {
t.Errorf("error deleting organization (%s): %s", org.Name, err)
}
})

assert.Equal(t, *options.Name, org.Name)
assert.Equal(t, *options.Email, org.Email)
assert.True(t, org.AllowForceDeleteWorkspaces)

org, err = client.Organizations.Update(ctx, org.Name, OrganizationUpdateOptions{AllowForceDeleteWorkspaces: Bool(false)})
require.NoError(t, err)
assert.False(t, org.AllowForceDeleteWorkspaces)

org, err = client.Organizations.Read(ctx, org.Name)
require.NoError(t, err)
assert.False(t, org.AllowForceDeleteWorkspaces)
})
}

func orgItemsContainsName(items []*Organization, name string) bool {
Expand Down
66 changes: 55 additions & 11 deletions workspace.go
Expand Up @@ -50,6 +50,12 @@ type Workspaces interface {
// DeleteByID deletes a workspace by its ID.
DeleteByID(ctx context.Context, workspaceID string) error

// SafeDelete a workspace by its name.
SafeDelete(ctx context.Context, organization string, workspace string) error

// SafeDeleteByID deletes a workspace by its ID.
SafeDeleteByID(ctx context.Context, workspaceID string) error

// RemoveVCSConnection from a workspace.
RemoveVCSConnection(ctx context.Context, organization, workspace string) (*Workspace, error)

Expand Down Expand Up @@ -194,17 +200,18 @@ type WorkspaceActions struct {

// WorkspacePermissions represents the workspace permissions.
type WorkspacePermissions struct {
CanDestroy bool `jsonapi:"attr,can-destroy"`
CanForceUnlock bool `jsonapi:"attr,can-force-unlock"`
CanLock bool `jsonapi:"attr,can-lock"`
CanManageRunTasks bool `jsonapi:"attr,can-manage-run-tasks"`
CanQueueApply bool `jsonapi:"attr,can-queue-apply"`
CanQueueDestroy bool `jsonapi:"attr,can-queue-destroy"`
CanQueueRun bool `jsonapi:"attr,can-queue-run"`
CanReadSettings bool `jsonapi:"attr,can-read-settings"`
CanUnlock bool `jsonapi:"attr,can-unlock"`
CanUpdate bool `jsonapi:"attr,can-update"`
CanUpdateVariable bool `jsonapi:"attr,can-update-variable"`
CanDestroy bool `jsonapi:"attr,can-destroy"`
CanForceUnlock bool `jsonapi:"attr,can-force-unlock"`
CanLock bool `jsonapi:"attr,can-lock"`
CanManageRunTasks bool `jsonapi:"attr,can-manage-run-tasks"`
CanQueueApply bool `jsonapi:"attr,can-queue-apply"`
CanQueueDestroy bool `jsonapi:"attr,can-queue-destroy"`
CanQueueRun bool `jsonapi:"attr,can-queue-run"`
CanReadSettings bool `jsonapi:"attr,can-read-settings"`
CanUnlock bool `jsonapi:"attr,can-unlock"`
CanUpdate bool `jsonapi:"attr,can-update"`
CanUpdateVariable bool `jsonapi:"attr,can-update-variable"`
CanForceDelete *bool `jsonapi:"attr,can-force-delete"` // pointer b/c it will be useful to check if this property exists, as opposed to having it default to false
}

// WSIncludeOpt represents the available options for include query params.
Expand Down Expand Up @@ -770,6 +777,43 @@ func (s *workspaces) DeleteByID(ctx context.Context, workspaceID string) error {
return req.Do(ctx, nil)
}

// SafeDelete a workspace by its name.
func (s *workspaces) SafeDelete(ctx context.Context, organization, workspace string) error {
if !validStringID(&organization) {
return ErrInvalidOrg
}
if !validStringID(&workspace) {
return ErrInvalidWorkspaceValue
}

u := fmt.Sprintf(
"organizations/%s/workspaces/%s/actions/safe-delete",
url.QueryEscape(organization),
url.QueryEscape(workspace),
)
req, err := s.client.NewRequest("POST", u, nil)
if err != nil {
return err
}

return req.Do(ctx, nil)
}

// SafeDeleteByID safely deletes a workspace by its ID.
func (s *workspaces) SafeDeleteByID(ctx context.Context, workspaceID string) error {
if !validStringID(&workspaceID) {
return ErrInvalidWorkspaceID
}

u := fmt.Sprintf("workspaces/%s/actions/safe-delete", url.QueryEscape(workspaceID))
req, err := s.client.NewRequest("POST", u, nil)
if err != nil {
return err
}

return req.Do(ctx, nil)
}

// RemoveVCSConnection from a workspace.
func (s *workspaces) RemoveVCSConnection(ctx context.Context, organization, workspace string) (*Workspace, error) {
if !validStringID(&organization) {
Expand Down