Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
43 changes: 37 additions & 6 deletions master/internal/api_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,17 @@ func (a *apiServer) workspaceHasModels(ctx context.Context, workspaceID int32) (
return exists, nil
}

func (a *apiServer) workspaceHasExperiments(ctx context.Context, workspaceID int32) (bool, error) {
exists, err := db.Bun().NewSelect().Table("experiments").
Join("INNER JOIN projects ON experiments.project_id = projects.id").
Where("projects.workspace_id=?", workspaceID).
Copy link

Choose a reason for hiding this comment

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

Suggested change
Where("projects.workspace_id=?", workspaceID).
Where("projects.workspace_id = ?", workspaceID).

Exists(ctx)
if err != nil {
return false, fmt.Errorf("checking workspace for experiments: %w", err)
}
return exists, nil
}

func (a *apiServer) GetWorkspace(
ctx context.Context, req *apiv1.GetWorkspaceRequest,
) (*apiv1.GetWorkspaceResponse, error) {
Expand Down Expand Up @@ -1293,15 +1304,35 @@ func (a *apiServer) DeleteWorkspace(
return nil, err
}

modelsExist, err := a.workspaceHasModels(ctx, req.Id)
if err != nil {
return nil, err
checks := []struct {
checkFunc func(context.Context, int32) (bool, error)
errorStr string
}{
{a.workspaceHasModels, "workspace (%d) contains models; move or delete models first"},
Copy link

Choose a reason for hiding this comment

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

@maxrussell it looks like you took this over. can you unroll this loop?

{a.workspaceHasExperiments, "workspace (%d) contains experiments; move or delete experiments first"},
}
if modelsExist {
return nil, status.Errorf(codes.FailedPrecondition, "workspace (%d) contains models; move or delete models first",
req.Id)

for _, check := range checks {
exists, err := check.checkFunc(ctx, req.Id)
if err != nil {
return nil, err
}
if exists {
return nil, status.Errorf(codes.FailedPrecondition, check.errorStr, req.Id)
}
}

// modelsExist, err := a.workspaceHasModels(ctx, req.Id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Thanks!

// if err != nil {
// return nil, err
// }
// if modelsExist {
// return nil, status.Errorf(codes.FailedPrecondition, "workspace (%d) contains models; move or delete models first",
// req.Id)
// }

// experimentsExist, err :=

holder := &workspacev1.Workspace{}
// TODO(kristine): DET-10138 update workspace state in transaction with template delete
err = a.m.db.QueryProto("deletable_workspace", holder, req.Id)
Expand Down
41 changes: 40 additions & 1 deletion master/internal/api_workspace_intg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"strconv"
"testing"
"time"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -939,7 +940,7 @@ func TestDeleteWorkspace(t *testing.T) {
mockRM := MockRM()
noName := ""
// set up api server
api, _, ctx := setupAPITest(t, nil, mockRM)
api, curUser, ctx := setupAPITest(t, nil, mockRM)
api.m.allRms = map[string]rm.ResourceManager{noName: mockRM}
// set up command service - required for successful DeleteWorkspaceRequest calls
cs, err := command.NewService(api.m.db, api.m.rm)
Expand Down Expand Up @@ -1002,6 +1003,44 @@ func TestDeleteWorkspace(t *testing.T) {
Id: resp.Workspace.Id,
})
require.Error(t, err)

// create another workspace, and add a experiment
Copy link

Choose a reason for hiding this comment

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

nit, put the new test in a separate test.

w, p := createProjectAndWorkspace(ctx, t, api)
err = db.Bun().RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {
autoGeneratedNamespaceName, err := getAutoGeneratedNamespaceName(ctx, w, &tx)
require.NoError(t, err)
autoCreatedNamespace = autoGeneratedNamespaceName
return nil
})
require.NoError(t, err)
e := createTestExpWithProjectID(t, api, curUser, p)
_, err = db.Bun().NewUpdate().Table("experiments").
Set("state = ?", model.CompletedState).
Where("id = ?", e.ID).Exec(ctx)
require.NoError(t, err)
_, err = api.DeleteWorkspace(ctx, &apiv1.DeleteWorkspaceRequest{
Id: int32(w),
})

require.Error(t, err)
// delete experiment, so that workspace can be deleted
_, err = api.DeleteExperiment(ctx, &apiv1.DeleteExperimentRequest{ExperimentId: int32(e.ID)})
require.NoError(t, err)

// since delete experiment is async, we need to wait for the experiment to be deleted
for i := 0; i < 10; i++ {
_, err = api.GetExperiment(ctx, &apiv1.GetExperimentRequest{ExperimentId: int32(e.ID)})
if err != nil {
break
}
time.Sleep(1 * time.Second)
}
// delete the workspace successfully
mockRM.On("DeleteNamespace", *autoCreatedNamespace).Return(nil).Once()
_, err = api.DeleteWorkspace(ctx, &apiv1.DeleteWorkspaceRequest{
Id: int32(w),
})
require.NoError(t, err)
}

func TestSetWorkspaceNamespaceBindings(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export const useWorkspaceActionMenu: (props: WorkspaceMenuPropsIn) => WorkspaceM
} else if (!workspace.archived) {
menuItems.push({ key: MenuKey.Edit, label: 'View Config' });
}
if (canDeleteWorkspace({ workspace }) && workspace.numExperiments === 0) {
if (canDeleteWorkspace({ workspace })) {
menuItems.push({ type: 'divider' });
menuItems.push({ danger: true, key: MenuKey.Delete, label: 'Delete...' });
}
Expand Down
Loading