diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index 632bce1ce..f6b33a83c 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "slices" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/libraries" @@ -24,20 +25,37 @@ func (v *validateArtifactPath) Name() string { return "validate:artifact_paths" } +func extractVolumeFromPath(artifactPath string) (string, string, string, error) { + if !libraries.IsVolumesPath(artifactPath) { + return "", "", "", fmt.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) + } + + parts := strings.Split(artifactPath, "/") + volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes////..., got %s", artifactPath) + + // Incorrect format. + if len(parts) < 5 { + return "", "", "", volumeFormatErr + } + + catalogName := parts[2] + schemaName := parts[3] + volumeName := parts[4] + + // Incorrect format. + if catalogName == "" || schemaName == "" || volumeName == "" { + return "", "", "", volumeFormatErr + } + + return catalogName, schemaName, volumeName, nil +} + func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { // We only validate UC Volumes paths right now. if !libraries.IsVolumesPath(rb.Config().Workspace.ArtifactPath) { return nil } - catalogName, schemaName, volumeName, err := libraries.ExtractVolumeFromPath(rb.Config().Workspace.ArtifactPath) - if err != nil { - return diag.FromErr(err) - } - volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) - w := rb.WorkspaceClient() - p, err := w.Grants.GetEffectiveBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, volumeFullName) - wrapErrorMsg := func(s string) diag.Diagnostics { return diag.Diagnostics{ { @@ -48,6 +66,15 @@ func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBund }, } } + + catalogName, schemaName, volumeName, err := extractVolumeFromPath(rb.Config().Workspace.ArtifactPath) + if err != nil { + return wrapErrorMsg(err.Error()) + } + volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) + w := rb.WorkspaceClient() + p, err := w.Grants.GetEffectiveBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, volumeFullName) + if errors.Is(err, apierr.ErrPermissionDenied) { return wrapErrorMsg(fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err)) } diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index 26e58bb9e..76e347513 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -1,13 +1,13 @@ -package validate_test +package validate import ( "context" + "fmt" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" @@ -16,6 +16,7 @@ import ( "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { @@ -50,7 +51,7 @@ func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { }) b.SetWorkpaceClient(m.WorkspaceClient) - diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), validate.ValidateArtifactPath()) + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), ValidateArtifactPath()) assert.Equal(t, diag.Diagnostics{{ Severity: diag.Error, Summary: "volume catalogN.schemaN.volumeN does not exist", @@ -149,7 +150,7 @@ func TestValidateArtifactPath(t *testing.T) { api.EXPECT().GetEffectiveBySecurableTypeAndFullName(mock.Anything, catalog.SecurableTypeVolume, "catalogN.schemaN.volumeN").Return(tc.permissions, tc.err) b.SetWorkpaceClient(m.WorkspaceClient) - diags := bundle.ApplyReadOnly(ctx, rb, validate.ValidateArtifactPath()) + diags := bundle.ApplyReadOnly(ctx, rb, ValidateArtifactPath()) if tc.expectedSummary != "" { assertDiags(t, diags, tc.expectedSummary) } else { @@ -157,3 +158,52 @@ func TestValidateArtifactPath(t *testing.T) { } } } + +func invalidVolumePaths() []string { + return []string{ + "/Volumes/", + "/Volumes/main", + "/Volumes/main/", + "/Volumes/main//", + "/Volumes/main//my_schema", + "/Volumes/main/my_schema", + "/Volumes/main/my_schema/", + "/Volumes/main/my_schema//", + "/Volumes//my_schema/my_volume", + } +} + +func TestExtractVolumeFromPath(t *testing.T) { + catalogName, schemaName, volumeName, err := extractVolumeFromPath("/Volumes/main/my_schema/my_volume") + require.NoError(t, err) + assert.Equal(t, "main", catalogName) + assert.Equal(t, "my_schema", schemaName) + assert.Equal(t, "my_volume", volumeName) + + for _, p := range invalidVolumePaths() { + _, _, _, err := extractVolumeFromPath(p) + assert.EqualError(t, err, fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p)) + } +} + +func TestValidateArtifactPathWithInvalidPaths(t *testing.T) { + for _, p := range invalidVolumePaths() { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: p, + }, + }, + } + + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), ValidateArtifactPath()) + require.Equal(t, diags, diag.Diagnostics{{ + Severity: diag.Error, + Summary: fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p), + Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }}) + } +} diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index b1ba4b769..e356d9b82 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "path" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" @@ -14,31 +13,6 @@ import ( "github.com/databricks/cli/libs/filer" ) -func ExtractVolumeFromPath(artifactPath string) (string, string, string, error) { - if !IsVolumesPath(artifactPath) { - return "", "", "", fmt.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) - } - - parts := strings.Split(artifactPath, "/") - volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes////..., got %s", artifactPath) - - // Incorrect format. - if len(parts) < 5 { - return "", "", "", volumeFormatErr - } - - catalogName := parts[2] - schemaName := parts[3] - volumeName := parts[4] - - // Incorrect format. - if catalogName == "" || schemaName == "" || volumeName == "" { - return "", "", "", volumeFormatErr - } - - return catalogName, schemaName, volumeName, nil -} - func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { w := b.WorkspaceClient() uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 9855ee3cb..c788ac919 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -185,42 +185,6 @@ the artifact_path.`, }, diags) } -func invalidVolumePaths() []string { - return []string{ - "/Volumes/", - "/Volumes/main", - "/Volumes/main/", - "/Volumes/main//", - "/Volumes/main//my_schema", - "/Volumes/main/my_schema", - "/Volumes/main/my_schema/", - "/Volumes/main/my_schema//", - "/Volumes//my_schema/my_volume", - } -} - -func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { - for _, p := range invalidVolumePaths() { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: p, - }, - }, - } - - bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) - - _, _, diags := GetFilerForLibraries(context.Background(), b) - require.Equal(t, diags, diag.Diagnostics{{ - Severity: diag.Error, - Summary: fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p), - Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - }}) - } -} - func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ @@ -262,16 +226,3 @@ func TestFilerForVolumeWithValidVolumePaths(t *testing.T) { assert.IsType(t, &filer.FilesClient{}, client) } } - -func TestExtractVolumeFromPath(t *testing.T) { - catalogName, schemaName, volumeName, err := ExtractVolumeFromPath("/Volumes/main/my_schema/my_volume") - require.NoError(t, err) - assert.Equal(t, "main", catalogName) - assert.Equal(t, "my_schema", schemaName) - assert.Equal(t, "my_volume", volumeName) - - for _, p := range invalidVolumePaths() { - _, _, _, err := ExtractVolumeFromPath(p) - assert.EqualError(t, err, fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p)) - } -}