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

[Fix] Fix deletion of dashboard if it was trashed out of band #4235

Merged
merged 1 commit into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion dashboards/resource_dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package dashboards

import (
"context"
"errors"
"log"
"strings"

"github.com/databricks/databricks-sdk-go/apierr"
"github.com/databricks/databricks-sdk-go/service/dashboards"
"github.com/databricks/terraform-provider-databricks/common"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -175,9 +177,32 @@ func ResourceDashboard() common.Resource {
if err != nil {
return err
}
return w.Lakeview.Trash(ctx, dashboards.TrashDashboardRequest{

// Attempt to trash the dashboard.
err = w.Lakeview.Trash(ctx, dashboards.TrashDashboardRequest{
DashboardId: d.Id(),
})

// If the dashboard was already trashed, we'll get a 403 (Permission Denied) error.
// There may be other cases where we get a 403, so we first confirm that the
// dashboard state is actually trashed, and if so, return success.
if errors.Is(err, apierr.ErrPermissionDenied) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. Curious -- should we also check for 404 or if a dashboard is trashed already then the only response is 403? Mentioning since the api doc https://docs.databricks.com/api/workspace/lakeview/trash has 404 as one of the responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will return 404 if it really doesn't exist (not even in the trash).

Good point, though. This fix works around the issue at deletion time, but we should also treat trashed as a 404 at read time so that if it was trashed, the next plan will show creation instead of recreation.

dashboard, nerr := w.Lakeview.Get(ctx, dashboards.GetDashboardRequest{
DashboardId: d.Id(),
})

// Return original error if we can't get the dashboard state.
if nerr != nil {
return err
}

// If the dashboard is trashed, return success.
if dashboard.LifecycleState == dashboards.LifecycleStateTrashed {
return nil
}
}

return err
},
}
}
Expand Down
79 changes: 76 additions & 3 deletions dashboards/resource_dashboard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import (
"fmt"
"testing"

"github.com/databricks/databricks-sdk-go/apierr"
"github.com/databricks/databricks-sdk-go/experimental/mocks"
"github.com/databricks/databricks-sdk-go/service/dashboards"
"github.com/databricks/terraform-provider-databricks/qa"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

Expand Down Expand Up @@ -222,9 +224,12 @@ func TestDashboardUpdate(t *testing.T) {
}

func TestDashboardDelete(t *testing.T) {
qa.ResourceFixture{
_, err := qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
w.GetMockLakeviewAPI().EXPECT().Trash(mock.Anything, dashboards.TrashDashboardRequest{
e := w.GetMockLakeviewAPI().EXPECT()

// Expect the dashboard to be trashed.
e.Trash(mock.Anything, dashboards.TrashDashboardRequest{
DashboardId: "xyz",
}).Return(nil)
},
Expand All @@ -237,5 +242,73 @@ func TestDashboardDelete(t *testing.T) {
parent_path = "/path"
serialized_dashboard = "serialized_json"
`,
}.ApplyNoError(t)
}.Apply(t)

// Expect this to succeed.
assert.NoError(t, err)
}

func TestDashboardDeletePermissionDenied(t *testing.T) {
_, err := qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
e := w.GetMockLakeviewAPI().EXPECT()

// First, expect the dashboard to be trashed.
e.Trash(mock.Anything, dashboards.TrashDashboardRequest{
DashboardId: "xyz",
}).Return(apierr.ErrPermissionDenied)

// Then, expect to get the dashboard to confirm if it was already trashed.
// We confirm below that the below error is ignored and the original error is returned.
e.Get(mock.Anything, dashboards.GetDashboardRequest{
DashboardId: "xyz",
}).Return(nil, fmt.Errorf("some other error"))
},
Resource: ResourceDashboard(),
Delete: true,
ID: "xyz",
HCL: `
display_name = "Dashboard name"
warehouse_id = "abc"
parent_path = "/path"
serialized_dashboard = "serialized_json"
`,
}.Apply(t)

// We expect the original error to be returned.
assert.Equal(t, err, apierr.ErrPermissionDenied)
}

func TestDashboardDeleteAlreadyTrashed(t *testing.T) {
_, err := qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
e := w.GetMockLakeviewAPI().EXPECT()

// First, expect the dashboard to be trashed.
e.Trash(mock.Anything, dashboards.TrashDashboardRequest{
DashboardId: "xyz",
}).Return(apierr.ErrPermissionDenied)

// Then, expect to get the dashboard to confirm if it was already trashed.
e.Get(mock.Anything, dashboards.GetDashboardRequest{
DashboardId: "xyz",
}).Return(&dashboards.Dashboard{
DashboardId: "xyz",
ParentPath: "/path",
LifecycleState: dashboards.LifecycleStateTrashed,
}, nil)
},
Resource: ResourceDashboard(),
Delete: true,
ID: "xyz",
HCL: `
display_name = "Dashboard name"
warehouse_id = "abc"
parent_path = "/path"
serialized_dashboard = "serialized_json"
`,
}.Apply(t)

// Expect this to succeed.
assert.NoError(t, err)
}
Loading