diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 7053a44f0..07fbd56b9 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -129,15 +129,13 @@ func collectLocalLibraries(b *bundle.Bundle) (map[string][]configLocation, error } func (u *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // If the client is not initialized, initialize it - // We use client field in mutator to allow for mocking client in testing client, uploadPath, diags := GetFilerForLibraries(ctx, b) if diags.HasError() { return diags } - // Only set the filer client if it's not already set. This allows for using - // a mock client in tests. + // Only set the filer client if it's not already set. We use client field + // in mutator to mock the filer client in testing if u.client == nil { u.client = client } @@ -191,7 +189,7 @@ func (u *upload) Name() string { return "libraries.Upload" } -// This function returns the right filer to use, to upload artifacts to the configured locations. +// This function returns the right filer to use, to upload artifacts to the configured location. // Supported locations: // 1. WSFS // 2. UC volumes @@ -200,7 +198,7 @@ func (u *upload) Name() string { // Then: // 1. If the UC volume existing in the workspace: // Returns a filer for the UC volume. -// 2. If the UC volume does not exist in the workspace but is very likely to be defined in +// 2. If the UC volume does not exist in the workspace but is (with high confidence) defined in // the bundle configuration: // Returns a warning along with the error that instructs the user to deploy the // UC volume before using it in the artifact path. @@ -212,11 +210,11 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, s return nil, "", diag.Errorf("remote artifact path not configured") } - // path to upload artifact files to. - uploadPath := path.Join(artifactPath, ".internal") - w := b.WorkspaceClient() - isVolumesPath := strings.HasPrefix(uploadPath, "/Volumes/") + isVolumesPath := strings.HasPrefix(artifactPath, "/Volumes/") + + // Path to upload artifact files to. + uploadPath := path.Join(artifactPath, ".internal") // Return early with a WSFS filer if the artifact path is not a UC volume path. if !isVolumesPath { @@ -225,7 +223,7 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, s } parts := strings.Split(artifactPath, "/") - volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes///, got %s", uploadPath) + volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes////..., got %s", uploadPath) // Incorrect format. if len(parts) < 5 { @@ -253,21 +251,21 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, s diags := diag.Errorf("failed to fetch metadata for the UC volume %s that is configured in the artifact_path: %s", volumePath, err) - path, locations, ok := matchVolumeInBundle(b, catalogName, schemaName, volumeName) + path, locations, ok := findVolumeInBundle(b, catalogName, schemaName, volumeName) if !ok { return nil, "", diags } warning := diag.Diagnostic{ Severity: diag.Warning, - Summary: `the UC volume that is likely being used in the artifact_path has not been deployed yet. Please deploy the UC volume in a separate bundle deploy before using it in the artifact_path.`, + Summary: `You might be using a UC volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please deploy the UC volume in a separate bundle deploy before using it in the artifact_path.`, Locations: locations, Paths: []dyn.Path{path}, } return nil, "", diags.Append(warning) } -func matchVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { +func findVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { volumes := b.Config.Resources.Volumes for k, v := range volumes { if v.CatalogName != catalogName || v.Name != volumeName { @@ -277,7 +275,7 @@ func matchVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName s // at runtime via the ${resources.schemas.} syntax. Thus we match the volume // definition if the schema name is the same as the one in the bundle, or if the // schema name is interpolated. - if v.SchemaName != schemaName && !dynvar.ContainsVariableReference(v.SchemaName) { + if v.SchemaName != schemaName && !dynvar.IsPureVariableReference(v.SchemaName) { continue } pathString := fmt.Sprintf("resources.volumes.%s", k) diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 1951105c6..6ee72d4e3 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -344,7 +344,7 @@ func TestUploadMultipleLibraries(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies, "/Workspace/Users/foo@bar.com/mywheel.whl") } -func TestMatchVolumeInBundle(t *testing.T) { +func TestFindVolumeInBundle(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Resources: config.Resources{ @@ -364,7 +364,7 @@ func TestMatchVolumeInBundle(t *testing.T) { bundletest.SetLocation(b, "resources.volumes.foo", "volume.yml") // volume is in DAB. - path, locations, ok := matchVolumeInBundle(b, "main", "my_schema", "my_volume") + path, locations, ok := findVolumeInBundle(b, "main", "my_schema", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ File: "volume.yml", @@ -372,16 +372,20 @@ func TestMatchVolumeInBundle(t *testing.T) { assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) // wrong volume name - _, _, ok = matchVolumeInBundle(b, "main", "my_schema", "doesnotexist") + _, _, ok = findVolumeInBundle(b, "main", "my_schema", "doesnotexist") assert.False(t, ok) // wrong schema name - _, _, ok = matchVolumeInBundle(b, "main", "doesnotexist", "my_volume") + _, _, ok = findVolumeInBundle(b, "main", "doesnotexist", "my_volume") + assert.False(t, ok) + + // wrong catalog name + _, _, ok = findVolumeInBundle(b, "doesnotexist", "my_schema", "my_volume") assert.False(t, ok) // schema name is interpolated. b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema}" - path, locations, ok = matchVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") + path, locations, ok = findVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ File: "volume.yml", @@ -442,7 +446,8 @@ func TestGetFilerForLibraries(t *testing.T) { b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := GetFilerForLibraries(context.Background(), b) - require.EqualError(t, diags.Error(), "failed to fetch metadata for the UC volume /Volumes/main/my_schema/doesnotexist that is configured in the artifact_path: error from API") + assert.EqualError(t, diags.Error(), "failed to fetch metadata for the UC volume /Volumes/main/my_schema/doesnotexist that is configured in the artifact_path: error from API") + assert.Len(t, diags, 1) }) t.Run("volume in DAB config", func(t *testing.T) { @@ -477,7 +482,7 @@ func TestGetFilerForLibraries(t *testing.T) { assert.EqualError(t, diags.Error(), "failed to fetch metadata for the UC volume /Volumes/main/my_schema/my_volume that is configured in the artifact_path: error from API") assert.Contains(t, diags, diag.Diagnostic{ Severity: diag.Warning, - Summary: "the UC volume that is likely being used in the artifact_path has not been deployed yet. Please deploy the UC volume in a separate bundle deploy before using it in the artifact_path.", + Summary: "You might be using a UC volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please deploy the UC volume in a separate bundle deploy before using it in the artifact_path.", Locations: []dyn.Location{{ File: "volume.yml", }}, @@ -515,7 +520,7 @@ func TestGetFilerForLibraries(t *testing.T) { } _, _, diags := GetFilerForLibraries(context.Background(), b) - require.EqualError(t, diags.Error(), fmt.Sprintf("expected UC volume path to be in the format /Volumes///, got %s", path.Join(p, ".internal"))) + require.EqualError(t, diags.Error(), fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", path.Join(p, ".internal"))) } }) } diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 213aed202..5c7d09402 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -14,10 +14,12 @@ import ( "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/internal" "github.com/databricks/cli/internal/acc" + "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -272,7 +274,7 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { } diags := bundle.Apply(ctx, b, bundle.Seq(libraries.ExpandGlobReferences(), libraries.Upload())) - require.EqualError(t, diags.Error(), fmt.Sprintf("the bundle is configured to upload artifacts to %s but a UC volume at %s does not exist", path.Join(volumePath, ".internal"), volumePath)) + assert.ErrorContains(t, diags.Error(), fmt.Sprintf("failed to fetch metadata for the UC volume %s that is configured in the artifact_path:", volumePath)) }) t.Run("volume in DAB config", func(t *testing.T) { @@ -318,17 +320,23 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { }) diags := bundle.Apply(ctx, b, bundle.Seq(libraries.ExpandGlobReferences(), libraries.Upload())) - require.EqualError( - t, - diags.Error(), - fmt.Sprintf(`the bundle is configured to upload artifacts to %s but a -UC volume at %s does not exist. Note: We detected that you have a UC volume -defined that matched the path above at %s. Please deploy the UC volume -in a separate deployment before using it in as a destination to upload -artifacts.`, path.Join(volumePath, ".internal"), volumePath, dyn.Location{ - File: filepath.Join(dir, "databricks.yml"), - Line: 1, - Column: 2, - })) + assert.Contains(t, diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("failed to fetch metadata for the UC volume %s that is configured in the artifact_path: Not Found", volumePath), + }) + assert.Contains(t, diags, diag.Diagnostic{ + Severity: diag.Warning, + Summary: "You might be using a UC volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please deploy the UC volume in a separate bundle deploy before using it in the artifact_path.", + Locations: []dyn.Location{ + { + File: filepath.Join(dir, "databricks.yml"), + Line: 1, + Column: 2, + }, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.volumes.foo"), + }, + }) }) } diff --git a/libs/dyn/dynvar/ref.go b/libs/dyn/dynvar/ref.go index 088b68d26..338ac8ce6 100644 --- a/libs/dyn/dynvar/ref.go +++ b/libs/dyn/dynvar/ref.go @@ -71,7 +71,3 @@ func (v ref) references() []string { func IsPureVariableReference(s string) bool { return len(s) > 0 && re.FindString(s) == s } - -func ContainsVariableReference(s string) bool { - return len(s) > 0 && re.FindString(s) != "" -}