mirror of https://github.com/databricks/cli.git
Restore original 404 handling logic
Based on reviewer feedback we want to minimize assumptions about API behavior
This commit is contained in:
parent
5ed6fc4b46
commit
26c3b421e8
|
@ -29,7 +29,7 @@ func (m *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
|
||||||
b.Files, err = sync.RunOnce(ctx)
|
b.Files, err = sync.RunOnce(ctx)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if errors.Is(err, fs.ErrPermission) {
|
if errors.Is(err, fs.ErrPermission) {
|
||||||
return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath)
|
return permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath)
|
||||||
}
|
}
|
||||||
return diag.FromErr(err)
|
return diag.FromErr(err)
|
||||||
}
|
}
|
||||||
|
|
|
@ -8,6 +8,7 @@ import (
|
||||||
"github.com/databricks/cli/bundle"
|
"github.com/databricks/cli/bundle"
|
||||||
"github.com/databricks/cli/bundle/permissions"
|
"github.com/databricks/cli/bundle/permissions"
|
||||||
"github.com/databricks/cli/libs/diag"
|
"github.com/databricks/cli/libs/diag"
|
||||||
|
"github.com/databricks/cli/libs/filer"
|
||||||
"github.com/databricks/cli/libs/locker"
|
"github.com/databricks/cli/libs/locker"
|
||||||
"github.com/databricks/cli/libs/log"
|
"github.com/databricks/cli/libs/log"
|
||||||
)
|
)
|
||||||
|
@ -53,7 +54,14 @@ func (m *acquire) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics
|
||||||
log.Errorf(ctx, "Failed to acquire deployment lock: %v", err)
|
log.Errorf(ctx, "Failed to acquire deployment lock: %v", err)
|
||||||
|
|
||||||
if errors.Is(err, fs.ErrPermission) {
|
if errors.Is(err, fs.ErrPermission) {
|
||||||
return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath)
|
return permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath)
|
||||||
|
}
|
||||||
|
|
||||||
|
notExistsError := filer.NoSuchDirectoryError{}
|
||||||
|
if errors.As(err, ¬ExistsError) {
|
||||||
|
// If we get a "doesn't exist" error from the API this indicates
|
||||||
|
// we either don't have permissions or the path is invalid.
|
||||||
|
return diag.Errorf("cannot write to deployment root (this can indicate a previous deploy was done with a different identity): %s", b.Config.Workspace.RootPath)
|
||||||
}
|
}
|
||||||
|
|
||||||
return diag.FromErr(err)
|
return diag.FromErr(err)
|
||||||
|
|
|
@ -109,20 +109,24 @@ func isGroupOfCurrentUser(b *bundle.Bundle, groupName string) bool {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
func ReportPermissionDenied(ctx context.Context, b *bundle.Bundle, path string) diag.Diagnostics {
|
// ReportPossiblePermissionDenied generates a diagnostic message when a permission denied error is encountered.
|
||||||
log.Errorf(ctx, "Failed to update, encountated permission denied error: %v", path)
|
//
|
||||||
|
// Note that since the workspace API doesn't always distinguish between permission denied and path errors,
|
||||||
|
// we must treat this as a "possible permission error". See acquire.go for more about this.
|
||||||
|
func ReportPossiblePermissionDenied(ctx context.Context, b *bundle.Bundle, path string) diag.Diagnostics {
|
||||||
|
log.Errorf(ctx, "Failed to update, encountered possible permission error: %v", path)
|
||||||
|
|
||||||
user := b.Config.Workspace.CurrentUser.DisplayName
|
user := b.Config.Workspace.CurrentUser.DisplayName
|
||||||
canManageBundle, assistance := analyzeBundlePermissions(b)
|
canManageBundle, assistance := analyzeBundlePermissions(b)
|
||||||
|
|
||||||
if !canManageBundle {
|
if !canManageBundle {
|
||||||
return diag.Diagnostics{{
|
return diag.Diagnostics{{
|
||||||
Summary: fmt.Sprintf("deployment permission denied for %s.\n"+
|
Summary: fmt.Sprintf("unable to deploy to %s as %s.\n"+
|
||||||
"Please make sure the current user or one of their groups is listed under the permissions of this bundle.\n"+
|
"Please make sure the current user or one of their groups is listed under the permissions of this bundle.\n"+
|
||||||
"%s\n"+
|
"%s\n"+
|
||||||
"They may need to redeploy the bundle to apply the new permissions.\n"+
|
"They may need to redeploy the bundle to apply the new permissions.\n"+
|
||||||
"Please refer to https://docs.databricks.com/dev-tools/bundles/permissions.html for more on managing permissions.",
|
"Please refer to https://docs.databricks.com/dev-tools/bundles/permissions.html for more on managing permissions.",
|
||||||
user, assistance),
|
path, user, assistance),
|
||||||
Severity: diag.Error,
|
Severity: diag.Error,
|
||||||
ID: diag.PathPermissionDenied,
|
ID: diag.PathPermissionDenied,
|
||||||
}}
|
}}
|
||||||
|
@ -132,7 +136,7 @@ func ReportPermissionDenied(ctx context.Context, b *bundle.Bundle, path string)
|
||||||
// But we're still seeing permission errors. So someone else will need
|
// But we're still seeing permission errors. So someone else will need
|
||||||
// to redeploy the bundle with the right set of permissions.
|
// to redeploy the bundle with the right set of permissions.
|
||||||
return diag.Diagnostics{{
|
return diag.Diagnostics{{
|
||||||
Summary: fmt.Sprintf("access denied while updating deployment permissions for %s.\n"+
|
Summary: fmt.Sprintf("access denied while updating deployment permissions as %s.\n"+
|
||||||
"%s\n"+
|
"%s\n"+
|
||||||
"They can redeploy the project to apply the latest set of permissions.\n"+
|
"They can redeploy the project to apply the latest set of permissions.\n"+
|
||||||
"Please refer to https://docs.databricks.com/dev-tools/bundles/permissions.html for more on managing permissions.",
|
"Please refer to https://docs.databricks.com/dev-tools/bundles/permissions.html for more on managing permissions.",
|
||||||
|
|
|
@ -46,7 +46,7 @@ func TestPermissionDiagnosticsPermissionDeniedWithPermission(t *testing.T) {
|
||||||
{Level: "CAN_MANAGE", GroupName: "testgroup"},
|
{Level: "CAN_MANAGE", GroupName: "testgroup"},
|
||||||
})
|
})
|
||||||
|
|
||||||
diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath")
|
diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath")
|
||||||
require.ErrorContains(t, diags.Error(), string(diag.CannotChangePathPermissions))
|
require.ErrorContains(t, diags.Error(), string(diag.CannotChangePathPermissions))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -55,14 +55,14 @@ func TestPermissionDiagnosticsPermissionDeniedWithoutPermission(t *testing.T) {
|
||||||
{Level: "CAN_VIEW", UserName: "testuser@databricks.com"},
|
{Level: "CAN_VIEW", UserName: "testuser@databricks.com"},
|
||||||
})
|
})
|
||||||
|
|
||||||
diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath")
|
diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath")
|
||||||
require.ErrorContains(t, diags.Error(), string(diag.PathPermissionDenied))
|
require.ErrorContains(t, diags.Error(), string(diag.PathPermissionDenied))
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestPermissionDiagnosticsPermissionDeniedNilPermission(t *testing.T) {
|
func TestPermissionDiagnosticsPermissionDeniedNilPermission(t *testing.T) {
|
||||||
b := mockBundle(nil)
|
b := mockBundle(nil)
|
||||||
|
|
||||||
diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath")
|
diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath")
|
||||||
require.ErrorContains(t, diags.Error(), string(diag.PathPermissionDenied))
|
require.ErrorContains(t, diags.Error(), string(diag.PathPermissionDenied))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -72,7 +72,7 @@ func TestPermissionDiagnosticsFindOtherOwners(t *testing.T) {
|
||||||
{Level: "CAN_MANAGE", UserName: "alice@databricks.com"},
|
{Level: "CAN_MANAGE", UserName: "alice@databricks.com"},
|
||||||
})
|
})
|
||||||
|
|
||||||
diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath")
|
diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath")
|
||||||
require.ErrorContains(t, diags.Error(), "include: alice@databricks.com")
|
require.ErrorContains(t, diags.Error(), "include: alice@databricks.com")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -3,7 +3,6 @@ package config_tests
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"fmt"
|
"fmt"
|
||||||
"path/filepath"
|
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/databricks/cli/bundle"
|
"github.com/databricks/cli/bundle"
|
||||||
|
|
|
@ -185,14 +185,7 @@ func (w *workspaceFilesClient) Write(ctx context.Context, name string, reader io
|
||||||
}
|
}
|
||||||
|
|
||||||
// Retry without CreateParentDirectories mode flag.
|
// Retry without CreateParentDirectories mode flag.
|
||||||
err = w.Write(ctx, name, bytes.NewReader(body), sliceWithout(mode, CreateParentDirectories)...)
|
return w.Write(ctx, name, bytes.NewReader(body), sliceWithout(mode, CreateParentDirectories)...)
|
||||||
if errors.Is(err, fs.ErrNotExist) {
|
|
||||||
// If we still get a 404 error when the dir exists,
|
|
||||||
// the problem is a permission error.
|
|
||||||
return PermissionError{absPath}
|
|
||||||
}
|
|
||||||
return err
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// This API returns 409 if the file already exists, when the object type is file
|
// This API returns 409 if the file already exists, when the object type is file
|
||||||
|
@ -309,8 +302,8 @@ func (w *workspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.D
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|
||||||
// This API returns a 404 if the specified path does not exist,
|
// NOTE: This API returns a 404 if the specified path does not exist,
|
||||||
// or if we don't have access to write to the path.
|
// but can also do so if we don't have access to write to the path.
|
||||||
if aerr.StatusCode == http.StatusNotFound {
|
if aerr.StatusCode == http.StatusNotFound {
|
||||||
return nil, NoSuchDirectoryError{path.Dir(absPath)}
|
return nil, NoSuchDirectoryError{path.Dir(absPath)}
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue