From f772ce425960896e2c5b3226117e2c0de52cd714 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 13:10:36 +0200 Subject: [PATCH 01/69] first comment --- bundle/config/resources.go | 1 + bundle/config/resources/volume.go | 27 +++++++++ .../deploy/terraform/tfdyn/convert_volume.go | 56 +++++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 bundle/config/resources/volume.go create mode 100644 bundle/deploy/terraform/tfdyn/convert_volume.go diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 22d69ffb53..3aecff86fc 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -19,6 +19,7 @@ type Resources struct { RegisteredModels map[string]*resources.RegisteredModel `json:"registered_models,omitempty"` QualityMonitors map[string]*resources.QualityMonitor `json:"quality_monitors,omitempty"` Schemas map[string]*resources.Schema `json:"schemas,omitempty"` + Volumes map[string]*resources.Volume `json:"volumes,omitempty"` } type ConfigResource interface { diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go new file mode 100644 index 0000000000..386d2ee376 --- /dev/null +++ b/bundle/config/resources/volume.go @@ -0,0 +1,27 @@ +package resources + +import ( + "github.com/databricks/databricks-sdk-go/marshal" + "github.com/databricks/databricks-sdk-go/service/catalog" +) + +type Volume struct { + // List of grants to apply on this schema. + Grants []Grant `json:"grants,omitempty"` + + // Full name of the schema (catalog_name.schema_name.volume_name). This value is read from + // the terraform state after deployment succeeds. + ID string `json:"id,omitempty" bundle:"readonly"` + + *catalog.CreateVolumeRequestContent + + ModifiedStatus ModifiedStatus `json:"modified_status,omitempty" bundle:"internal"` +} + +func (v *Volume) UnmarshalJSON(b []byte) error { + return marshal.Unmarshal(b, v) +} + +func (v Volume) MarshalJSON() ([]byte, error) { + return marshal.Marshal(v) +} diff --git a/bundle/deploy/terraform/tfdyn/convert_volume.go b/bundle/deploy/terraform/tfdyn/convert_volume.go new file mode 100644 index 0000000000..d841c7c605 --- /dev/null +++ b/bundle/deploy/terraform/tfdyn/convert_volume.go @@ -0,0 +1,56 @@ +package tfdyn + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle/internal/tf/schema" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/cli/libs/log" +) + +func convertVolumeResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { + // Normalize the output value to the target schema. + v, diags := convert.Normalize(schema.ResourceVolume{}, vin) + for _, diag := range diags { + log.Debugf(ctx, "volume normalization diagnostic: %s", diag.Summary) + } + + // TODO: What happens if I try to delete a UC volume that has data in it? + // Do I need force destroy functionality here. + + // We always set force destroy as it allows DABs to manage the lifecycle + // of the schema. It's the responsibility of the CLI to ensure the user + // is adequately warned when they try to delete a UC Volume. + vout, err := dyn.SetByPath(v, dyn.MustPathFromString("force_destroy"), dyn.V(true)) + if err != nil { + return dyn.InvalidValue, err + } + + return vout, nil +} + +type volumeConverter struct{} + +func (volumeConverter) Convert(ctx context.Context, key string, vin dyn.Value, out *schema.Resources) error { + vout, err := convertVolumeResource(ctx, vin) + if err != nil { + return err + } + + // Add the converted resource to the output. + out.Schema[key] = vout.AsAny() + + // Configure grants for this resource. + if grants := convertGrantsResource(ctx, vin); grants != nil { + grants.Schema = fmt.Sprintf("${databricks_schema.%s.id}", key) + out.Grants["schema_"+key] = grants + } + + return nil +} + +func init() { + registerConverter("volumes", volumeConverter{}) +} From ce5792c256801fdff1bee8c3fb7cd782af6a0756 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 14:40:10 +0200 Subject: [PATCH 02/69] fix convertor and add unit test --- .../deploy/terraform/tfdyn/convert_volume.go | 19 ++--- .../terraform/tfdyn/convert_volume_test.go | 70 +++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-) create mode 100644 bundle/deploy/terraform/tfdyn/convert_volume_test.go diff --git a/bundle/deploy/terraform/tfdyn/convert_volume.go b/bundle/deploy/terraform/tfdyn/convert_volume.go index d841c7c605..f63e5069f0 100644 --- a/bundle/deploy/terraform/tfdyn/convert_volume.go +++ b/bundle/deploy/terraform/tfdyn/convert_volume.go @@ -10,24 +10,15 @@ import ( "github.com/databricks/cli/libs/log" ) +// TODO: Articulate the consequences of deleting a UC volume in the prompt message that +// is displayed. func convertVolumeResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { // Normalize the output value to the target schema. - v, diags := convert.Normalize(schema.ResourceVolume{}, vin) + vout, diags := convert.Normalize(schema.ResourceVolume{}, vin) for _, diag := range diags { log.Debugf(ctx, "volume normalization diagnostic: %s", diag.Summary) } - // TODO: What happens if I try to delete a UC volume that has data in it? - // Do I need force destroy functionality here. - - // We always set force destroy as it allows DABs to manage the lifecycle - // of the schema. It's the responsibility of the CLI to ensure the user - // is adequately warned when they try to delete a UC Volume. - vout, err := dyn.SetByPath(v, dyn.MustPathFromString("force_destroy"), dyn.V(true)) - if err != nil { - return dyn.InvalidValue, err - } - return vout, nil } @@ -44,8 +35,8 @@ func (volumeConverter) Convert(ctx context.Context, key string, vin dyn.Value, o // Configure grants for this resource. if grants := convertGrantsResource(ctx, vin); grants != nil { - grants.Schema = fmt.Sprintf("${databricks_schema.%s.id}", key) - out.Grants["schema_"+key] = grants + grants.Volume = fmt.Sprintf("${databricks_volume.%s.id}", key) + out.Grants["volume_"+key] = grants } return nil diff --git a/bundle/deploy/terraform/tfdyn/convert_volume_test.go b/bundle/deploy/terraform/tfdyn/convert_volume_test.go new file mode 100644 index 0000000000..ded32fcbcb --- /dev/null +++ b/bundle/deploy/terraform/tfdyn/convert_volume_test.go @@ -0,0 +1,70 @@ +package tfdyn + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/tf/schema" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConvertVolume(t *testing.T) { + var src = resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "catalog", + Comment: "comment", + Name: "name", + SchemaName: "schema", + StorageLocation: "s3://bucket/path", + VolumeType: "EXTERNAL", + }, + Grants: []resources.Grant{ + { + Privileges: []string{"READ_VOLUME"}, + Principal: "jack@gmail.com", + }, + { + Privileges: []string{"WRITE_VOLUME"}, + Principal: "jane@gmail.com", + }, + }, + } + + vin, err := convert.FromTyped(src, dyn.NilValue) + require.NoError(t, err) + + ctx := context.Background() + out := schema.NewResources() + err = volumeConverter{}.Convert(ctx, "my_volume", vin, out) + require.NoError(t, err) + + // Assert equality on the volume + require.Equal(t, map[string]any{ + "catalog_name": "catalog", + "comment": "comment", + "name": "name", + "schema_name": "schema", + "storage_location": "s3://bucket/path", + "volume_type": "EXTERNAL", + }, out.Schema["my_volume"]) + + // Assert equality on the grants + assert.Equal(t, &schema.ResourceGrants{ + Volume: "${databricks_volume.my_volume.id}", + Grant: []schema.ResourceGrantsGrant{ + { + Privileges: []string{"READ_VOLUME"}, + Principal: "jack@gmail.com", + }, + { + Privileges: []string{"WRITE_VOLUME"}, + Principal: "jane@gmail.com", + }, + }, + }, out.Grants["volume_my_volume"]) +} From 7c7abeff815c7a46f39c5ac68e88723008512a63 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 14:41:42 +0200 Subject: [PATCH 03/69] run as support --- bundle/config/mutator/run_as_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index e6cef9ba45..fbfdba7b07 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -40,6 +40,7 @@ func allResourceTypes(t *testing.T) []string { "quality_monitors", "registered_models", "schemas", + "volumes", }, resourceTypes, ) @@ -138,6 +139,7 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { "registered_models", "experiments", "schemas", + "volumes", } base := config.Root{ From d04b6b08ea247daaf1fa8b3094ac265a307285aa Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 14:52:25 +0200 Subject: [PATCH 04/69] - --- bundle/deploy/terraform/tfdyn/convert_volume.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundle/deploy/terraform/tfdyn/convert_volume.go b/bundle/deploy/terraform/tfdyn/convert_volume.go index f63e5069f0..dd06cf1824 100644 --- a/bundle/deploy/terraform/tfdyn/convert_volume.go +++ b/bundle/deploy/terraform/tfdyn/convert_volume.go @@ -12,6 +12,8 @@ import ( // TODO: Articulate the consequences of deleting a UC volume in the prompt message that // is displayed. +// TODO: What sort of interpolation should be allowed at `artifact_path`? Should it be +// ${volumes.foo.id} or ${volumes.foo.name} or something else? func convertVolumeResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { // Normalize the output value to the target schema. vout, diags := convert.Normalize(schema.ResourceVolume{}, vin) From 9b66cd523bf4cad4e4aca6abce202a08eff65530 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 15:11:57 +0200 Subject: [PATCH 05/69] add apply target mode prefix functionality --- bundle/config/mutator/apply_presets.go | 7 +++ bundle/config/mutator/apply_presets_test.go | 56 +++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 28d015c109..c255f3571a 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -160,6 +160,13 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // the Databricks UI and via the SQL API. } + // Apply the prefix to volumes + for i := range r.Volumes { + r.Volumes[i].Name = normalizePrefix(prefix) + r.Volumes[i].Name + // HTTP API for volumes doesn't yet support tags. It's only supported in + // the Databricks UI and via the SQL API. + } + return nil } diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index ab2478aee0..b3f4e2d02e 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -125,6 +125,62 @@ func TestApplyPresetsPrefixForUcSchema(t *testing.T) { } } +func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { + tests := []struct { + name string + prefix string + volume *resources.Volume + want string + }{ + { + name: "add prefix to volume", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + }, + }, + want: "prefix_volume1", + }, + { + name: "add empty prefix to volume", + prefix: "", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + }, + }, + want: "volume1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "volume1": tt.volume, + }, + }, + Presets: config.Presets{ + NamePrefix: tt.prefix, + }, + }, + } + + ctx := context.Background() + diag := bundle.Apply(ctx, b, mutator.ApplyPresets()) + + if diag.HasError() { + t.Fatalf("unexpected error: %v", diag) + } + + require.Equal(t, tt.want, b.Config.Resources.Volumes["volume1"].Name) + }) + } +} + func TestApplyPresetsTags(t *testing.T) { tests := []struct { name string From 4b22e2d6581d13bec46b62c71d7b2f586ac9cb4e Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 15:36:29 +0200 Subject: [PATCH 06/69] add conversion tests --- bundle/config/resources/volume.go | 3 ++ bundle/deploy/terraform/convert.go | 15 +++++++ bundle/deploy/terraform/convert_test.go | 56 +++++++++++++++++++++++++ 3 files changed, 74 insertions(+) diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go index 386d2ee376..0ee4dce8ae 100644 --- a/bundle/config/resources/volume.go +++ b/bundle/config/resources/volume.go @@ -9,10 +9,13 @@ type Volume struct { // List of grants to apply on this schema. Grants []Grant `json:"grants,omitempty"` + // TODO: Confirm the accuracy of this comment. // Full name of the schema (catalog_name.schema_name.volume_name). This value is read from // the terraform state after deployment succeeds. ID string `json:"id,omitempty" bundle:"readonly"` + // TODO: Are there fields in the edit API or terraform that are not in this struct? + // If so call it out in the PR. *catalog.CreateVolumeRequestContent ModifiedStatus ModifiedStatus `json:"modified_status,omitempty" bundle:"internal"` diff --git a/bundle/deploy/terraform/convert.go b/bundle/deploy/terraform/convert.go index f13c241cee..37ea416441 100644 --- a/bundle/deploy/terraform/convert.go +++ b/bundle/deploy/terraform/convert.go @@ -394,6 +394,16 @@ func TerraformToBundle(state *resourcesState, config *config.Root) error { } cur.ID = instance.Attributes.ID config.Resources.Schemas[resource.Name] = cur + case "databricks_volume": + if config.Resources.Volumes == nil { + config.Resources.Volumes = make(map[string]*resources.Volume) + } + cur := config.Resources.Volumes[resource.Name] + if cur == nil { + cur = &resources.Volume{ModifiedStatus: resources.ModifiedStatusDeleted} + } + cur.ID = instance.Attributes.ID + config.Resources.Volumes[resource.Name] = cur case "databricks_permissions": case "databricks_grants": // Ignore; no need to pull these back into the configuration. @@ -443,6 +453,11 @@ func TerraformToBundle(state *resourcesState, config *config.Root) error { src.ModifiedStatus = resources.ModifiedStatusCreated } } + for _, src := range config.Resources.Volumes { + if src.ModifiedStatus == "" && src.ID == "" { + src.ModifiedStatus = resources.ModifiedStatusCreated + } + } return nil } diff --git a/bundle/deploy/terraform/convert_test.go b/bundle/deploy/terraform/convert_test.go index e4ef6114a9..52c19a30c6 100644 --- a/bundle/deploy/terraform/convert_test.go +++ b/bundle/deploy/terraform/convert_test.go @@ -663,6 +663,14 @@ func TestTerraformToBundleEmptyLocalResources(t *testing.T) { {Attributes: stateInstanceAttributes{ID: "1"}}, }, }, + { + Type: "databricks_volume", + Mode: "managed", + Name: "test_volume", + Instances: []stateResourceInstance{ + {Attributes: stateInstanceAttributes{ID: "1"}}, + }, + }, }, } err := TerraformToBundle(&tfState, &config) @@ -692,6 +700,9 @@ func TestTerraformToBundleEmptyLocalResources(t *testing.T) { assert.Equal(t, "1", config.Resources.Schemas["test_schema"].ID) assert.Equal(t, resources.ModifiedStatusDeleted, config.Resources.Schemas["test_schema"].ModifiedStatus) + assert.Equal(t, "1", config.Resources.Volumes["test_volume"].ID) + assert.Equal(t, resources.ModifiedStatusDeleted, config.Resources.Volumes["test_volume"].ModifiedStatus) + AssertFullResourceCoverage(t, &config) } @@ -754,6 +765,13 @@ func TestTerraformToBundleEmptyRemoteResources(t *testing.T) { }, }, }, + Volumes: map[string]*resources.Volume{ + "test_volume": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "test_volume", + }, + }, + }, }, } var tfState = resourcesState{ @@ -786,6 +804,9 @@ func TestTerraformToBundleEmptyRemoteResources(t *testing.T) { assert.Equal(t, "", config.Resources.Schemas["test_schema"].ID) assert.Equal(t, resources.ModifiedStatusCreated, config.Resources.Schemas["test_schema"].ModifiedStatus) + assert.Equal(t, "", config.Resources.Volumes["test_volume"].ID) + assert.Equal(t, resources.ModifiedStatusCreated, config.Resources.Volumes["test_volume"].ModifiedStatus) + AssertFullResourceCoverage(t, &config) } @@ -888,6 +909,18 @@ func TestTerraformToBundleModifiedResources(t *testing.T) { }, }, }, + Volumes: map[string]*resources.Volume{ + "test_volume": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "test_volume", + }, + }, + "test_volume_new": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "test_volume_new", + }, + }, + }, }, } var tfState = resourcesState{ @@ -1020,6 +1053,22 @@ func TestTerraformToBundleModifiedResources(t *testing.T) { {Attributes: stateInstanceAttributes{ID: "2"}}, }, }, + { + Type: "databricks_volume", + Mode: "managed", + Name: "test_volume", + Instances: []stateResourceInstance{ + {Attributes: stateInstanceAttributes{ID: "1"}}, + }, + }, + { + Type: "databricks_volume", + Mode: "managed", + Name: "test_volume_old", + Instances: []stateResourceInstance{ + {Attributes: stateInstanceAttributes{ID: "2"}}, + }, + }, }, } err := TerraformToBundle(&tfState, &config) @@ -1081,6 +1130,13 @@ func TestTerraformToBundleModifiedResources(t *testing.T) { assert.Equal(t, "", config.Resources.Schemas["test_schema_new"].ID) assert.Equal(t, resources.ModifiedStatusCreated, config.Resources.Schemas["test_schema_new"].ModifiedStatus) + assert.Equal(t, "1", config.Resources.Volumes["test_volume"].ID) + assert.Equal(t, "", config.Resources.Volumes["test_volume"].ModifiedStatus) + assert.Equal(t, "2", config.Resources.Volumes["test_volume_old"].ID) + assert.Equal(t, resources.ModifiedStatusDeleted, config.Resources.Volumes["test_volume_old"].ModifiedStatus) + assert.Equal(t, "", config.Resources.Volumes["test_volume_new"].ID) + assert.Equal(t, resources.ModifiedStatusCreated, config.Resources.Volumes["test_volume_new"].ModifiedStatus) + AssertFullResourceCoverage(t, &config) } From 88d0402f441561f31a1066af6c8c49aed086f769 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 9 Sep 2024 15:41:03 +0200 Subject: [PATCH 07/69] add inteprolation for volumes fields --- bundle/deploy/terraform/interpolate.go | 2 ++ bundle/deploy/terraform/interpolate_test.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/bundle/deploy/terraform/interpolate.go b/bundle/deploy/terraform/interpolate.go index faa098e1cc..96a71c16b3 100644 --- a/bundle/deploy/terraform/interpolate.go +++ b/bundle/deploy/terraform/interpolate.go @@ -58,6 +58,8 @@ func (m *interpolateMutator) Apply(ctx context.Context, b *bundle.Bundle) diag.D path = dyn.NewPath(dyn.Key("databricks_quality_monitor")).Append(path[2:]...) case dyn.Key("schemas"): path = dyn.NewPath(dyn.Key("databricks_schema")).Append(path[2:]...) + case dyn.Key("volumes"): + path = dyn.NewPath(dyn.Key("databricks_volume")).Append(path[2:]...) default: // Trigger "key not found" for unknown resource types. return dyn.GetByPath(root, path) diff --git a/bundle/deploy/terraform/interpolate_test.go b/bundle/deploy/terraform/interpolate_test.go index 5ceb243bcd..4890099928 100644 --- a/bundle/deploy/terraform/interpolate_test.go +++ b/bundle/deploy/terraform/interpolate_test.go @@ -31,6 +31,7 @@ func TestInterpolate(t *testing.T) { "other_model_serving": "${resources.model_serving_endpoints.other_model_serving.id}", "other_registered_model": "${resources.registered_models.other_registered_model.id}", "other_schema": "${resources.schemas.other_schema.id}", + "other_volume": "${resources.volumes.other_volume.id}", }, Tasks: []jobs.Task{ { @@ -67,6 +68,7 @@ func TestInterpolate(t *testing.T) { assert.Equal(t, "${databricks_model_serving.other_model_serving.id}", j.Tags["other_model_serving"]) assert.Equal(t, "${databricks_registered_model.other_registered_model.id}", j.Tags["other_registered_model"]) assert.Equal(t, "${databricks_schema.other_schema.id}", j.Tags["other_schema"]) + assert.Equal(t, "${databricks_volume.other_volume.id}", j.Tags["other_volume"]) m := b.Config.Resources.Models["my_model"] assert.Equal(t, "my_model", m.Model.Name) From 6f9817e194092e47c7e5b41da4df62f1ea7caeeb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 10 Sep 2024 12:52:31 +0200 Subject: [PATCH 08/69] add prompt and crud test for volumes --- .../deploy/terraform/tfdyn/convert_volume.go | 2 +- bundle/phases/deploy.go | 46 ++++++++-------- bundle/phases/deploy_test.go | 2 +- .../uc_volume/databricks_template_schema.json | 8 +++ .../uc_volume/template/databricks.yml.tmpl | 26 +++++++++ .../bundle/bundles/uc_volume/template/nb.sql | 2 + internal/bundle/deploy_test.go | 53 +++++++++++++++++++ 7 files changed, 112 insertions(+), 27 deletions(-) create mode 100644 internal/bundle/bundles/uc_volume/databricks_template_schema.json create mode 100644 internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl create mode 100644 internal/bundle/bundles/uc_volume/template/nb.sql diff --git a/bundle/deploy/terraform/tfdyn/convert_volume.go b/bundle/deploy/terraform/tfdyn/convert_volume.go index dd06cf1824..281cd43262 100644 --- a/bundle/deploy/terraform/tfdyn/convert_volume.go +++ b/bundle/deploy/terraform/tfdyn/convert_volume.go @@ -33,7 +33,7 @@ func (volumeConverter) Convert(ctx context.Context, key string, vin dyn.Value, o } // Add the converted resource to the output. - out.Schema[key] = vout.AsAny() + out.Volume[key] = vout.AsAny() // Configure grants for this resource. if grants := convertGrantsResource(ctx, vin); grants != nil { diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 49544227ec..d71c61912d 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -22,10 +22,12 @@ import ( tfjson "github.com/hashicorp/terraform-json" ) -func parseTerraformActions(changes []*tfjson.ResourceChange, toInclude func(typ string, actions tfjson.Actions) bool) []terraformlib.Action { +func filterDeleteOrRecreateActions(changes []*tfjson.ResourceChange, resourceType string) []terraformlib.Action { res := make([]terraformlib.Action, 0) for _, rc := range changes { - if !toInclude(rc.Type, rc.Change.Actions) { + // TODO: Add end to end integration tests for the interactive prompt UXs. + // Good PR to introduce the first one, and make changes more confidently. + if rc.Type != resourceType { continue } @@ -36,7 +38,7 @@ func parseTerraformActions(changes []*tfjson.ResourceChange, toInclude func(typ case rc.Change.Actions.Replace(): actionType = terraformlib.ActionTypeRecreate default: - // No use case for other action types yet. + // Filter other action types.. continue } @@ -62,30 +64,12 @@ func approvalForDeploy(ctx context.Context, b *bundle.Bundle) (bool, error) { return false, err } - schemaActions := parseTerraformActions(plan.ResourceChanges, func(typ string, actions tfjson.Actions) bool { - // Filter in only UC schema resources. - if typ != "databricks_schema" { - return false - } - - // We only display prompts for destructive actions like deleting or - // recreating a schema. - return actions.Delete() || actions.Replace() - }) - - dltActions := parseTerraformActions(plan.ResourceChanges, func(typ string, actions tfjson.Actions) bool { - // Filter in only DLT pipeline resources. - if typ != "databricks_pipeline" { - return false - } - - // Recreating DLT pipeline leads to metadata loss and for a transient period - // the underling tables will be unavailable. - return actions.Replace() || actions.Delete() - }) + schemaActions := filterDeleteOrRecreateActions(plan.ResourceChanges, "databricks_schema") + dltActions := filterDeleteOrRecreateActions(plan.ResourceChanges, "databricks_pipeline") + volumeActions := filterDeleteOrRecreateActions(plan.ResourceChanges, "databricks_volume") // We don't need to display any prompts in this case. - if len(dltActions) == 0 && len(schemaActions) == 0 { + if len(schemaActions) == 0 && len(dltActions) == 0 && len(volumeActions) == 0 { return true, nil } @@ -110,6 +94,18 @@ properties such as the 'catalog' or 'storage' are changed:` } } + // One or more volumes is being recreated. + if len(volumeActions) != 0 { + msg := ` +This action will result in the deletion or recreation of the following Volumes. For managed volumes, +this typically results in a deletion of the upstream data in the cloud tenant in ~30 days. For external +volumes the upstream data in the cloud tenant is not affected:` + cmdio.LogString(ctx, msg) + for _, action := range volumeActions { + cmdio.Log(ctx, action) + } + } + if b.AutoApprove { return true, nil } diff --git a/bundle/phases/deploy_test.go b/bundle/phases/deploy_test.go index e00370b380..f239302d49 100644 --- a/bundle/phases/deploy_test.go +++ b/bundle/phases/deploy_test.go @@ -40,7 +40,7 @@ func TestParseTerraformActions(t *testing.T) { }, } - res := parseTerraformActions(changes, func(typ string, actions tfjson.Actions) bool { + res := filterDeleteOrRecreateActions(changes, func(typ string, actions tfjson.Actions) bool { if typ != "databricks_pipeline" { return false } diff --git a/internal/bundle/bundles/uc_volume/databricks_template_schema.json b/internal/bundle/bundles/uc_volume/databricks_template_schema.json new file mode 100644 index 0000000000..762f4470c2 --- /dev/null +++ b/internal/bundle/bundles/uc_volume/databricks_template_schema.json @@ -0,0 +1,8 @@ +{ + "properties": { + "unique_id": { + "type": "string", + "description": "Unique ID for the schema and pipeline names" + } + } +} diff --git a/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl b/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl new file mode 100644 index 0000000000..9d377f8e7a --- /dev/null +++ b/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl @@ -0,0 +1,26 @@ +bundle: + name: test-uc-volumes-{{.unique_id}} + +variables: + schema_name: + default: ${resources.schemas.schema1.name} + +resources: + schemas: + schema1: + name: schema1-{{.unique_id}} + catalog_name: main + comment: This schema was created from DABs + + schema2: + name: schema2-{{.unique_id}} + catalog_name: main + comment: This schema was created from DABs + + volumes: + foo: + catalog_name: main + name: my_volume + schema_name: ${var.schema_name} + volume_type: MANAGED + comment: This volume was created from DABs. diff --git a/internal/bundle/bundles/uc_volume/template/nb.sql b/internal/bundle/bundles/uc_volume/template/nb.sql new file mode 100644 index 0000000000..199ff50788 --- /dev/null +++ b/internal/bundle/bundles/uc_volume/template/nb.sql @@ -0,0 +1,2 @@ +-- Databricks notebook source +select 1 diff --git a/internal/bundle/deploy_test.go b/internal/bundle/deploy_test.go index 736c880dbc..225458e195 100644 --- a/internal/bundle/deploy_test.go +++ b/internal/bundle/deploy_test.go @@ -243,3 +243,56 @@ func TestAccDeployBasicBundleLogs(t *testing.T) { }, "\n"), stderr) assert.Equal(t, "", stdout) } + +func TestAccDeployUcVolume(t *testing.T) { + ctx, wt := acc.UcWorkspaceTest(t) + w := wt.W + + uniqueId := uuid.New().String() + bundleRoot, err := initTestTemplate(t, ctx, "uc_volume", map[string]any{ + "unique_id": uniqueId, + }) + require.NoError(t, err) + + err = deployBundle(t, ctx, bundleRoot) + require.NoError(t, err) + + t.Cleanup(func() { + destroyBundle(t, ctx, bundleRoot) + }) + + // Assert the volume is created successfully + catalogName := "main" + schemaName := "schema1-" + uniqueId + volumeName := "my_volume" + volume, err := w.Volumes.ReadByName(ctx, fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName)) + require.NoError(t, err) + require.Equal(t, volume.Name, volumeName) + require.Equal(t, catalogName, volume.CatalogName) + require.Equal(t, schemaName, volume.SchemaName) + + // Recreation of the volume without --auto-approve should fail since prompting is not possible + t.Setenv("TERM", "dumb") + t.Setenv("BUNDLE_ROOT", bundleRoot) + stdout, stderr, err := internal.NewCobraTestRunnerWithContext(t, ctx, "bundle", "deploy", "--var=schema_name=${resources.schemas.schema2.name}").Run() + assert.Error(t, err) + assert.Contains(t, stderr.String(), `This action will result in the deletion or recreation of the following Volumes. For managed volumes, +this typically results in a deletion of the upstream data in the cloud tenant in ~30 days. For external +volumes the upstream data in the cloud tenant is not affected: + recreate volume foo`) + assert.Contains(t, stdout.String(), "the deployment requires destructive actions, but current console does not support prompting. Please specify --auto-approve if you would like to skip prompts and proceed") + + // Recreation of the volume without --auto-approve should fail since prompting is not possible + t.Setenv("TERM", "dumb") + t.Setenv("BUNDLE_ROOT", bundleRoot) + _, _, err = internal.NewCobraTestRunnerWithContext(t, ctx, "bundle", "deploy", "--var=schema_name=${resources.schemas.schema2.name}", "--auto-approve").Run() + assert.NoError(t, err) + + // Assert the volume is updated successfully + schemaName = "schema2-" + uniqueId + volume, err = w.Volumes.ReadByName(ctx, fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName)) + require.NoError(t, err) + require.Equal(t, volume.Name, volumeName) + require.Equal(t, catalogName, volume.CatalogName) + require.Equal(t, schemaName, volume.SchemaName) +} From d180bab15d41c83b6556a1bd9466d7f37c9f1c71 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Sun, 15 Sep 2024 23:19:40 +0200 Subject: [PATCH 09/69] add filer --- bundle/artifacts/upload.go | 6 +-- bundle/libraries/upload.go | 86 +++++++++++++++++++++++++++++++++----- libs/dyn/dynvar/ref.go | 4 ++ 3 files changed, 82 insertions(+), 14 deletions(-) diff --git a/bundle/artifacts/upload.go b/bundle/artifacts/upload.go index 58c006dc18..d7b3e1baa7 100644 --- a/bundle/artifacts/upload.go +++ b/bundle/artifacts/upload.go @@ -26,9 +26,9 @@ func (m *cleanUp) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics return diag.FromErr(err) } - client, err := libraries.GetFilerForLibraries(b.WorkspaceClient(), uploadPath) - if err != nil { - return diag.FromErr(err) + client, diags := libraries.GetFilerForLibraries(ctx, b, uploadPath) + if diags.HasError() { + return diags } // We intentionally ignore the error because it is not critical to the deployment diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 90a1a21fc4..a45db5dc4f 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -13,11 +13,10 @@ import ( "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/log" - "github.com/databricks/databricks-sdk-go" - "golang.org/x/sync/errgroup" ) @@ -138,9 +137,9 @@ 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 if u.client == nil { - filer, err := GetFilerForLibraries(b.WorkspaceClient(), uploadPath) - if err != nil { - return diag.FromErr(err) + filer, diags := GetFilerForLibraries(ctx, b, uploadPath) + if diags.HasError() { + return diags } u.client = filer @@ -197,15 +196,80 @@ func (u *upload) Name() string { return "libraries.Upload" } -func GetFilerForLibraries(w *databricks.WorkspaceClient, uploadPath string) (filer.Filer, error) { - if isVolumesPath(uploadPath) { - return filer.NewFilesClient(w, uploadPath) +// TODO: TODO: Nicer comments here. +// Case 1: UC volume path is valid. Return the client. +// Case 2: invalid path. +// (a) Not enough elements. +// (b) catalog and schema correspond to a volume define in the DAB. +// +// -> exception for when the schema value is fully or partially interpolated. +// In that case only check the catalog name. +func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath string) (filer.Filer, diag.Diagnostics) { + w := b.WorkspaceClient() + isVolumesPath := strings.HasPrefix(uploadPath, "/Volumes/") + + // If the path is not a volume path, use the workspace file system. + if !isVolumesPath { + f, err := filer.NewWorkspaceFilesClient(w, uploadPath) + return f, diag.FromErr(err) } - return filer.NewWorkspaceFilesClient(w, uploadPath) + + parts := strings.Split(uploadPath, "/") + volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes///, got %s", uploadPath) + if len(strings.Split(uploadPath, "/")) < 5 { + return nil, diag.FromErr(volumeFormatErr) + } + + catalogName := parts[2] + schemaName := parts[3] + volumeName := parts[4] + + // Incorrect format. + if catalogName == "" || schemaName == "" || volumeName == "" { + return nil, diag.FromErr(volumeFormatErr) + } + + // If the volume exists already, directly return the filer for the upload path. + volumePath := fmt.Sprintf("/Volumes/%s/%s/%s", catalogName, schemaName, volumeName) + vf, err := filer.NewFilesClient(w, volumePath) + if err != nil { + return nil, diag.FromErr(err) + } + if _, err := vf.Stat(ctx, "."); err == nil { + f, err := filer.NewFilesClient(w, uploadPath) + return f, diag.FromErr(err) + } + + // The volume does not exist. Check if the volume is defined in the bundle. + // TODO: Does this break? Did it work before if the volume was not defined, but + // the schema was? + l, ok := locationForVolume(b, catalogName, schemaName, volumeName) + if !ok { + return nil, diag.Errorf("UC volume %s does not exist", volumePath) + } + + return nil, diag.Errorf(`UC volume %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 separately before using it in as a +destination to upload artifacts.`, l, volumePath) } -func isVolumesPath(path string) bool { - return strings.HasPrefix(path, "/Volumes/") +func locationForVolume(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Location, bool) { + volumes := b.Config.Resources.Volumes + for k, v := range volumes { + if v.CatalogName != catalogName || v.Name != volumeName { + continue + } + // UC schemas can be defined in the bundle itself, and thus might be interpolated + // 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) { + continue + } + return b.Config.GetLocation(fmt.Sprintf("resources.volumes.%s", k)), true + } + return dyn.Location{}, false } // Function to upload file (a library, artifact and etc) to Workspace or UC volume diff --git a/libs/dyn/dynvar/ref.go b/libs/dyn/dynvar/ref.go index 338ac8ce6c..088b68d262 100644 --- a/libs/dyn/dynvar/ref.go +++ b/libs/dyn/dynvar/ref.go @@ -71,3 +71,7 @@ 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) != "" +} From 73826acb2f7f5c9af6e84ea57488c44153e4f9c1 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Sun, 15 Sep 2024 23:31:04 +0200 Subject: [PATCH 10/69] fix test and improve error --- bundle/libraries/upload.go | 11 ++++++----- bundle/phases/deploy_test.go | 12 +----------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index a45db5dc4f..c4d6273ba0 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -245,13 +245,14 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath stri // the schema was? l, ok := locationForVolume(b, catalogName, schemaName, volumeName) if !ok { - return nil, diag.Errorf("UC volume %s does not exist", volumePath) + return nil, diag.Errorf("the bundle is configured to upload artifacts to %s but a UC volume at %s does not exist", uploadPath, volumePath) } - return nil, diag.Errorf(`UC volume %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 separately before using it in as a -destination to upload artifacts.`, l, volumePath) + return nil, diag.Errorf(`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.`, uploadPath, volumePath, l) } func locationForVolume(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Location, bool) { diff --git a/bundle/phases/deploy_test.go b/bundle/phases/deploy_test.go index f239302d49..2cef954044 100644 --- a/bundle/phases/deploy_test.go +++ b/bundle/phases/deploy_test.go @@ -40,17 +40,7 @@ func TestParseTerraformActions(t *testing.T) { }, } - res := filterDeleteOrRecreateActions(changes, func(typ string, actions tfjson.Actions) bool { - if typ != "databricks_pipeline" { - return false - } - - if actions.Delete() || actions.Replace() { - return true - } - - return false - }) + res := filterDeleteOrRecreateActions(changes, "databricks_pipeline") assert.Equal(t, []terraformlib.Action{ { From de7eb94e457fadcdc4cba65855ec43659bffaaf8 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Sun, 15 Sep 2024 23:37:28 +0200 Subject: [PATCH 11/69] - --- bundle/libraries/upload.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index c4d6273ba0..060468047e 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -204,6 +204,8 @@ func (u *upload) Name() string { // // -> exception for when the schema value is fully or partially interpolated. // In that case only check the catalog name. +// +// TODO: Convert to warning? We don't error today if you specify an invalid volume path. func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath string) (filer.Filer, diag.Diagnostics) { w := b.WorkspaceClient() isVolumesPath := strings.HasPrefix(uploadPath, "/Volumes/") @@ -241,8 +243,7 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath stri } // The volume does not exist. Check if the volume is defined in the bundle. - // TODO: Does this break? Did it work before if the volume was not defined, but - // the schema was? + // TODO: Note that this is not a breaking change. l, ok := locationForVolume(b, catalogName, schemaName, volumeName) if !ok { return nil, diag.Errorf("the bundle is configured to upload artifacts to %s but a UC volume at %s does not exist", uploadPath, volumePath) From f10038a20e0e0cd52288ccf722429b4eb3fc1fed Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Sun, 15 Sep 2024 23:45:01 +0200 Subject: [PATCH 12/69] - --- bundle/config/mutator/process_target_mode_test.go | 7 +++++++ bundle/deploy/terraform/tfdyn/convert_volume_test.go | 2 +- bundle/libraries/upload.go | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 42f1929c89..75888a125d 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -119,6 +119,9 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Schemas: map[string]*resources.Schema{ "schema1": {CreateSchema: &catalog.CreateSchema{Name: "schema1"}}, }, + Volumes: map[string]*resources.Volume{ + "volume1": {CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{Name: "volume1"}}, + }, }, }, // Use AWS implementation for testing. @@ -281,6 +284,8 @@ func TestProcessTargetModeDefault(t *testing.T) { assert.Equal(t, "servingendpoint1", b.Config.Resources.ModelServingEndpoints["servingendpoint1"].Name) assert.Equal(t, "registeredmodel1", b.Config.Resources.RegisteredModels["registeredmodel1"].Name) assert.Equal(t, "qualityMonitor1", b.Config.Resources.QualityMonitors["qualityMonitor1"].TableName) + assert.Equal(t, "schema1", b.Config.Resources.Schemas["schema1"].Name) + assert.Equal(t, "volume1", b.Config.Resources.Volumes["volume1"].Name) } func TestProcessTargetModeProduction(t *testing.T) { @@ -322,6 +327,8 @@ func TestProcessTargetModeProduction(t *testing.T) { assert.Equal(t, "servingendpoint1", b.Config.Resources.ModelServingEndpoints["servingendpoint1"].Name) assert.Equal(t, "registeredmodel1", b.Config.Resources.RegisteredModels["registeredmodel1"].Name) assert.Equal(t, "qualityMonitor1", b.Config.Resources.QualityMonitors["qualityMonitor1"].TableName) + assert.Equal(t, "schema1", b.Config.Resources.Schemas["schema1"].Name) + assert.Equal(t, "volume1", b.Config.Resources.Volumes["volume1"].Name) } func TestProcessTargetModeProductionOkForPrincipal(t *testing.T) { diff --git a/bundle/deploy/terraform/tfdyn/convert_volume_test.go b/bundle/deploy/terraform/tfdyn/convert_volume_test.go index ded32fcbcb..c897ae69a8 100644 --- a/bundle/deploy/terraform/tfdyn/convert_volume_test.go +++ b/bundle/deploy/terraform/tfdyn/convert_volume_test.go @@ -51,7 +51,7 @@ func TestConvertVolume(t *testing.T) { "schema_name": "schema", "storage_location": "s3://bucket/path", "volume_type": "EXTERNAL", - }, out.Schema["my_volume"]) + }, out.Volume["my_volume"]) // Assert equality on the grants assert.Equal(t, &schema.ResourceGrants{ diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 060468047e..0e6511893c 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -244,6 +244,7 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath stri // The volume does not exist. Check if the volume is defined in the bundle. // TODO: Note that this is not a breaking change. + // TODO: Include error as well for why the stat call failed. It's more context. l, ok := locationForVolume(b, catalogName, schemaName, volumeName) if !ok { return nil, diag.Errorf("the bundle is configured to upload artifacts to %s but a UC volume at %s does not exist", uploadPath, volumePath) From df3bbad70b47a62cf1c2455e0723f386dd2a3026 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 00:50:12 +0200 Subject: [PATCH 13/69] add integration tests for artifacts portion: --- internal/bundle/artifacts_test.go | 107 ++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index fa052e2235..213aed2024 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -1,6 +1,7 @@ package bundle import ( + "fmt" "os" "path" "path/filepath" @@ -13,6 +14,8 @@ import ( "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/internal" "github.com/databricks/cli/internal/acc" + "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/require" @@ -225,3 +228,107 @@ func TestAccUploadArtifactFileToCorrectRemotePathForVolumes(t *testing.T) { b.Config.Resources.Jobs["test"].JobSettings.Tasks[0].Libraries[0].Whl, ) } + +func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { + ctx, wt := acc.UcWorkspaceTest(t) + w := wt.W + + schemaName := internal.RandomName("schema-") + + _, err := w.Schemas.Create(ctx, catalog.CreateSchema{ + CatalogName: "main", + Comment: "test schema", + Name: schemaName, + }) + require.NoError(t, err) + + t.Run("volume not in DAB", func(t *testing.T) { + volumePath := fmt.Sprintf("/Volumes/main/%s/doesnotexist", schemaName) + dir := t.TempDir() + + b := &bundle.Bundle{ + RootPath: dir, + SyncRootPath: dir, + Config: config.Root{ + Bundle: config.Bundle{ + Target: "whatever", + }, + Workspace: config.Workspace{ + ArtifactPath: volumePath, + }, + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + VolumeType: "MANAGED", + SchemaName: schemaName, + }, + }, + }, + }, + }, + } + + 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)) + }) + + t.Run("volume in DAB config", func(t *testing.T) { + volumePath := fmt.Sprintf("/Volumes/main/%s/my_volume", schemaName) + dir := t.TempDir() + + b := &bundle.Bundle{ + RootPath: dir, + SyncRootPath: dir, + Config: config.Root{ + Bundle: config.Bundle{ + Target: "whatever", + }, + Workspace: config.Workspace{ + ArtifactPath: volumePath, + }, + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + VolumeType: "MANAGED", + SchemaName: schemaName, + }, + }, + }, + }, + }, + } + + // set location of volume definition in config. + b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "resources.volumes.foo", func(p dyn.Path, volume dyn.Value) (dyn.Value, error) { + return volume.WithLocations([]dyn.Location{ + { + File: filepath.Join(dir, "databricks.yml"), + Line: 1, + Column: 2, + }, + }), nil + }) + }) + + 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, + })) + }) +} From aeab4efda1d7dbadbb83243238cc7ef62167bb67 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 00:57:38 +0200 Subject: [PATCH 14/69] unit test for comparision of locatoin --- bundle/libraries/upload.go | 4 +-- bundle/libraries/upload_test.go | 47 +++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 0e6511893c..cac9fa18f8 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -245,7 +245,7 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath stri // The volume does not exist. Check if the volume is defined in the bundle. // TODO: Note that this is not a breaking change. // TODO: Include error as well for why the stat call failed. It's more context. - l, ok := locationForVolume(b, catalogName, schemaName, volumeName) + l, ok := locationOfVolumeInBundle(b, catalogName, schemaName, volumeName) if !ok { return nil, diag.Errorf("the bundle is configured to upload artifacts to %s but a UC volume at %s does not exist", uploadPath, volumePath) } @@ -257,7 +257,7 @@ in a separate deployment before using it in as a destination to upload artifacts.`, uploadPath, volumePath, l) } -func locationForVolume(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Location, bool) { +func locationOfVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Location, bool) { volumes := b.Config.Resources.Volumes for k, v := range volumes { if v.CatalogName != catalogName || v.Name != volumeName { diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 44b194c56a..3eec469200 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -8,11 +8,15 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" + "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/mock" "github.com/stretchr/testify/require" ) @@ -329,3 +333,46 @@ func TestUploadMultipleLibraries(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies, "/Workspace/foo/bar/artifacts/.internal/source4.whl") require.Contains(t, b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies, "/Workspace/Users/foo@bar.com/mywheel.whl") } + +func TestLocationOfVolumeInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + SchemaName: "my_schema", + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.volumes.foo", "volume.yml") + + // volume is in DAB directly. + l, ok := locationOfVolumeInBundle(b, "main", "my_schema", "my_volume") + assert.True(t, ok) + assert.Equal(t, dyn.Location{ + File: "volume.yml", + }, l) + + // wrong volume name + _, ok = locationOfVolumeInBundle(b, "main", "my_schema", "doesnotexist") + assert.False(t, ok) + + // wrong schema name + _, ok = locationOfVolumeInBundle(b, "main", "doesnotexist", "my_volume") + assert.False(t, ok) + + // schema name is interpolated. + b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema}" + l, ok = locationOfVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") + assert.True(t, ok) + assert.Equal(t, dyn.Location{ + File: "volume.yml", + }, l) +} From aa2e16d757c4aa24a5892b587b4249b7d93fd094 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 02:28:58 +0200 Subject: [PATCH 15/69] cleanup and add test --- bundle/artifacts/upload.go | 9 +- bundle/libraries/upload.go | 110 ++++++------ bundle/libraries/upload_test.go | 157 ++++++++++++++++-- libs/filer/workspace_files_client.go | 16 +- .../workspace_files_extensions_client_test.go | 2 +- 5 files changed, 213 insertions(+), 81 deletions(-) diff --git a/bundle/artifacts/upload.go b/bundle/artifacts/upload.go index d7b3e1baa7..c69939e8c7 100644 --- a/bundle/artifacts/upload.go +++ b/bundle/artifacts/upload.go @@ -21,18 +21,13 @@ func (m *cleanUp) Name() string { } func (m *cleanUp) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - uploadPath, err := libraries.GetUploadBasePath(b) - if err != nil { - return diag.FromErr(err) - } - - client, diags := libraries.GetFilerForLibraries(ctx, b, uploadPath) + client, uploadPath, diags := libraries.GetFilerForLibraries(ctx, b) if diags.HasError() { return diags } // We intentionally ignore the error because it is not critical to the deployment - err = client.Delete(ctx, ".", filer.DeleteRecursively) + err := client.Delete(ctx, ".", filer.DeleteRecursively) if err != nil { log.Errorf(ctx, "failed to delete %s: %v", uploadPath, err) } diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index cac9fa18f8..163b56131e 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -129,24 +129,17 @@ func collectLocalLibraries(b *bundle.Bundle) (map[string][]configLocation, error } func (u *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - uploadPath, err := GetUploadBasePath(b) - if err != nil { - return diag.FromErr(err) - } - // If the client is not initialized, initialize it // We use client field in mutator to allow for mocking client in testing + var uploadPath string + var diags diag.Diagnostics if u.client == nil { - filer, diags := GetFilerForLibraries(ctx, b, uploadPath) + u.client, uploadPath, diags = GetFilerForLibraries(ctx, b) if diags.HasError() { return diags } - - u.client = filer } - var diags diag.Diagnostics - libs, err := collectLocalLibraries(b) if err != nil { return diag.FromErr(err) @@ -196,30 +189,45 @@ func (u *upload) Name() string { return "libraries.Upload" } -// TODO: TODO: Nicer comments here. -// Case 1: UC volume path is valid. Return the client. -// Case 2: invalid path. -// (a) Not enough elements. -// (b) catalog and schema correspond to a volume define in the DAB. +// This function returns the right filer to use, to upload artifacts to the configured locations. +// Supported locations: +// 1. WSFS +// 2. UC volumes // -// -> exception for when the schema value is fully or partially interpolated. -// In that case only check the catalog name. -// -// TODO: Convert to warning? We don't error today if you specify an invalid volume path. -func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath string) (filer.Filer, diag.Diagnostics) { +// If a UC Volume is configured, this function checks if the UC volume exists in the workspace. +// 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 +// 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. +// 3. If the UC volume does not exist in the workspace and is not defined in the bundle configuration: +// Returns an error. +func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { + artifactPath := b.Config.Workspace.ArtifactPath + if artifactPath == "" { + 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/") - // If the path is not a volume path, use the workspace file system. + // Return early with a WSFS filer if the artifact path is not a UC volume path. if !isVolumesPath { f, err := filer.NewWorkspaceFilesClient(w, uploadPath) - return f, diag.FromErr(err) + return f, uploadPath, diag.FromErr(err) } - parts := strings.Split(uploadPath, "/") + parts := strings.Split(artifactPath, "/") volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes///, got %s", uploadPath) - if len(strings.Split(uploadPath, "/")) < 5 { - return nil, diag.FromErr(volumeFormatErr) + + // Incorrect format. + if len(parts) < 5 { + return nil, "", diag.FromErr(volumeFormatErr) } catalogName := parts[2] @@ -228,36 +236,36 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle, uploadPath stri // Incorrect format. if catalogName == "" || schemaName == "" || volumeName == "" { - return nil, diag.FromErr(volumeFormatErr) + return nil, "", diag.FromErr(volumeFormatErr) } - // If the volume exists already, directly return the filer for the upload path. + // Check if the UC volume exists in the workspace. volumePath := fmt.Sprintf("/Volumes/%s/%s/%s", catalogName, schemaName, volumeName) - vf, err := filer.NewFilesClient(w, volumePath) - if err != nil { - return nil, diag.FromErr(err) - } - if _, err := vf.Stat(ctx, "."); err == nil { + err := w.Files.GetDirectoryMetadataByDirectoryPath(ctx, volumePath) + + // If the volume exists already, directly return the filer for the upload path. + if err == nil { f, err := filer.NewFilesClient(w, uploadPath) - return f, diag.FromErr(err) + return f, uploadPath, diag.FromErr(err) } - // The volume does not exist. Check if the volume is defined in the bundle. - // TODO: Note that this is not a breaking change. - // TODO: Include error as well for why the stat call failed. It's more context. - l, ok := locationOfVolumeInBundle(b, catalogName, schemaName, volumeName) + 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) if !ok { - return nil, diag.Errorf("the bundle is configured to upload artifacts to %s but a UC volume at %s does not exist", uploadPath, volumePath) + return nil, "", diags } - return nil, diag.Errorf(`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.`, uploadPath, volumePath, l) + 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.`, + Locations: locations, + Paths: []dyn.Path{path}, + } + return nil, "", diags.Append(warning) } -func locationOfVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Location, bool) { +func matchVolumeInBundle(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 { @@ -270,9 +278,10 @@ func locationOfVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeN if v.SchemaName != schemaName && !dynvar.ContainsVariableReference(v.SchemaName) { continue } - return b.Config.GetLocation(fmt.Sprintf("resources.volumes.%s", k)), true + pathString := fmt.Sprintf("resources.volumes.%s", k) + return dyn.MustPathFromString(pathString), b.Config.GetLocations(pathString), true } - return dyn.Location{}, false + return nil, nil, false } // Function to upload file (a library, artifact and etc) to Workspace or UC volume @@ -294,12 +303,3 @@ func UploadFile(ctx context.Context, file string, client filer.Filer) error { log.Infof(ctx, "Upload succeeded") return nil } - -func GetUploadBasePath(b *bundle.Bundle) (string, error) { - artifactPath := b.Config.Workspace.ArtifactPath - if artifactPath == "" { - return "", fmt.Errorf("remote artifact path not configured") - } - - return path.Join(artifactPath, ".internal"), nil -} diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 3eec469200..f15e2b93e4 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -2,6 +2,7 @@ package libraries import ( "context" + "fmt" "path/filepath" "testing" @@ -11,8 +12,11 @@ import ( "github.com/databricks/cli/bundle/internal/bundletest" mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" + sdkconfig "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -334,7 +338,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 TestLocationOfVolumeInBundle(t *testing.T) { +func TestMatchVolumeInBundle(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Resources: config.Resources{ @@ -353,26 +357,159 @@ func TestLocationOfVolumeInBundle(t *testing.T) { bundletest.SetLocation(b, "resources.volumes.foo", "volume.yml") - // volume is in DAB directly. - l, ok := locationOfVolumeInBundle(b, "main", "my_schema", "my_volume") + // volume is in DAB. + path, locations, ok := matchVolumeInBundle(b, "main", "my_schema", "my_volume") assert.True(t, ok) - assert.Equal(t, dyn.Location{ + assert.Equal(t, []dyn.Location{{ File: "volume.yml", - }, l) + }}, locations) + assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) // wrong volume name - _, ok = locationOfVolumeInBundle(b, "main", "my_schema", "doesnotexist") + _, _, ok = matchVolumeInBundle(b, "main", "my_schema", "doesnotexist") assert.False(t, ok) // wrong schema name - _, ok = locationOfVolumeInBundle(b, "main", "doesnotexist", "my_volume") + _, _, ok = matchVolumeInBundle(b, "main", "doesnotexist", "my_volume") assert.False(t, ok) // schema name is interpolated. b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema}" - l, ok = locationOfVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") + path, locations, ok = matchVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") assert.True(t, ok) - assert.Equal(t, dyn.Location{ + assert.Equal(t, []dyn.Location{{ File: "volume.yml", - }, l) + }}, locations) + assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) +} + +func TestGetFilerForLibraries(t *testing.T) { + t.Run("valid wsfs", func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/foo/bar/artifacts", + }, + }, + } + + client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, "/foo/bar/artifacts/.internal", uploadPath) + + assert.IsType(t, &filer.WorkspaceFilesClient{}, client) + }) + + t.Run("valid uc volume", func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) + b.SetWorkpaceClient(m.WorkspaceClient) + + client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, "/Volumes/main/my_schema/my_volume/.internal", uploadPath) + + assert.IsType(t, &filer.FilesClient{}, client) + }) + + t.Run("volume not in DAB", func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/doesnotexist", + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) + 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") + }) + + t.Run("volume in DAB config", func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + VolumeType: "MANAGED", + SchemaName: "my_schema", + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.volumes.foo", "volume.yml") + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) + + _, _, diags := GetFilerForLibraries(context.Background(), b) + 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.", + Locations: []dyn.Location{{ + File: "volume.yml", + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.volumes.foo")}, + }) + }) + + t.Run("remote path is not set", func(t *testing.T) { + b := &bundle.Bundle{} + + _, _, diags := GetFilerForLibraries(context.Background(), b) + require.EqualError(t, diags.Error(), "remote artifact path not configured") + }) + + t.Run("invalid volume paths", func(t *testing.T) { + invalidPaths := []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", + } + + for _, path := range invalidPaths { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: path, + }, + }, + } + + _, _, diags := GetFilerForLibraries(context.Background(), b) + require.EqualError(t, diags.Error(), fmt.Sprintf("expected UC volume path to be in the format /Volumes///, got %s", filepath.Join(path, ".internal"))) + } + }) } diff --git a/libs/filer/workspace_files_client.go b/libs/filer/workspace_files_client.go index d8ab5a6bbe..5e2a9813bd 100644 --- a/libs/filer/workspace_files_client.go +++ b/libs/filer/workspace_files_client.go @@ -114,7 +114,7 @@ type apiClient interface { // NOTE: This API is available for files under /Repos if a workspace has files-in-repos enabled. // It can access any workspace path if files-in-workspace is enabled. -type workspaceFilesClient struct { +type WorkspaceFilesClient struct { workspaceClient *databricks.WorkspaceClient apiClient apiClient @@ -128,7 +128,7 @@ func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, return nil, err } - return &workspaceFilesClient{ + return &WorkspaceFilesClient{ workspaceClient: w, apiClient: apiClient, @@ -136,7 +136,7 @@ func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, }, nil } -func (w *workspaceFilesClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { +func (w *WorkspaceFilesClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { absPath, err := w.root.Join(name) if err != nil { return err @@ -206,7 +206,7 @@ func (w *workspaceFilesClient) Write(ctx context.Context, name string, reader io return err } -func (w *workspaceFilesClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { +func (w *WorkspaceFilesClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { absPath, err := w.root.Join(name) if err != nil { return nil, err @@ -230,7 +230,7 @@ func (w *workspaceFilesClient) Read(ctx context.Context, name string) (io.ReadCl return w.workspaceClient.Workspace.Download(ctx, absPath) } -func (w *workspaceFilesClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { +func (w *WorkspaceFilesClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { absPath, err := w.root.Join(name) if err != nil { return err @@ -274,7 +274,7 @@ func (w *workspaceFilesClient) Delete(ctx context.Context, name string, mode ... return err } -func (w *workspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { +func (w *WorkspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { absPath, err := w.root.Join(name) if err != nil { return nil, err @@ -307,7 +307,7 @@ func (w *workspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.D return wsfsDirEntriesFromObjectInfos(objects), nil } -func (w *workspaceFilesClient) Mkdir(ctx context.Context, name string) error { +func (w *WorkspaceFilesClient) Mkdir(ctx context.Context, name string) error { dirPath, err := w.root.Join(name) if err != nil { return err @@ -317,7 +317,7 @@ func (w *workspaceFilesClient) Mkdir(ctx context.Context, name string) error { }) } -func (w *workspaceFilesClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { +func (w *WorkspaceFilesClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { absPath, err := w.root.Join(name) if err != nil { return nil, err diff --git a/libs/filer/workspace_files_extensions_client_test.go b/libs/filer/workspace_files_extensions_client_test.go index 321c437128..33c5588846 100644 --- a/libs/filer/workspace_files_extensions_client_test.go +++ b/libs/filer/workspace_files_extensions_client_test.go @@ -123,7 +123,7 @@ func TestFilerWorkspaceFilesExtensionsErrorsOnDupName(t *testing.T) { "return_export_info": "true", }, mock.AnythingOfType("*filer.wsfsFileInfo"), []func(*http.Request) error(nil)).Return(nil, statNotebook) - workspaceFilesClient := workspaceFilesClient{ + workspaceFilesClient := WorkspaceFilesClient{ workspaceClient: mockedWorkspaceClient.WorkspaceClient, apiClient: &mockedApiClient, root: NewWorkspaceRootPath("/dir"), From a90eb57a5beff3d165cd088a24605f55a13c168c Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 03:00:22 +0200 Subject: [PATCH 16/69] fix unit tests --- bundle/libraries/upload.go | 15 +++++++++------ bundle/libraries/upload_test.go | 5 +++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 163b56131e..f8d331eb33 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -131,13 +131,15 @@ 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 - var uploadPath string - var diags diag.Diagnostics + 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. if u.client == nil { - u.client, uploadPath, diags = GetFilerForLibraries(ctx, b) - if diags.HasError() { - return diags - } + u.client = client } libs, err := collectLocalLibraries(b) @@ -189,6 +191,7 @@ func (u *upload) Name() string { return "libraries.Upload" } +// TODO: As a followup add testing for interactive flows. // This function returns the right filer to use, to upload artifacts to the configured locations. // Supported locations: // 1. WSFS diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index f15e2b93e4..f7b8a2ffc0 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -189,6 +189,11 @@ func TestArtifactUploadForVolumes(t *testing.T) { filer.CreateParentDirectories, ).Return(nil) + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/foo/bar/artifacts").Return(nil) + b.SetWorkpaceClient(m.WorkspaceClient) + diags := bundle.Apply(context.Background(), b, bundle.Seq(ExpandGlobReferences(), UploadWithClient(mockFiler))) require.NoError(t, diags.Error()) From 39cb5e84719004baa1f760c745824a8941cdfaa2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 03:08:42 +0200 Subject: [PATCH 17/69] fix test on windows --- bundle/libraries/upload_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index f7b8a2ffc0..1951105c6d 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -3,6 +3,7 @@ package libraries import ( "context" "fmt" + "path" "path/filepath" "testing" @@ -504,17 +505,17 @@ func TestGetFilerForLibraries(t *testing.T) { "/Volumes//my_schema/my_volume", } - for _, path := range invalidPaths { + for _, p := range invalidPaths { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - ArtifactPath: path, + ArtifactPath: p, }, }, } _, _, diags := GetFilerForLibraries(context.Background(), b) - require.EqualError(t, diags.Error(), fmt.Sprintf("expected UC volume path to be in the format /Volumes///, got %s", filepath.Join(path, ".internal"))) + require.EqualError(t, diags.Error(), fmt.Sprintf("expected UC volume path to be in the format /Volumes///, got %s", path.Join(p, ".internal"))) } }) } From 13748f177d605a9698c93228549d6394cbd3cc09 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 03:14:30 +0200 Subject: [PATCH 18/69] cleanup todos --- bundle/config/resources/volume.go | 3 --- bundle/deploy/terraform/tfdyn/convert_volume.go | 4 ---- 2 files changed, 7 deletions(-) diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go index 0ee4dce8ae..386d2ee376 100644 --- a/bundle/config/resources/volume.go +++ b/bundle/config/resources/volume.go @@ -9,13 +9,10 @@ type Volume struct { // List of grants to apply on this schema. Grants []Grant `json:"grants,omitempty"` - // TODO: Confirm the accuracy of this comment. // Full name of the schema (catalog_name.schema_name.volume_name). This value is read from // the terraform state after deployment succeeds. ID string `json:"id,omitempty" bundle:"readonly"` - // TODO: Are there fields in the edit API or terraform that are not in this struct? - // If so call it out in the PR. *catalog.CreateVolumeRequestContent ModifiedStatus ModifiedStatus `json:"modified_status,omitempty" bundle:"internal"` diff --git a/bundle/deploy/terraform/tfdyn/convert_volume.go b/bundle/deploy/terraform/tfdyn/convert_volume.go index 281cd43262..4211e1f9e1 100644 --- a/bundle/deploy/terraform/tfdyn/convert_volume.go +++ b/bundle/deploy/terraform/tfdyn/convert_volume.go @@ -10,10 +10,6 @@ import ( "github.com/databricks/cli/libs/log" ) -// TODO: Articulate the consequences of deleting a UC volume in the prompt message that -// is displayed. -// TODO: What sort of interpolation should be allowed at `artifact_path`? Should it be -// ${volumes.foo.id} or ${volumes.foo.name} or something else? func convertVolumeResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { // Normalize the output value to the target schema. vout, diags := convert.Normalize(schema.ResourceVolume{}, vin) From bdecd08206f817b73193316b8efbc5b4fc96ab40 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 03:17:05 +0200 Subject: [PATCH 19/69] - --- bundle/libraries/upload.go | 1 - bundle/phases/deploy.go | 2 -- 2 files changed, 3 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index f8d331eb33..7053a44f06 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -191,7 +191,6 @@ func (u *upload) Name() string { return "libraries.Upload" } -// TODO: As a followup add testing for interactive flows. // This function returns the right filer to use, to upload artifacts to the configured locations. // Supported locations: // 1. WSFS diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index d71c61912d..c32dda1595 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -25,8 +25,6 @@ import ( func filterDeleteOrRecreateActions(changes []*tfjson.ResourceChange, resourceType string) []terraformlib.Action { res := make([]terraformlib.Action, 0) for _, rc := range changes { - // TODO: Add end to end integration tests for the interactive prompt UXs. - // Good PR to introduce the first one, and make changes more confidently. if rc.Type != resourceType { continue } From 274fd636e0dc4528c6eb8c83cdad23732cf9c1c8 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 03:50:40 +0200 Subject: [PATCH 20/69] iter --- bundle/libraries/upload.go | 28 ++++++++++++------------- bundle/libraries/upload_test.go | 21 +++++++++++-------- internal/bundle/artifacts_test.go | 34 +++++++++++++++++++------------ libs/dyn/dynvar/ref.go | 4 ---- 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 7053a44f06..07fbd56b90 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 1951105c6d..6ee72d4e30 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 213aed2024..5c7d094024 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 088b68d262..338ac8ce6c 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) != "" -} From d3d5d4c0d6ffaf17102321d7da199d0db96226fb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 03:54:15 +0200 Subject: [PATCH 21/69] - --- bundle/libraries/upload.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 07fbd56b90..36d64e4320 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -196,11 +196,11 @@ func (u *upload) Name() string { // // If a UC Volume is configured, this function checks if the UC volume exists in the workspace. // Then: -// 1. If the UC volume existing in the workspace: +// 1. If the UC volume exists in the workspace: // Returns a filer for the UC volume. // 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 +// Returns an error and a warning that instructs the user to deploy the // UC volume before using it in the artifact path. // 3. If the UC volume does not exist in the workspace and is not defined in the bundle configuration: // Returns an error. From 227dfe95cab083c9af5b259b6269ad030061a287 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 16 Sep 2024 04:07:37 +0200 Subject: [PATCH 22/69] fixes --- bundle/libraries/upload_test.go | 2 +- internal/bundle/deploy_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 6ee72d4e30..a1a601e87d 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -499,7 +499,7 @@ func TestGetFilerForLibraries(t *testing.T) { t.Run("invalid volume paths", func(t *testing.T) { invalidPaths := []string{ - "/Volumes", + "/Volumes/", "/Volumes/main", "/Volumes/main/", "/Volumes/main//", diff --git a/internal/bundle/deploy_test.go b/internal/bundle/deploy_test.go index 225458e195..849fd98bf3 100644 --- a/internal/bundle/deploy_test.go +++ b/internal/bundle/deploy_test.go @@ -282,7 +282,7 @@ volumes the upstream data in the cloud tenant is not affected: recreate volume foo`) assert.Contains(t, stdout.String(), "the deployment requires destructive actions, but current console does not support prompting. Please specify --auto-approve if you would like to skip prompts and proceed") - // Recreation of the volume without --auto-approve should fail since prompting is not possible + // Successfully recreate the volume with --auto-approve t.Setenv("TERM", "dumb") t.Setenv("BUNDLE_ROOT", bundleRoot) _, _, err = internal.NewCobraTestRunnerWithContext(t, ctx, "bundle", "deploy", "--var=schema_name=${resources.schemas.schema2.name}", "--auto-approve").Run() From f919e94bce5d23a39c36fe79b9fc7d2e36a47bee Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 14 Oct 2024 15:05:33 +0200 Subject: [PATCH 23/69] typo fix --- bundle/config/resources/volume.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go index 386d2ee376..f7af05bdaa 100644 --- a/bundle/config/resources/volume.go +++ b/bundle/config/resources/volume.go @@ -6,10 +6,10 @@ import ( ) type Volume struct { - // List of grants to apply on this schema. + // List of grants to apply on this volume. Grants []Grant `json:"grants,omitempty"` - // Full name of the schema (catalog_name.schema_name.volume_name). This value is read from + // Full name of the volume (catalog_name.schema_name.volume_name). This value is read from // the terraform state after deployment succeeds. ID string `json:"id,omitempty" bundle:"readonly"` From 992126392888fe87ba3227a2e99a4868dc81eaa8 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 14 Oct 2024 15:18:58 +0200 Subject: [PATCH 24/69] separate GetFilerForLibraries tests --- bundle/libraries/upload_test.go | 228 +++++++++++++++++--------------- 1 file changed, 122 insertions(+), 106 deletions(-) diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index a1a601e87d..955169fadc 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -361,13 +361,21 @@ func TestFindVolumeInBundle(t *testing.T) { }, } - bundletest.SetLocation(b, "resources.volumes.foo", "volume.yml") + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ + { + File: "volume.yml", + Line: 1, + Column: 2, + }, + }) // volume is in DAB. path, locations, ok := findVolumeInBundle(b, "main", "my_schema", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ - File: "volume.yml", + File: "volume.yml", + Line: 1, + Column: 2, }}, locations) assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) @@ -388,139 +396,147 @@ func TestFindVolumeInBundle(t *testing.T) { path, locations, ok = findVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ - File: "volume.yml", + File: "volume.yml", + Line: 1, + Column: 2, }}, locations) assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) } -func TestGetFilerForLibraries(t *testing.T) { - t.Run("valid wsfs", func(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/foo/bar/artifacts", - }, +func TestGetFilerForLibrariesValidWsfs(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/foo/bar/artifacts", }, - } + }, + } - client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) - require.NoError(t, diags.Error()) - assert.Equal(t, "/foo/bar/artifacts/.internal", uploadPath) + client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, "/foo/bar/artifacts/.internal", uploadPath) - assert.IsType(t, &filer.WorkspaceFilesClient{}, client) - }) + assert.IsType(t, &filer.WorkspaceFilesClient{}, client) +} - t.Run("valid uc volume", func(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/my_volume", - }, +func TestGetFilerForLibrariesValidUcVolume(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", }, - } + }, + } - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) - b.SetWorkpaceClient(m.WorkspaceClient) + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) + b.SetWorkpaceClient(m.WorkspaceClient) - client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) - require.NoError(t, diags.Error()) - assert.Equal(t, "/Volumes/main/my_schema/my_volume/.internal", uploadPath) + client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, "/Volumes/main/my_schema/my_volume/.internal", uploadPath) - assert.IsType(t, &filer.FilesClient{}, client) - }) + assert.IsType(t, &filer.FilesClient{}, client) +} - t.Run("volume not in DAB", func(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/doesnotexist", - }, +func TestGetFilerForLibrariesVolumeNotInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/doesnotexist", }, - } + }, + } - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) - b.SetWorkpaceClient(m.WorkspaceClient) + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) - _, _, diags := GetFilerForLibraries(context.Background(), b) - 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) - }) + _, _, diags := GetFilerForLibraries(context.Background(), b) + 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) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/my_volume", - }, - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - VolumeType: "MANAGED", - SchemaName: "my_schema", - }, +func TestGetFilerForLibrariesVolumeInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + VolumeType: "MANAGED", + SchemaName: "my_schema", }, }, }, }, - } + }, + } - bundletest.SetLocation(b, "resources.volumes.foo", "volume.yml") + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ + { + File: "volume.yml", + Line: 1, + Column: 2, + }, + }) - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) - b.SetWorkpaceClient(m.WorkspaceClient) + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) - _, _, diags := GetFilerForLibraries(context.Background(), b) - 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: "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", - }}, - Paths: []dyn.Path{dyn.MustPathFromString("resources.volumes.foo")}, - }) + _, _, diags := GetFilerForLibraries(context.Background(), b) + 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: "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", + Line: 1, + Column: 2, + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.volumes.foo")}, }) +} - t.Run("remote path is not set", func(t *testing.T) { - b := &bundle.Bundle{} +func TestGetFilerForLibrariesVolumeInBundleWithArtifactPath(t *testing.T) { + b := &bundle.Bundle{} - _, _, diags := GetFilerForLibraries(context.Background(), b) - require.EqualError(t, diags.Error(), "remote artifact path not configured") - }) + _, _, diags := GetFilerForLibraries(context.Background(), b) + require.EqualError(t, diags.Error(), "remote artifact path not configured") +} - t.Run("invalid volume paths", func(t *testing.T) { - invalidPaths := []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 TestGetFilerForLibrariesWithInvalidVolumePaths(t *testing.T) { + invalidPaths := []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", + } - for _, p := range invalidPaths { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: p, - }, + for _, p := range invalidPaths { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: p, }, - } - - _, _, 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"))) + }, } - }) + + _, _, 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"))) + } } From 266c26ce09adbb13b72f82851cf1fce759e46515 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 14 Oct 2024 15:41:47 +0200 Subject: [PATCH 25/69] fix tesT --- internal/bundle/artifacts_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index cf12282fb7..4030db08d1 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -249,7 +249,7 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { dir := t.TempDir() b := &bundle.Bundle{ - RootPath: dir, + BundleRootPath: dir, SyncRootPath: dir, Config: config.Root{ Bundle: config.Bundle{ @@ -282,7 +282,7 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { dir := t.TempDir() b := &bundle.Bundle{ - RootPath: dir, + BundleRootPath: dir, SyncRootPath: dir, Config: config.Root{ Bundle: config.Bundle{ From a9b8575bc3667e7537d3232cba082354e2f18e54 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 14 Oct 2024 15:49:19 +0200 Subject: [PATCH 26/69] fmt and fix test --- bundle/config/resources.go | 1 + internal/bundle/artifacts_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 33040e7cf9..d3ddde8944 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -76,6 +76,7 @@ func SupportedResources() map[string]ResourceDescription { "registered_models": {SingularName: "registered_model"}, "quality_monitors": {SingularName: "quality_monitor"}, "schemas": {SingularName: "schema"}, + "volumes": {SingularName: "volume"}, "clusters": {SingularName: "cluster"}, } } diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 4030db08d1..d2938fa72c 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -249,8 +249,8 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { dir := t.TempDir() b := &bundle.Bundle{ - BundleRootPath: dir, - SyncRootPath: dir, + BundleRootPath: dir, + SyncRootPath: dir, Config: config.Root{ Bundle: config.Bundle{ Target: "whatever", @@ -282,8 +282,8 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { dir := t.TempDir() b := &bundle.Bundle{ - BundleRootPath: dir, - SyncRootPath: dir, + BundleRootPath: dir, + SyncRootPath: dir, Config: config.Root{ Bundle: config.Bundle{ Target: "whatever", From c5a02ef8fba4ef165b3d9a0fd00065966a4782d3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 15 Oct 2024 15:22:41 +0200 Subject: [PATCH 27/69] split into filer files --- bundle/libraries/filer.go | 29 ++++ bundle/libraries/filer_test.go | 63 ++++++++ bundle/libraries/filer_volume.go | 94 ++++++++++++ bundle/libraries/filer_volume_test.go | 204 ++++++++++++++++++++++++++ bundle/libraries/filer_workspace.go | 15 ++ bundle/libraries/upload.go | 96 ------------ bundle/libraries/upload_test.go | 204 -------------------------- 7 files changed, 405 insertions(+), 300 deletions(-) create mode 100644 bundle/libraries/filer.go create mode 100644 bundle/libraries/filer_test.go create mode 100644 bundle/libraries/filer_volume.go create mode 100644 bundle/libraries/filer_volume_test.go create mode 100644 bundle/libraries/filer_workspace.go diff --git a/bundle/libraries/filer.go b/bundle/libraries/filer.go new file mode 100644 index 0000000000..74f8175d91 --- /dev/null +++ b/bundle/libraries/filer.go @@ -0,0 +1,29 @@ +package libraries + +import ( + "context" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/filer" +) + +// This function returns the right filer to use, to upload artifacts to the configured location. +// Supported locations: +// 1. WSFS +// 2. UC volumes +func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { + artifactPath := b.Config.Workspace.ArtifactPath + if artifactPath == "" { + return nil, "", diag.Errorf("remote artifact path not configured") + } + + switch { + case strings.HasPrefix(artifactPath, "/Volumes/"): + return filerForVolume(ctx, b) + + default: + return filerForWorkspace(b) + } +} diff --git a/bundle/libraries/filer_test.go b/bundle/libraries/filer_test.go new file mode 100644 index 0000000000..88ba152fc1 --- /dev/null +++ b/bundle/libraries/filer_test.go @@ -0,0 +1,63 @@ +package libraries + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/filer" + sdkconfig "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestGetFilerForLibrariesValidWsfs(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/foo/bar/artifacts", + }, + }, + } + + client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, "/foo/bar/artifacts/.internal", uploadPath) + + assert.IsType(t, &filer.WorkspaceFilesClient{}, client) +} + +func TestGetFilerForLibrariesValidUcVolume(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) + b.SetWorkpaceClient(m.WorkspaceClient) + + client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, "/Volumes/main/my_schema/my_volume/.internal", uploadPath) + + assert.IsType(t, &filer.FilesClient{}, client) +} + +func TestGetFilerForLibrariesRemotePathNotSet(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{}, + }, + } + + _, _, diags := GetFilerForLibraries(context.Background(), b) + require.EqualError(t, diags.Error(), "remote artifact path not configured") +} diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go new file mode 100644 index 0000000000..7308b65e2e --- /dev/null +++ b/bundle/libraries/filer_volume.go @@ -0,0 +1,94 @@ +package libraries + +import ( + "context" + "fmt" + "path" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" + "github.com/databricks/cli/libs/filer" +) + +// This function returns a filer for ".internal" folder inside the directory configured +// at `workspace.artifact_path`. +// This function also checks if the UC volume exists in the workspace and then: +// 1. If the UC volume exists in the workspace: +// Returns a filer for the UC volume. +// 2. If the UC volume does not exist in the workspace but is (with high confidence) defined in +// the bundle configuration: +// Returns an error and a warning that instructs the user to deploy the +// UC volume before using it in the artifact path. +// 3. If the UC volume does not exist in the workspace and is not defined in the bundle configuration: +// Returns an error. +func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { + artifactPath := b.Config.Workspace.ArtifactPath + + w := b.WorkspaceClient() + + 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 nil, "", diag.FromErr(volumeFormatErr) + } + + catalogName := parts[2] + schemaName := parts[3] + volumeName := parts[4] + + // Incorrect format. + if catalogName == "" || schemaName == "" || volumeName == "" { + return nil, "", diag.FromErr(volumeFormatErr) + } + + // Check if the UC volume exists in the workspace. + volumePath := fmt.Sprintf("/Volumes/%s/%s/%s", catalogName, schemaName, volumeName) + err := w.Files.GetDirectoryMetadataByDirectoryPath(ctx, volumePath) + + // If the volume exists already, directly return the filer for the path to + // upload the artifacts to. + if err == nil { + uploadPath := path.Join(artifactPath, ".internal") + f, err := filer.NewFilesClient(w, uploadPath) + return f, uploadPath, diag.FromErr(err) + } + + 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 := findVolumeInBundle(b, catalogName, schemaName, volumeName) + if !ok { + return nil, "", diags + } + + warning := 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: locations, + Paths: []dyn.Path{path}, + } + return nil, "", diags.Append(warning) +} + +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 { + continue + } + // UC schemas can be defined in the bundle itself, and thus might be interpolated + // 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.IsPureVariableReference(v.SchemaName) { + continue + } + pathString := fmt.Sprintf("resources.volumes.%s", k) + return dyn.MustPathFromString(pathString), b.Config.GetLocations(pathString), true + } + return nil, nil, false +} diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go new file mode 100644 index 0000000000..82b8ab5500 --- /dev/null +++ b/bundle/libraries/filer_volume_test.go @@ -0,0 +1,204 @@ +package libraries + +import ( + "context" + "fmt" + "path" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/filer" + sdkconfig "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestFindVolumeInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + SchemaName: "my_schema", + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ + { + File: "volume.yml", + Line: 1, + Column: 2, + }, + }) + + // volume is in DAB. + path, locations, ok := findVolumeInBundle(b, "main", "my_schema", "my_volume") + assert.True(t, ok) + assert.Equal(t, []dyn.Location{{ + File: "volume.yml", + Line: 1, + Column: 2, + }}, locations) + assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) + + // wrong volume name + _, _, ok = findVolumeInBundle(b, "main", "my_schema", "doesnotexist") + assert.False(t, ok) + + // wrong schema name + _, _, 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 = findVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") + assert.True(t, ok) + assert.Equal(t, []dyn.Location{{ + File: "volume.yml", + Line: 1, + Column: 2, + }}, locations) + assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) +} + +func TestFilerForVolumeNotInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/doesnotexist", + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) + + _, _, diags := filerForVolume(context.Background(), b) + 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) +} + +func TestFilerForVolumeInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + VolumeType: "MANAGED", + SchemaName: "my_schema", + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ + { + File: "volume.yml", + Line: 1, + Column: 2, + }, + }) + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) + + _, _, diags := GetFilerForLibraries(context.Background(), b) + 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: "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", + Line: 1, + Column: 2, + }}, + Paths: []dyn.Path{dyn.MustPathFromString("resources.volumes.foo")}, + }) +} + +func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { + invalidPaths := []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", + } + + for _, p := range invalidPaths { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: p, + }, + }, + } + + _, _, 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"))) + } +} + +func TestFilerForVolumeWithValidlVolumePaths(t *testing.T) { + validPaths := []string{ + "/Volumes/main/my_schema/my_volume", + "/Volumes/main/my_schema/my_volume/", + "/Volumes/main/my_schema/my_volume/a/b/c", + "/Volumes/main/my_schema/my_volume/a/a/a", + } + + for _, p := range validPaths { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: p, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) + b.SetWorkpaceClient(m.WorkspaceClient) + + client, uploadPath, diags := filerForVolume(context.Background(), b) + require.NoError(t, diags.Error()) + assert.Equal(t, path.Join(p, ".internal"), uploadPath) + assert.IsType(t, &filer.FilesClient{}, client) + } +} diff --git a/bundle/libraries/filer_workspace.go b/bundle/libraries/filer_workspace.go new file mode 100644 index 0000000000..9172fef92b --- /dev/null +++ b/bundle/libraries/filer_workspace.go @@ -0,0 +1,15 @@ +package libraries + +import ( + "path" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/filer" +) + +func filerForWorkspace(b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { + uploadPath := path.Join(b.Config.Workspace.ArtifactPath, ".internal") + f, err := filer.NewWorkspaceFilesClient(b.WorkspaceClient(), uploadPath) + return f, uploadPath, diag.FromErr(err) +} diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 36d64e4320..8f1b425f09 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -13,7 +13,6 @@ import ( "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" - "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/log" @@ -189,101 +188,6 @@ func (u *upload) Name() string { return "libraries.Upload" } -// This function returns the right filer to use, to upload artifacts to the configured location. -// Supported locations: -// 1. WSFS -// 2. UC volumes -// -// If a UC Volume is configured, this function checks if the UC volume exists in the workspace. -// Then: -// 1. If the UC volume exists in the workspace: -// Returns a filer for the UC volume. -// 2. If the UC volume does not exist in the workspace but is (with high confidence) defined in -// the bundle configuration: -// Returns an error and a warning that instructs the user to deploy the -// UC volume before using it in the artifact path. -// 3. If the UC volume does not exist in the workspace and is not defined in the bundle configuration: -// Returns an error. -func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { - artifactPath := b.Config.Workspace.ArtifactPath - if artifactPath == "" { - return nil, "", diag.Errorf("remote artifact path not configured") - } - - w := b.WorkspaceClient() - 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 { - f, err := filer.NewWorkspaceFilesClient(w, uploadPath) - return f, uploadPath, diag.FromErr(err) - } - - parts := strings.Split(artifactPath, "/") - volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes////..., got %s", uploadPath) - - // Incorrect format. - if len(parts) < 5 { - return nil, "", diag.FromErr(volumeFormatErr) - } - - catalogName := parts[2] - schemaName := parts[3] - volumeName := parts[4] - - // Incorrect format. - if catalogName == "" || schemaName == "" || volumeName == "" { - return nil, "", diag.FromErr(volumeFormatErr) - } - - // Check if the UC volume exists in the workspace. - volumePath := fmt.Sprintf("/Volumes/%s/%s/%s", catalogName, schemaName, volumeName) - err := w.Files.GetDirectoryMetadataByDirectoryPath(ctx, volumePath) - - // If the volume exists already, directly return the filer for the upload path. - if err == nil { - f, err := filer.NewFilesClient(w, uploadPath) - return f, uploadPath, diag.FromErr(err) - } - - 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 := findVolumeInBundle(b, catalogName, schemaName, volumeName) - if !ok { - return nil, "", diags - } - - warning := 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: locations, - Paths: []dyn.Path{path}, - } - return nil, "", diags.Append(warning) -} - -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 { - continue - } - // UC schemas can be defined in the bundle itself, and thus might be interpolated - // 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.IsPureVariableReference(v.SchemaName) { - continue - } - pathString := fmt.Sprintf("resources.volumes.%s", k) - return dyn.MustPathFromString(pathString), b.Config.GetLocations(pathString), true - } - return nil, nil, false -} - // Function to upload file (a library, artifact and etc) to Workspace or UC volume func UploadFile(ctx context.Context, file string, client filer.Filer) error { filename := filepath.Base(file) diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 955169fadc..493785bf50 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -2,26 +2,19 @@ package libraries import ( "context" - "fmt" - "path" "path/filepath" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" "github.com/databricks/cli/internal/testutil" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" sdkconfig "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/experimental/mocks" - "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/mock" "github.com/stretchr/testify/require" ) @@ -343,200 +336,3 @@ func TestUploadMultipleLibraries(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies, "/Workspace/foo/bar/artifacts/.internal/source4.whl") require.Contains(t, b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies, "/Workspace/Users/foo@bar.com/mywheel.whl") } - -func TestFindVolumeInBundle(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - SchemaName: "my_schema", - }, - }, - }, - }, - }, - } - - bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ - { - File: "volume.yml", - Line: 1, - Column: 2, - }, - }) - - // volume is in DAB. - path, locations, ok := findVolumeInBundle(b, "main", "my_schema", "my_volume") - assert.True(t, ok) - assert.Equal(t, []dyn.Location{{ - File: "volume.yml", - Line: 1, - Column: 2, - }}, locations) - assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) - - // wrong volume name - _, _, ok = findVolumeInBundle(b, "main", "my_schema", "doesnotexist") - assert.False(t, ok) - - // wrong schema name - _, _, 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 = findVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") - assert.True(t, ok) - assert.Equal(t, []dyn.Location{{ - File: "volume.yml", - Line: 1, - Column: 2, - }}, locations) - assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) -} - -func TestGetFilerForLibrariesValidWsfs(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/foo/bar/artifacts", - }, - }, - } - - client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) - require.NoError(t, diags.Error()) - assert.Equal(t, "/foo/bar/artifacts/.internal", uploadPath) - - assert.IsType(t, &filer.WorkspaceFilesClient{}, client) -} - -func TestGetFilerForLibrariesValidUcVolume(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/my_volume", - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) - b.SetWorkpaceClient(m.WorkspaceClient) - - client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) - require.NoError(t, diags.Error()) - assert.Equal(t, "/Volumes/main/my_schema/my_volume/.internal", uploadPath) - - assert.IsType(t, &filer.FilesClient{}, client) -} - -func TestGetFilerForLibrariesVolumeNotInBundle(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/doesnotexist", - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) - b.SetWorkpaceClient(m.WorkspaceClient) - - _, _, diags := GetFilerForLibraries(context.Background(), b) - 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) -} - -func TestGetFilerForLibrariesVolumeInBundle(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/my_volume", - }, - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - VolumeType: "MANAGED", - SchemaName: "my_schema", - }, - }, - }, - }, - }, - } - - bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ - { - File: "volume.yml", - Line: 1, - Column: 2, - }, - }) - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) - b.SetWorkpaceClient(m.WorkspaceClient) - - _, _, diags := GetFilerForLibraries(context.Background(), b) - 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: "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", - Line: 1, - Column: 2, - }}, - Paths: []dyn.Path{dyn.MustPathFromString("resources.volumes.foo")}, - }) -} - -func TestGetFilerForLibrariesVolumeInBundleWithArtifactPath(t *testing.T) { - b := &bundle.Bundle{} - - _, _, diags := GetFilerForLibraries(context.Background(), b) - require.EqualError(t, diags.Error(), "remote artifact path not configured") -} - -func TestGetFilerForLibrariesWithInvalidVolumePaths(t *testing.T) { - invalidPaths := []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", - } - - for _, p := range invalidPaths { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: p, - }, - }, - } - - _, _, 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"))) - } -} From eb94cd671723bb3cae41176694200f2ab54582be Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 15 Oct 2024 15:27:58 +0200 Subject: [PATCH 28/69] - --- bundle/libraries/filer_volume.go | 5 ++++- bundle/libraries/filer_volume_test.go | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 7308b65e2e..0348ef6fa1 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -26,9 +26,12 @@ import ( // Returns an error. func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { artifactPath := b.Config.Workspace.ArtifactPath - w := b.WorkspaceClient() + if !strings.HasPrefix(artifactPath, "/Volumes/") { + return nil, "", diag.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) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 82b8ab5500..e45a7cb0f9 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -174,6 +174,19 @@ func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { } } +func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volume/main/my_schema/my_volume", + }, + }, + } + + _, _, diags := filerForVolume(context.Background(), b) + require.EqualError(t, diags.Error(), "expected artifact_path to start with /Volumes/, got /Volume/main/my_schema/my_volume") +} + func TestFilerForVolumeWithValidlVolumePaths(t *testing.T) { validPaths := []string{ "/Volumes/main/my_schema/my_volume", From 3e3ddfd0cbd6b842cbdc7c0dba587586fd63ecbb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 15 Oct 2024 15:29:24 +0200 Subject: [PATCH 29/69] fix test --- bundle/libraries/filer_volume_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index e45a7cb0f9..e4fe66302d 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -170,7 +170,7 @@ func TestFilerForVolumeWithInvalidVolumePaths(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", p)) } } From d241c2b39c915351a289a6b1a6950ad52e939c13 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 15 Oct 2024 16:05:23 +0200 Subject: [PATCH 30/69] add integration test for grant on volume --- .../uc_volume/template/databricks.yml.tmpl | 5 +++++ internal/bundle/deploy_test.go | 20 +++++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl b/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl index 9d377f8e7a..d7f31439b8 100644 --- a/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl +++ b/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl @@ -24,3 +24,8 @@ resources: schema_name: ${var.schema_name} volume_type: MANAGED comment: This volume was created from DABs. + + grants: + - principal: account users + privileges: + - WRITE_VOLUME diff --git a/internal/bundle/deploy_test.go b/internal/bundle/deploy_test.go index 72ec7aafcd..9399c648fa 100644 --- a/internal/bundle/deploy_test.go +++ b/internal/bundle/deploy_test.go @@ -265,12 +265,20 @@ func TestAccDeployUcVolume(t *testing.T) { catalogName := "main" schemaName := "schema1-" + uniqueId volumeName := "my_volume" - volume, err := w.Volumes.ReadByName(ctx, fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName)) + fullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) + volume, err := w.Volumes.ReadByName(ctx, fullName) require.NoError(t, err) require.Equal(t, volume.Name, volumeName) require.Equal(t, catalogName, volume.CatalogName) require.Equal(t, schemaName, volume.SchemaName) + // Assert that the grants were successfully applied. + grants, err := w.Grants.GetBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, fullName) + require.NoError(t, err) + assert.Len(t, grants.PrivilegeAssignments, 1) + assert.Equal(t, "account users", grants.PrivilegeAssignments[0].Principal) + assert.Equal(t, []catalog.Privilege{catalog.PrivilegeWriteVolume}, grants.PrivilegeAssignments[0].Privileges) + // Recreation of the volume without --auto-approve should fail since prompting is not possible t.Setenv("TERM", "dumb") t.Setenv("BUNDLE_ROOT", bundleRoot) @@ -290,9 +298,17 @@ volumes the upstream data in the cloud tenant is not affected: // Assert the volume is updated successfully schemaName = "schema2-" + uniqueId - volume, err = w.Volumes.ReadByName(ctx, fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName)) + fullName = fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) + volume, err = w.Volumes.ReadByName(ctx, fullName) require.NoError(t, err) require.Equal(t, volume.Name, volumeName) require.Equal(t, catalogName, volume.CatalogName) require.Equal(t, schemaName, volume.SchemaName) + + // assert that the grants were applied / retained on recreate. + grants, err = w.Grants.GetBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, fullName) + require.NoError(t, err) + assert.Len(t, grants.PrivilegeAssignments, 1) + assert.Equal(t, "account users", grants.PrivilegeAssignments[0].Principal) + assert.Equal(t, []catalog.Privilege{catalog.PrivilegeWriteVolume}, grants.PrivilegeAssignments[0].Privileges) } From 6192835d6324e046d6f348c5954199b01d9728c3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 15 Oct 2024 18:04:20 +0200 Subject: [PATCH 31/69] add custom prefixing behaviour for volumes --- bundle/config/mutator/apply_presets.go | 23 +++++- bundle/config/mutator/apply_presets_test.go | 87 ++++++++++++++++++++ bundle/config/mutator/process_target_mode.go | 6 +- bundle/libraries/filer_volume.go | 6 +- bundle/libraries/filer_volume_test.go | 8 +- libs/dyn/dynvar/ref.go | 20 +++++ libs/dyn/dynvar/ref_test.go | 31 +++++++ 7 files changed, 176 insertions(+), 5 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 23bf1ba4c7..4d5a4f51ec 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -11,6 +11,8 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" + "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/textutil" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -194,8 +196,25 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } // Apply the prefix to volumes - for i := range r.Volumes { - r.Volumes[i].Name = normalizePrefix(prefix) + r.Volumes[i].Name + for _, v := range r.Volumes { + if containsUserIdentity(v.CatalogName, b.Config.Workspace.CurrentUser) { + log.Debugf(ctx, "Skipping prefix for volume %s because catalog %s contains the current user's identity", v.Name, v.CatalogName) + continue + } + if containsUserIdentity(v.SchemaName, b.Config.Workspace.CurrentUser) { + log.Debugf(ctx, "Skipping prefix for volume %s because schema %s contains the current user's identity", v.Name, v.SchemaName) + continue + } + // We only have to check for ${resources.schemas...} references because any + // other valid reference (like ${var.foo}) would have been interpolated by this point. + if p, ok := dynvar.PureReferenceToPath(v.SchemaName); ok && p.HasPrefix(dyn.Path{dyn.Key("resources"), dyn.Key("schemas")}) { + log.Debugf(ctx, "Skipping prefix for volume %s because schema name %s is defined in the bundle and will be interpolated at runtime", v.Name, v.SchemaName) + continue + + } + + v.Name = normalizePrefix(prefix) + v.Name + // HTTP API for volumes doesn't yet support tags. It's only supported in // the Databricks UI and via the SQL API. } diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index d3165672c0..b6b71d46e9 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -9,7 +9,9 @@ import ( "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -125,6 +127,15 @@ func TestApplyPresetsPrefixForUcSchema(t *testing.T) { } } +func TestApplyPresentsReminderToAddSupportForSkippingPrefixes(t *testing.T) { + _, ok := config.SupportedResources()["catalogs"] + assert.False(t, ok, + `Since you are adding support for UC catalogs to DABs please ensure that +you add logic to skip applying presets.name_prefix for UC schemas, UC volumes and +any other resources that fall under a catalog in the UC hierarchy (like registered models). +Once you do so feel free to remove this test.`) +} + func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { tests := []struct { name string @@ -138,6 +149,8 @@ func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { volume: &resources.Volume{ CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ Name: "volume1", + CatalogName: "catalog1", + SchemaName: "schema1", }, }, want: "prefix_volume1", @@ -152,6 +165,72 @@ func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { }, want: "volume1", }, + { + name: "skip prefix when catalog name contains user short name", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + CatalogName: "dev_john_wick_targets", + }, + }, + want: "volume1", + }, + { + name: "skip prefix when catalog name contains user email", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + CatalogName: "dev_john.wick@continental.com_targets", + }, + }, + want: "volume1", + }, + { + name: "skip prefix when schema name contains user short name", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + SchemaName: "dev_john_wick_weapons", + }, + }, + want: "volume1", + }, + { + name: "skip prefix when schema name contains user email", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + SchemaName: "dev_john.wick@continental.com_targets", + }, + }, + want: "volume1", + }, + { + name: "skip prefix when schema is defined in the bundle and will be interpolated at runtime", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + SchemaName: "${resources.schemas.schema1.name}", + }, + }, + want: "volume1", + }, + { + name: "add prefix when schema is a reference without the resources.schemas prefix", + prefix: "[prefix]", + volume: &resources.Volume{ + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + SchemaName: "${foo.bar.baz}", + }, + }, + want: "prefix_volume1", + }, } for _, tt := range tests { @@ -166,6 +245,14 @@ func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { Presets: config.Presets{ NamePrefix: tt.prefix, }, + Workspace: config.Workspace{ + CurrentUser: &config.User{ + ShortName: "john_wick", + User: &iam.User{ + UserName: "john.wick@continental.com", + }, + }, + }, }, } diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 44b53681dd..870afe9d5b 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -63,6 +63,10 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { } } +func containsUserIdentity(s string, u *config.User) bool { + return strings.Contains(s, u.ShortName) || strings.Contains(s, u.UserName) +} + func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics p := b.Config.Presets @@ -92,7 +96,7 @@ func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { diags = diags.Extend(diag.Errorf("%s must start with '~/' or contain the current username to ensure uniqueness when using 'mode: development'", path)) } } - if p.NamePrefix != "" && !strings.Contains(p.NamePrefix, u.ShortName) && !strings.Contains(p.NamePrefix, u.UserName) { + if p.NamePrefix != "" && !containsUserIdentity(p.NamePrefix, u) { // Resources such as pipelines require a unique name, e.g. '[dev steve] my_pipeline'. // For this reason we require the name prefix to contain the current username; // it's a pitfall for users if they don't include it and later find out that diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 0348ef6fa1..ec88bb9452 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -87,7 +87,11 @@ func findVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName st // 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.IsPureVariableReference(v.SchemaName) { + // We only have to check for ${resources.schemas...} references because any + // other valid reference (like ${var.foo}) would have been interpolated by this point. + p, ok := dynvar.PureReferenceToPath(v.SchemaName) + isSchemaDefinedInBundle := ok && p.HasPrefix(dyn.Path{dyn.Key("resources"), dyn.Key("schemas")}) + if v.SchemaName != schemaName && !isSchemaDefinedInBundle { continue } pathString := fmt.Sprintf("resources.volumes.%s", k) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index e4fe66302d..047a009a13 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -68,8 +68,14 @@ func TestFindVolumeInBundle(t *testing.T) { _, _, ok = findVolumeInBundle(b, "doesnotexist", "my_schema", "my_volume") assert.False(t, ok) + // schema name is interpolated but does not have the right prefix. In this case + // we should not match the volume. + b.Config.Resources.Volumes["foo"].SchemaName = "${foo.bar.baz}" + _, _, ok = findVolumeInBundle(b, "main", "my_schema", "my_volume") + assert.False(t, ok) + // schema name is interpolated. - b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema}" + b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema.name}" path, locations, ok = findVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ diff --git a/libs/dyn/dynvar/ref.go b/libs/dyn/dynvar/ref.go index 338ac8ce6c..a28938823e 100644 --- a/libs/dyn/dynvar/ref.go +++ b/libs/dyn/dynvar/ref.go @@ -71,3 +71,23 @@ func (v ref) references() []string { func IsPureVariableReference(s string) bool { return len(s) > 0 && re.FindString(s) == s } + +// If s is a pure variable reference, this function returns the corresponding +// dyn.Path. Otherwise, it returns false. +func PureReferenceToPath(s string) (dyn.Path, bool) { + ref, ok := newRef(dyn.V(s)) + if !ok { + return nil, false + } + + if !ref.isPure() { + return nil, false + } + + p, err := dyn.NewPathFromString(ref.references()[0]) + if err != nil { + return nil, false + } + + return p, true +} diff --git a/libs/dyn/dynvar/ref_test.go b/libs/dyn/dynvar/ref_test.go index aff3643e02..4110732f89 100644 --- a/libs/dyn/dynvar/ref_test.go +++ b/libs/dyn/dynvar/ref_test.go @@ -51,3 +51,34 @@ func TestIsPureVariableReference(t *testing.T) { assert.False(t, IsPureVariableReference("prefix ${foo.bar}")) assert.True(t, IsPureVariableReference("${foo.bar}")) } + +func TestPureReferenceToPath(t *testing.T) { + for _, tc := range []struct { + in string + out string + ok bool + }{ + {"${foo.bar}", "foo.bar", true}, + {"${foo.bar.baz}", "foo.bar.baz", true}, + {"${foo.bar.baz[0]}", "foo.bar.baz[0]", true}, + {"${foo.bar.baz[0][1]}", "foo.bar.baz[0][1]", true}, + {"${foo.bar.baz[0][1].qux}", "foo.bar.baz[0][1].qux", true}, + + {"${foo.one}${foo.two}", "", false}, + {"prefix ${foo.bar}", "", false}, + {"${foo.bar} suffix", "", false}, + {"${foo.bar", "", false}, + {"foo.bar}", "", false}, + {"foo.bar", "", false}, + {"{foo.bar}", "", false}, + {"", "", false}, + } { + path, ok := PureReferenceToPath(tc.in) + if tc.ok { + assert.True(t, ok) + assert.Equal(t, dyn.MustPathFromString(tc.out), path) + } else { + assert.False(t, ok) + } + } +} From 701b1786a85d21903e3d1718229035bbc57dc906 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 15 Oct 2024 22:03:29 +0200 Subject: [PATCH 32/69] fmt --- bundle/config/mutator/apply_presets_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index b6b71d46e9..50c5618bed 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -148,7 +148,7 @@ func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { prefix: "[prefix]", volume: &resources.Volume{ CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", + Name: "volume1", CatalogName: "catalog1", SchemaName: "schema1", }, From 810da66809cf5771da20ce12926408c0cf72a2f3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 16 Oct 2024 13:36:55 +0200 Subject: [PATCH 33/69] - --- bundle/config/mutator/apply_presets.go | 2 +- bundle/deploy/terraform/convert_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 4d5a4f51ec..9d79835d5b 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -208,7 +208,7 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // We only have to check for ${resources.schemas...} references because any // other valid reference (like ${var.foo}) would have been interpolated by this point. if p, ok := dynvar.PureReferenceToPath(v.SchemaName); ok && p.HasPrefix(dyn.Path{dyn.Key("resources"), dyn.Key("schemas")}) { - log.Debugf(ctx, "Skipping prefix for volume %s because schema name %s is defined in the bundle and will be interpolated at runtime", v.Name, v.SchemaName) + log.Debugf(ctx, "Skipping prefix for volume %s because schema %s is defined in the bundle and the schema name will be interpolated at runtime", v.Name, v.SchemaName) continue } diff --git a/bundle/deploy/terraform/convert_test.go b/bundle/deploy/terraform/convert_test.go index bd36c84a0d..6808076bcf 100644 --- a/bundle/deploy/terraform/convert_test.go +++ b/bundle/deploy/terraform/convert_test.go @@ -1110,7 +1110,8 @@ func TestTerraformToBundleModifiedResources(t *testing.T) { {Attributes: stateInstanceAttributes{ID: "2"}}, }, }, - {Type: "databricks_cluster", + { + Type: "databricks_cluster", Mode: "managed", Name: "test_cluster_old", Instances: []stateResourceInstance{ From 1a961eb19c71c0ef33483428d97b701e7bff6458 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 16 Oct 2024 13:46:45 +0200 Subject: [PATCH 34/69] - --- .../bundle/bundles/uc_volume/databricks_template_schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/bundle/bundles/uc_volume/databricks_template_schema.json b/internal/bundle/bundles/uc_volume/databricks_template_schema.json index 762f4470c2..d849dacf83 100644 --- a/internal/bundle/bundles/uc_volume/databricks_template_schema.json +++ b/internal/bundle/bundles/uc_volume/databricks_template_schema.json @@ -2,7 +2,7 @@ "properties": { "unique_id": { "type": "string", - "description": "Unique ID for the schema and pipeline names" + "description": "Unique ID for the schema names" } } } From e32ebd0b480fe8c3a506df04ac079d359312962c Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 31 Oct 2024 15:24:51 +0100 Subject: [PATCH 35/69] use IsVolumesPath --- bundle/libraries/filer.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bundle/libraries/filer.go b/bundle/libraries/filer.go index 74f8175d91..8cbf1a5101 100644 --- a/bundle/libraries/filer.go +++ b/bundle/libraries/filer.go @@ -2,7 +2,6 @@ package libraries import ( "context" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" @@ -20,7 +19,7 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, s } switch { - case strings.HasPrefix(artifactPath, "/Volumes/"): + case IsVolumesPath(artifactPath): return filerForVolume(ctx, b) default: From 6b122348adf1726a7ca7fc11d53615386ce5f671 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 31 Oct 2024 15:57:45 +0100 Subject: [PATCH 36/69] better message --- bundle/phases/deploy.go | 7 ++++--- internal/bundle/deploy_test.go | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 28b78f2865..f5fe42b034 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -96,9 +96,10 @@ properties such as the 'catalog' or 'storage' are changed:` // One or more volumes is being recreated. if len(volumeActions) != 0 { msg := ` -This action will result in the deletion or recreation of the following Volumes. For managed volumes, -this typically results in a deletion of the upstream data in the cloud tenant in ~30 days. For external -volumes the upstream data in the cloud tenant is not affected:` +This action will result in the deletion or recreation of the following Volumes. +For managed volumes, the files stored in the volume are also deleted from your +cloud tenant within 30 days. For external volumes, the metadata about the volume +is removed from the catalog, but the underlying files are not deleted:` cmdio.LogString(ctx, msg) for _, action := range volumeActions { cmdio.Log(ctx, action) diff --git a/internal/bundle/deploy_test.go b/internal/bundle/deploy_test.go index 9399c648fa..1956139a0e 100644 --- a/internal/bundle/deploy_test.go +++ b/internal/bundle/deploy_test.go @@ -284,9 +284,10 @@ func TestAccDeployUcVolume(t *testing.T) { t.Setenv("BUNDLE_ROOT", bundleRoot) stdout, stderr, err := internal.NewCobraTestRunnerWithContext(t, ctx, "bundle", "deploy", "--var=schema_name=${resources.schemas.schema2.name}").Run() assert.Error(t, err) - assert.Contains(t, stderr.String(), `This action will result in the deletion or recreation of the following Volumes. For managed volumes, -this typically results in a deletion of the upstream data in the cloud tenant in ~30 days. For external -volumes the upstream data in the cloud tenant is not affected: + assert.Contains(t, stderr.String(), `This action will result in the deletion or recreation of the following Volumes. +For managed volumes, the files stored in the volume are also deleted from your +cloud tenant within 30 days. For external volumes, the metadata about the volume +is removed from the catalog, but the underlying files are not deleted: recreate volume foo`) assert.Contains(t, stdout.String(), "the deployment requires destructive actions, but current console does not support prompting. Please specify --auto-approve if you would like to skip prompts and proceed") From f9287e01015719c5fd8b6d17794bbb099ff9b474 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 31 Oct 2024 17:52:45 +0100 Subject: [PATCH 37/69] address comments --- bundle/artifacts/expand_globs_test.go | 2 +- bundle/{internal => }/bundletest/location.go | 0 bundle/{internal => }/bundletest/mutate.go | 0 .../configure_dashboard_defaults_test.go | 2 +- .../expand_pipeline_glob_paths_test.go | 2 +- .../config/mutator/rewrite_sync_paths_test.go | 2 +- bundle/config/mutator/sync_infer_root_test.go | 2 +- bundle/config/mutator/translate_paths_test.go | 2 +- bundle/deploy/metadata/compute_test.go | 2 +- .../libraries/expand_glob_references_test.go | 2 +- bundle/libraries/filer_volume_test.go | 2 +- internal/bundle/artifacts_test.go | 22 +++++++++---------- 12 files changed, 20 insertions(+), 20 deletions(-) rename bundle/{internal => }/bundletest/location.go (100%) rename bundle/{internal => }/bundletest/mutate.go (100%) diff --git a/bundle/artifacts/expand_globs_test.go b/bundle/artifacts/expand_globs_test.go index dc7c77de70..af048f8e52 100644 --- a/bundle/artifacts/expand_globs_test.go +++ b/bundle/artifacts/expand_globs_test.go @@ -7,8 +7,8 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" diff --git a/bundle/internal/bundletest/location.go b/bundle/bundletest/location.go similarity index 100% rename from bundle/internal/bundletest/location.go rename to bundle/bundletest/location.go diff --git a/bundle/internal/bundletest/mutate.go b/bundle/bundletest/mutate.go similarity index 100% rename from bundle/internal/bundletest/mutate.go rename to bundle/bundletest/mutate.go diff --git a/bundle/config/mutator/configure_dashboard_defaults_test.go b/bundle/config/mutator/configure_dashboard_defaults_test.go index 4804b7159a..d34c35bc6a 100644 --- a/bundle/config/mutator/configure_dashboard_defaults_test.go +++ b/bundle/config/mutator/configure_dashboard_defaults_test.go @@ -5,10 +5,10 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/dashboards" "github.com/stretchr/testify/assert" diff --git a/bundle/config/mutator/expand_pipeline_glob_paths_test.go b/bundle/config/mutator/expand_pipeline_glob_paths_test.go index 9f70b74ae4..063c76b35c 100644 --- a/bundle/config/mutator/expand_pipeline_glob_paths_test.go +++ b/bundle/config/mutator/expand_pipeline_glob_paths_test.go @@ -7,9 +7,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/pipelines" diff --git a/bundle/config/mutator/rewrite_sync_paths_test.go b/bundle/config/mutator/rewrite_sync_paths_test.go index 0e4dfc4cec..90da879207 100644 --- a/bundle/config/mutator/rewrite_sync_paths_test.go +++ b/bundle/config/mutator/rewrite_sync_paths_test.go @@ -6,9 +6,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" ) diff --git a/bundle/config/mutator/sync_infer_root_test.go b/bundle/config/mutator/sync_infer_root_test.go index f507cbc7f8..98c9d36258 100644 --- a/bundle/config/mutator/sync_infer_root_test.go +++ b/bundle/config/mutator/sync_infer_root_test.go @@ -6,9 +6,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index 9d655b27bf..04325b6846 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -8,11 +8,11 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/config/variable" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/vfs" diff --git a/bundle/deploy/metadata/compute_test.go b/bundle/deploy/metadata/compute_test.go index 2c2c723769..2d852bf799 100644 --- a/bundle/deploy/metadata/compute_test.go +++ b/bundle/deploy/metadata/compute_test.go @@ -5,9 +5,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/bundle/metadata" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/jobs" diff --git a/bundle/libraries/expand_glob_references_test.go b/bundle/libraries/expand_glob_references_test.go index 2dfbddb743..bdd04216a4 100644 --- a/bundle/libraries/expand_glob_references_test.go +++ b/bundle/libraries/expand_glob_references_test.go @@ -6,9 +6,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 047a009a13..987c57cb72 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -7,9 +7,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index d2938fa72c..3cc731f435 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/libraries" @@ -244,6 +245,11 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { }) require.NoError(t, err) + t.Cleanup(func() { + err = w.Schemas.DeleteByFullName(ctx, "main."+schemaName) + require.NoError(t, err) + }) + t.Run("volume not in DAB", func(t *testing.T) { volumePath := fmt.Sprintf("/Volumes/main/%s/doesnotexist", schemaName) dir := t.TempDir() @@ -307,17 +313,11 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { } // set location of volume definition in config. - b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - return dyn.Map(v, "resources.volumes.foo", func(p dyn.Path, volume dyn.Value) (dyn.Value, error) { - return volume.WithLocations([]dyn.Location{ - { - File: filepath.Join(dir, "databricks.yml"), - Line: 1, - Column: 2, - }, - }), nil - }) - }) + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{{ + File: filepath.Join(dir, "databricks.yml"), + Line: 1, + Column: 2, + }}) diags := bundle.Apply(ctx, b, bundle.Seq(libraries.ExpandGlobReferences(), libraries.Upload())) assert.Contains(t, diags, diag.Diagnostic{ From 250d4265ceac562b8a41870c38164c311a8ee553 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 31 Oct 2024 18:00:31 +0100 Subject: [PATCH 38/69] rename to volume --- .../{uc_volume => volume}/databricks_template_schema.json | 0 .../bundles/{uc_volume => volume}/template/databricks.yml.tmpl | 0 internal/bundle/bundles/{uc_volume => volume}/template/nb.sql | 0 internal/bundle/deploy_test.go | 2 +- 4 files changed, 1 insertion(+), 1 deletion(-) rename internal/bundle/bundles/{uc_volume => volume}/databricks_template_schema.json (100%) rename internal/bundle/bundles/{uc_volume => volume}/template/databricks.yml.tmpl (100%) rename internal/bundle/bundles/{uc_volume => volume}/template/nb.sql (100%) diff --git a/internal/bundle/bundles/uc_volume/databricks_template_schema.json b/internal/bundle/bundles/volume/databricks_template_schema.json similarity index 100% rename from internal/bundle/bundles/uc_volume/databricks_template_schema.json rename to internal/bundle/bundles/volume/databricks_template_schema.json diff --git a/internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl b/internal/bundle/bundles/volume/template/databricks.yml.tmpl similarity index 100% rename from internal/bundle/bundles/uc_volume/template/databricks.yml.tmpl rename to internal/bundle/bundles/volume/template/databricks.yml.tmpl diff --git a/internal/bundle/bundles/uc_volume/template/nb.sql b/internal/bundle/bundles/volume/template/nb.sql similarity index 100% rename from internal/bundle/bundles/uc_volume/template/nb.sql rename to internal/bundle/bundles/volume/template/nb.sql diff --git a/internal/bundle/deploy_test.go b/internal/bundle/deploy_test.go index 1956139a0e..0ddc79f3b7 100644 --- a/internal/bundle/deploy_test.go +++ b/internal/bundle/deploy_test.go @@ -249,7 +249,7 @@ func TestAccDeployUcVolume(t *testing.T) { w := wt.W uniqueId := uuid.New().String() - bundleRoot, err := initTestTemplate(t, ctx, "uc_volume", map[string]any{ + bundleRoot, err := initTestTemplate(t, ctx, "volume", map[string]any{ "unique_id": uniqueId, }) require.NoError(t, err) From 1218178e64844a091fb06293b6fc14552273942b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 31 Oct 2024 18:01:37 +0100 Subject: [PATCH 39/69] - --- bundle/config/mutator/apply_presets.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index ffe9fc939d..2b60f52e6e 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -195,7 +195,7 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // the Databricks UI and via the SQL API. } - // Apply the prefix to volumes + // Volumes: Prefix for _, v := range r.Volumes { if containsUserIdentity(v.CatalogName, b.Config.Workspace.CurrentUser) { log.Debugf(ctx, "Skipping prefix for volume %s because catalog %s contains the current user's identity", v.Name, v.CatalogName) From 4cc2790300d41cd3f82709fc1355c3dc2ee92626 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:03:22 +0100 Subject: [PATCH 40/69] remove prefixing for uc volumes --- bundle/config/mutator/apply_presets.go | 27 ---- bundle/config/mutator/apply_presets_test.go | 153 +++--------------- .../translate_paths_dashboards_test.go | 2 +- 3 files changed, 21 insertions(+), 161 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 45ee79ef50..4472de0452 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -11,8 +11,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" - "github.com/databricks/cli/libs/dyn/dynvar" - "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/textutil" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -194,31 +192,6 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // HTTP API for schemas doesn't yet support tags. It's only supported in // the Databricks UI and via the SQL API. } - - // Volumes: Prefix - for _, v := range r.Volumes { - if containsUserIdentity(v.CatalogName, b.Config.Workspace.CurrentUser) { - log.Debugf(ctx, "Skipping prefix for volume %s because catalog %s contains the current user's identity", v.Name, v.CatalogName) - continue - } - if containsUserIdentity(v.SchemaName, b.Config.Workspace.CurrentUser) { - log.Debugf(ctx, "Skipping prefix for volume %s because schema %s contains the current user's identity", v.Name, v.SchemaName) - continue - } - // We only have to check for ${resources.schemas...} references because any - // other valid reference (like ${var.foo}) would have been interpolated by this point. - if p, ok := dynvar.PureReferenceToPath(v.SchemaName); ok && p.HasPrefix(dyn.Path{dyn.Key("resources"), dyn.Key("schemas")}) { - log.Debugf(ctx, "Skipping prefix for volume %s because schema %s is defined in the bundle and the schema name will be interpolated at runtime", v.Name, v.SchemaName) - continue - - } - - v.Name = normalizePrefix(prefix) + v.Name - - // HTTP API for volumes doesn't yet support tags. It's only supported in - // the Databricks UI and via the SQL API. - } - // Clusters: Prefix, Tags for key, c := range r.Clusters { if c.ClusterSpec == nil { diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 50c5618bed..7ca13291e8 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -9,9 +9,7 @@ import ( "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/catalog" - "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -127,145 +125,34 @@ func TestApplyPresetsPrefixForUcSchema(t *testing.T) { } } -func TestApplyPresentsReminderToAddSupportForSkippingPrefixes(t *testing.T) { - _, ok := config.SupportedResources()["catalogs"] - assert.False(t, ok, - `Since you are adding support for UC catalogs to DABs please ensure that -you add logic to skip applying presets.name_prefix for UC schemas, UC volumes and -any other resources that fall under a catalog in the UC hierarchy (like registered models). -Once you do so feel free to remove this test.`) -} - -func TestApplyPresetsPrefixForUcVolumes(t *testing.T) { - tests := []struct { - name string - prefix string - volume *resources.Volume - want string - }{ - { - name: "add prefix to volume", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - CatalogName: "catalog1", - SchemaName: "schema1", - }, - }, - want: "prefix_volume1", - }, - { - name: "add empty prefix to volume", - prefix: "", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - }, - }, - want: "volume1", - }, - { - name: "skip prefix when catalog name contains user short name", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - CatalogName: "dev_john_wick_targets", - }, - }, - want: "volume1", - }, - { - name: "skip prefix when catalog name contains user email", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - CatalogName: "dev_john.wick@continental.com_targets", - }, - }, - want: "volume1", - }, - { - name: "skip prefix when schema name contains user short name", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - SchemaName: "dev_john_wick_weapons", - }, - }, - want: "volume1", - }, - { - name: "skip prefix when schema name contains user email", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - SchemaName: "dev_john.wick@continental.com_targets", - }, - }, - want: "volume1", - }, - { - name: "skip prefix when schema is defined in the bundle and will be interpolated at runtime", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - SchemaName: "${resources.schemas.schema1.name}", +func TestApplyPresetsUCVolumesShouldNotBePrefixed(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "volume1": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + Name: "volume1", + CatalogName: "catalog1", + SchemaName: "schema1", + }, + }, }, }, - want: "volume1", - }, - { - name: "add prefix when schema is a reference without the resources.schemas prefix", - prefix: "[prefix]", - volume: &resources.Volume{ - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - Name: "volume1", - SchemaName: "${foo.bar.baz}", - }, + Presets: config.Presets{ + NamePrefix: "[prefix]", }, - want: "prefix_volume1", }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "volume1": tt.volume, - }, - }, - Presets: config.Presets{ - NamePrefix: tt.prefix, - }, - Workspace: config.Workspace{ - CurrentUser: &config.User{ - ShortName: "john_wick", - User: &iam.User{ - UserName: "john.wick@continental.com", - }, - }, - }, - }, - } - - ctx := context.Background() - diag := bundle.Apply(ctx, b, mutator.ApplyPresets()) - - if diag.HasError() { - t.Fatalf("unexpected error: %v", diag) - } + ctx := context.Background() + diag := bundle.Apply(ctx, b, mutator.ApplyPresets()) - require.Equal(t, tt.want, b.Config.Resources.Volumes["volume1"].Name) - }) + if diag.HasError() { + t.Fatalf("unexpected error: %v", diag) } + + require.Equal(t, "volume1", b.Config.Resources.Volumes["volume1"].Name) } func TestApplyPresetsTags(t *testing.T) { diff --git a/bundle/config/mutator/translate_paths_dashboards_test.go b/bundle/config/mutator/translate_paths_dashboards_test.go index 5e4e69f5d8..ae74f31438 100644 --- a/bundle/config/mutator/translate_paths_dashboards_test.go +++ b/bundle/config/mutator/translate_paths_dashboards_test.go @@ -6,10 +6,10 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go/service/dashboards" From 040626589a1e4cd911bf7691b85d5c926325d052 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:07:24 +0100 Subject: [PATCH 41/69] - --- bundle/config/mutator/apply_presets.go | 1 + 1 file changed, 1 insertion(+) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 4472de0452..95917738eb 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -192,6 +192,7 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // HTTP API for schemas doesn't yet support tags. It's only supported in // the Databricks UI and via the SQL API. } + // Clusters: Prefix, Tags for key, c := range r.Clusters { if c.ClusterSpec == nil { From b0e527efe899ecd60f9c160ab8c96ee22c5b8808 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:07:54 +0100 Subject: [PATCH 42/69] - --- bundle/config/mutator/apply_presets.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 95917738eb..59b8547be4 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -192,7 +192,7 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // HTTP API for schemas doesn't yet support tags. It's only supported in // the Databricks UI and via the SQL API. } - + // Clusters: Prefix, Tags for key, c := range r.Clusters { if c.ClusterSpec == nil { From e6723deb9db1ea959f5d557d3fcf37a07d1b8c5f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:10:28 +0100 Subject: [PATCH 43/69] undo containsUsername --- bundle/config/mutator/process_target_mode.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 870afe9d5b..44b53681dd 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -63,10 +63,6 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { } } -func containsUserIdentity(s string, u *config.User) bool { - return strings.Contains(s, u.ShortName) || strings.Contains(s, u.UserName) -} - func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics p := b.Config.Presets @@ -96,7 +92,7 @@ func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { diags = diags.Extend(diag.Errorf("%s must start with '~/' or contain the current username to ensure uniqueness when using 'mode: development'", path)) } } - if p.NamePrefix != "" && !containsUserIdentity(p.NamePrefix, u) { + if p.NamePrefix != "" && !strings.Contains(p.NamePrefix, u.ShortName) && !strings.Contains(p.NamePrefix, u.UserName) { // Resources such as pipelines require a unique name, e.g. '[dev steve] my_pipeline'. // For this reason we require the name prefix to contain the current username; // it's a pitfall for users if they don't include it and later find out that From f5ea8dac266ccf96a838c6bd88402b23235b840d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:35:03 +0100 Subject: [PATCH 44/69] remove other mutator --- bundle/libraries/upload.go | 4 ++-- internal/bundle/artifacts_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 8f1b425f09..4b6f43701b 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -133,8 +133,8 @@ func (u *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return diags } - // Only set the filer client if it's not already set. We use client field - // in mutator to mock the filer client in testing + // Only set the filer client if it's not already set. We use the client field + // in the mutator to mock the filer client in testing if u.client == nil { u.client = client } diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 3cc731f435..9496c1dc7b 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -279,7 +279,7 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { }, } - diags := bundle.Apply(ctx, b, bundle.Seq(libraries.ExpandGlobReferences(), libraries.Upload())) + diags := bundle.Apply(ctx, b, libraries.Upload()) assert.ErrorContains(t, diags.Error(), fmt.Sprintf("failed to fetch metadata for the UC volume %s that is configured in the artifact_path:", volumePath)) }) @@ -319,7 +319,7 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { Column: 2, }}) - diags := bundle.Apply(ctx, b, bundle.Seq(libraries.ExpandGlobReferences(), libraries.Upload())) + diags := bundle.Apply(ctx, b, libraries.Upload()) 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), From ea6906e88c1bf0b32019ad0e51eae408186dedad Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:52:17 +0100 Subject: [PATCH 45/69] add support for initializeUrl --- bundle/config/resources.go | 7 +++++++ bundle/config/resources/volume.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 2e3f4dbf48..f44e7e9ded 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -80,6 +80,7 @@ func (r *Resources) AllResources() []ResourceGroup { collectResourceMap(descriptions["schemas"], r.Schemas), collectResourceMap(descriptions["clusters"], r.Clusters), collectResourceMap(descriptions["dashboards"], r.Dashboards), + collectResourceMap(descriptions["volumes"], r.Volumes), } } @@ -184,5 +185,11 @@ func SupportedResources() map[string]ResourceDescription { SingularTitle: "Dashboard", PluralTitle: "Dashboards", }, + "volumes": { + SingularName: "volume", + PluralName: "volumes", + SingularTitle: "Volume", + PluralTitle: "Volumes", + }, } } diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go index f7af05bdaa..d459f9e138 100644 --- a/bundle/config/resources/volume.go +++ b/bundle/config/resources/volume.go @@ -1,6 +1,11 @@ package resources import ( + "context" + "fmt" + "net/url" + + "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/marshal" "github.com/databricks/databricks-sdk-go/service/catalog" ) @@ -16,6 +21,7 @@ type Volume struct { *catalog.CreateVolumeRequestContent ModifiedStatus ModifiedStatus `json:"modified_status,omitempty" bundle:"internal"` + URL string `json:"url,omitempty" bundle:"internal"` } func (v *Volume) UnmarshalJSON(b []byte) error { @@ -25,3 +31,28 @@ func (v *Volume) UnmarshalJSON(b []byte) error { func (v Volume) MarshalJSON() ([]byte, error) { return marshal.Marshal(v) } + +func (v *Volume) Exists(ctx context.Context, w *databricks.WorkspaceClient, id string) (bool, error) { + return false, fmt.Errorf("volume.Exists() is not supported") +} + +func (v *Volume) TerraformResourceName() string { + return "databricks_volume" +} + +// TODO: Test unit and manually. Maybe just manually. +func (v *Volume) InitializeURL(baseURL url.URL) { + if v.ID == "" { + return + } + baseURL.Path = fmt.Sprintf("explore/data/volumes/%s", v.ID) + v.URL = baseURL.String() +} + +func (v *Volume) GetURL() string { + return v.URL +} + +func (v *Volume) GetName() string { + return v.Name +} From 76092ccaa76706d17965f98b80f90f1b27a93eb7 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 17:58:07 +0100 Subject: [PATCH 46/69] remove todo --- bundle/config/resources/volume.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go index d459f9e138..4d04294953 100644 --- a/bundle/config/resources/volume.go +++ b/bundle/config/resources/volume.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/url" + "strings" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/marshal" @@ -40,12 +41,11 @@ func (v *Volume) TerraformResourceName() string { return "databricks_volume" } -// TODO: Test unit and manually. Maybe just manually. func (v *Volume) InitializeURL(baseURL url.URL) { if v.ID == "" { return } - baseURL.Path = fmt.Sprintf("explore/data/volumes/%s", v.ID) + baseURL.Path = fmt.Sprintf("explore/data/volumes/%s", strings.ReplaceAll(v.ID, ".", "/")) v.URL = baseURL.String() } From 039057fdd7af97e3307b4fe74f4bf01ebc1b3519 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 18 Nov 2024 18:11:55 +0100 Subject: [PATCH 47/69] fix renaming test --- .../mutator/process_target_mode_test.go | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index b87c72bef3..468ca7e2db 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -3,7 +3,7 @@ package mutator import ( "context" "reflect" - "strings" + "slices" "testing" "github.com/databricks/cli/bundle" @@ -391,10 +391,17 @@ func TestAllResourcesMocked(t *testing.T) { } } -// Make sure that we at least rename all resources -func TestAllResourcesRenamed(t *testing.T) { +// Make sure that we at rename all non UC resources +func TestAllNonUCResourcesAreRenamed(t *testing.T) { b := mockBundle(config.Development) + // UC resources should not have a prefix added to their name. Right now + // this list only contains the Volume resource since we have yet to remove + // prefixing support for UC schemas and registered models. + ucFields := []reflect.Type{ + reflect.TypeOf(&resources.Volume{}), + } + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -407,14 +414,14 @@ func TestAllResourcesRenamed(t *testing.T) { for _, key := range field.MapKeys() { resource := field.MapIndex(key) nameField := resource.Elem().FieldByName("Name") - if nameField.IsValid() && nameField.Kind() == reflect.String { - assert.True( - t, - strings.Contains(nameField.String(), "dev"), - "process_target_mode should rename '%s' in '%s'", - key, - resources.Type().Field(i).Name, - ) + if !nameField.IsValid() || nameField.Kind() != reflect.String { + continue + } + + if slices.Contains(ucFields, resource.Type()) { + assert.NotContains(t, nameField.String(), "dev", "process_target_mode should not rename '%s' in '%s'", key, resources.Type().Field(i).Name) + } else { + assert.Contains(t, nameField.String(), "dev", "process_target_mode should rename '%s' in '%s'", key, resources.Type().Field(i).Name) } } } From 5ec784bcaea4a17ecfd1a9acc98232cf0a7c1244 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 02:20:41 +0100 Subject: [PATCH 48/69] merge --- bundle/config/mutator/apply_presets_test.go | 2 +- bundle/config/resources/volume.go | 4 ++++ bundle/config/validate/single_node_cluster_test.go | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index dfbcf02d5c..b4b1840bee 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -9,7 +9,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" diff --git a/bundle/config/resources/volume.go b/bundle/config/resources/volume.go index 4d04294953..cae2a34637 100644 --- a/bundle/config/resources/volume.go +++ b/bundle/config/resources/volume.go @@ -56,3 +56,7 @@ func (v *Volume) GetURL() string { func (v *Volume) GetName() string { return v.Name } + +func (v *Volume) IsNil() bool { + return v.CreateVolumeRequestContent == nil +} diff --git a/bundle/config/validate/single_node_cluster_test.go b/bundle/config/validate/single_node_cluster_test.go index 18771cc00d..6c650f6e97 100644 --- a/bundle/config/validate/single_node_cluster_test.go +++ b/bundle/config/validate/single_node_cluster_test.go @@ -5,9 +5,9 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" From fcb8ffff3822900264dd475dcfac7d0e1d00d8a0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 02:22:10 +0100 Subject: [PATCH 49/69] - --- bundle/deploy/terraform/convert_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bundle/deploy/terraform/convert_test.go b/bundle/deploy/terraform/convert_test.go index bb6b96118c..076d9b7a05 100644 --- a/bundle/deploy/terraform/convert_test.go +++ b/bundle/deploy/terraform/convert_test.go @@ -1135,19 +1135,19 @@ func TestTerraformToBundleModifiedResources(t *testing.T) { }, }, { - Type: "databricks_cluster", + Type: "databricks_volume", Mode: "managed", - Name: "test_cluster", + Name: "test_volume_old", Instances: []stateResourceInstance{ - {Attributes: stateInstanceAttributes{ID: "1"}}, + {Attributes: stateInstanceAttributes{ID: "2"}}, }, }, { - Type: "databricks_volume", + Type: "databricks_cluster", Mode: "managed", - Name: "test_volume_old", + Name: "test_cluster", Instances: []stateResourceInstance{ - {Attributes: stateInstanceAttributes{ID: "2"}}, + {Attributes: stateInstanceAttributes{ID: "1"}}, }, }, { From e9a5544e8c0ba3950d1e526556d3695ffed3655c Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 02:29:18 +0100 Subject: [PATCH 50/69] - --- bundle/config/mutator/apply_presets_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index b4b1840bee..19050aee2d 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -73,7 +73,7 @@ func TestApplyPresetsPrefix(t *testing.T) { } } -func TestApplyPresetsPrefixForUcSchema(t *testing.T) { +func TestApplyPresetsPrefixForSchema(t *testing.T) { tests := []struct { name string prefix string @@ -129,7 +129,7 @@ func TestApplyPresetsPrefixForUcSchema(t *testing.T) { } } -func TestApplyPresetsUCVolumesShouldNotBePrefixed(t *testing.T) { +func TestApplyPresetsVolumesShouldNotBePrefixed(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Resources: config.Resources{ From 5531e2a0c00c6f1b06cdfc800e38f76b8da33b4f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 02:30:33 +0100 Subject: [PATCH 51/69] - --- bundle/config/mutator/process_target_mode_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index ebe2249fe1..14d524416e 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -396,7 +396,7 @@ func TestAllResourcesMocked(t *testing.T) { } // Make sure that we at rename all non UC resources -func TestAllNonUCResourcesAreRenamed(t *testing.T) { +func TestAllNonUcResourcesAreRenamed(t *testing.T) { b := mockBundle(config.Development) // UC resources should not have a prefix added to their name. Right now From 30905888a8187beb2dc913c46bff3f2f34d75f9f Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Fri, 29 Nov 2024 07:01:10 +0530 Subject: [PATCH 52/69] Update bundle/libraries/filer.go Co-authored-by: Pieter Noordhuis --- bundle/libraries/filer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/libraries/filer.go b/bundle/libraries/filer.go index 8cbf1a5101..01d58eae06 100644 --- a/bundle/libraries/filer.go +++ b/bundle/libraries/filer.go @@ -8,7 +8,7 @@ import ( "github.com/databricks/cli/libs/filer" ) -// This function returns the right filer to use, to upload artifacts to the configured location. +// This function returns a filer for uploading artifacts to the configured location. // Supported locations: // 1. WSFS // 2. UC volumes From 015f8cdc945acc6e3f81d44206be4df2696b721e Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Fri, 29 Nov 2024 07:01:33 +0530 Subject: [PATCH 53/69] Update bundle/libraries/filer_volume.go Co-authored-by: Pieter Noordhuis --- bundle/libraries/filer_volume.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index ec88bb9452..7128f4d770 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -28,7 +28,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, artifactPath := b.Config.Workspace.ArtifactPath w := b.WorkspaceClient() - if !strings.HasPrefix(artifactPath, "/Volumes/") { + if !IsVolumesPath(artifactPath) { return nil, "", diag.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) } From d0385a0205cd09eb4221f9aa56bf82df7839d955 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 02:45:33 +0100 Subject: [PATCH 54/69] add extractVolumeFromPath --- bundle/libraries/filer_volume.go | 52 ++++++++++++++++++--------- bundle/libraries/filer_volume_test.go | 30 +++++++++++++--- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 7128f4d770..e6b6c0fe5e 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -13,6 +13,31 @@ 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 +} + // This function returns a filer for ".internal" folder inside the directory configured // at `workspace.artifact_path`. // This function also checks if the UC volume exists in the workspace and then: @@ -32,26 +57,21 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, return nil, "", diag.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 nil, "", diag.FromErr(volumeFormatErr) - } - - catalogName := parts[2] - schemaName := parts[3] - volumeName := parts[4] - - // Incorrect format. - if catalogName == "" || schemaName == "" || volumeName == "" { - return nil, "", diag.FromErr(volumeFormatErr) + catalogName, schemaName, volumeName, err := extractVolumeFromPath(artifactPath) + if err != nil { + return nil, "", diag.Diagnostics{ + { + Severity: diag.Error, + Summary: err.Error(), + Locations: b.Config.GetLocations("workspace.artifact_path"), + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }, + } } // Check if the UC volume exists in the workspace. volumePath := fmt.Sprintf("/Volumes/%s/%s/%s", catalogName, schemaName, volumeName) - err := w.Files.GetDirectoryMetadataByDirectoryPath(ctx, volumePath) + err = w.Files.GetDirectoryMetadataByDirectoryPath(ctx, volumePath) // If the volume exists already, directly return the filer for the path to // upload the artifacts to. diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 987c57cb72..53f8665aad 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -153,8 +153,8 @@ func TestFilerForVolumeInBundle(t *testing.T) { }) } -func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { - invalidPaths := []string{ +func invalidVolumePaths() []string { + return []string{ "/Volumes/", "/Volumes/main", "/Volumes/main/", @@ -165,8 +165,10 @@ func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { "/Volumes/main/my_schema//", "/Volumes//my_schema/my_volume", } +} - for _, p := range invalidPaths { +func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { + for _, p := range invalidVolumePaths() { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -175,8 +177,15 @@ func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { }, } + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + _, _, diags := GetFilerForLibraries(context.Background(), b) - require.EqualError(t, diags.Error(), fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p)) + 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")}, + }}) } } @@ -221,3 +230,16 @@ func TestFilerForVolumeWithValidlVolumePaths(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)) + } +} From 5ac2d678fd14618aee5e089d05c2ed8c3ea71b88 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 02:46:31 +0100 Subject: [PATCH 55/69] - --- bundle/libraries/filer_volume_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 53f8665aad..03ffe54bda 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -202,7 +202,7 @@ func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { require.EqualError(t, diags.Error(), "expected artifact_path to start with /Volumes/, got /Volume/main/my_schema/my_volume") } -func TestFilerForVolumeWithValidlVolumePaths(t *testing.T) { +func TestFilerForVolumeWithValidVolumePaths(t *testing.T) { validPaths := []string{ "/Volumes/main/my_schema/my_volume", "/Volumes/main/my_schema/my_volume/", From 9493795d889a00c12b4989439d3ac07ab1334957 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 19:43:25 +0100 Subject: [PATCH 56/69] address comments --- bundle/libraries/filer_volume.go | 32 ++-- bundle/libraries/filer_volume_test.go | 48 +++--- internal/bundle/artifacts_test.go | 143 +++++++----------- .../databricks_template_schema.json | 16 ++ .../template/databricks.yml.tmpl | 14 ++ 5 files changed, 134 insertions(+), 119 deletions(-) create mode 100644 internal/bundle/bundles/artifact_path_with_volume/databricks_template_schema.json create mode 100644 internal/bundle/bundles/artifact_path_with_volume/template/databricks.yml.tmpl diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index e6b6c0fe5e..6af375022d 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -2,7 +2,9 @@ package libraries import ( "context" + "errors" "fmt" + "net/http" "path" "strings" @@ -11,6 +13,7 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/filer" + "github.com/databricks/databricks-sdk-go/apierr" ) func extractVolumeFromPath(artifactPath string) (string, string, string, error) { @@ -81,20 +84,27 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, return f, uploadPath, diag.FromErr(err) } - 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 := findVolumeInBundle(b, catalogName, schemaName, volumeName) - if !ok { - return nil, "", diags + baseErr := diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("failed to fetch metadata for %s: %s", volumePath, err), + Locations: b.Config.GetLocations("workspace.artifact_path"), + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, } - warning := 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: locations, - Paths: []dyn.Path{path}, + var aerr *apierr.APIError + if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { + path, locations, ok := findVolumeInBundle(b, catalogName, schemaName, volumeName) + if !ok { + return nil, "", diag.Diagnostics{baseErr} + } + baseErr.Detail = `You are 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.` + baseErr.Paths = append(baseErr.Paths, path) + baseErr.Locations = append(baseErr.Locations, locations...) } - return nil, "", diags.Append(warning) + + return nil, "", diag.Diagnostics{baseErr} } func findVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 03ffe54bda..3389211704 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -13,6 +13,7 @@ import ( "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" + "github.com/databricks/databricks-sdk-go/apierr" sdkconfig "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/catalog" @@ -95,14 +96,21 @@ func TestFilerForVolumeNotInBundle(t *testing.T) { }, } + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + m := mocks.NewMockWorkspaceClient(t) m.WorkspaceClient.Config = &sdkconfig.Config{} m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := filerForVolume(context.Background(), b) - 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) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "failed to fetch metadata for /Volumes/main/my_schema/doesnotexist: error from API", + Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }}, diags) } func TestFilerForVolumeInBundle(t *testing.T) { @@ -126,31 +134,29 @@ func TestFilerForVolumeInBundle(t *testing.T) { }, } - bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ - { - File: "volume.yml", - Line: 1, - Column: 2, - }, - }) + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{{File: "volume.yml", Line: 1, Column: 2}}) m := mocks.NewMockWorkspaceClient(t) m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(&apierr.APIError{ + StatusCode: 404, + Message: "error from API", + }) b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := GetFilerForLibraries(context.Background(), b) - 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: "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", - Line: 1, - Column: 2, - }}, - Paths: []dyn.Path{dyn.MustPathFromString("resources.volumes.foo")}, - }) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "failed to fetch metadata for /Volumes/main/my_schema/my_volume: error from API", + Locations: []dyn.Location{{"config.yml", 1, 2}, {"volume.yml", 1, 2}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, + Detail: `You are 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.`, + }, + }, diags) } func invalidVolumePaths() []string { diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 9496c1dc7b..ef7efdf04b 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -9,17 +9,15 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "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/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -232,7 +230,7 @@ func TestAccUploadArtifactFileToCorrectRemotePathForVolumes(t *testing.T) { ) } -func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { +func TestAccUploadArtifactFileToVolumeThatDoesNotExist(t *testing.T) { ctx, wt := acc.UcWorkspaceTest(t) w := wt.W @@ -250,93 +248,64 @@ func TestAccUploadArtifactFileToInvalidVolume(t *testing.T) { require.NoError(t, err) }) - t.Run("volume not in DAB", func(t *testing.T) { - volumePath := fmt.Sprintf("/Volumes/main/%s/doesnotexist", schemaName) - dir := t.TempDir() + bundleRoot, err := initTestTemplate(t, ctx, "artifact_path_with_volume", map[string]any{ + "unique_id": uuid.New().String(), + "schema_name": schemaName, + "volume_name": "doesnotexist", + }) + require.NoError(t, err) - b := &bundle.Bundle{ - BundleRootPath: dir, - SyncRootPath: dir, - Config: config.Root{ - Bundle: config.Bundle{ - Target: "whatever", - }, - Workspace: config.Workspace{ - ArtifactPath: volumePath, - }, - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - VolumeType: "MANAGED", - SchemaName: schemaName, - }, - }, - }, - }, - }, - } + t.Setenv("BUNDLE_ROOT", bundleRoot) + stdout, stderr, err := internal.RequireErrorRun(t, "bundle", "deploy") - diags := bundle.Apply(ctx, b, libraries.Upload()) - assert.ErrorContains(t, diags.Error(), fmt.Sprintf("failed to fetch metadata for the UC volume %s that is configured in the artifact_path:", volumePath)) + assert.Error(t, err) + assert.Equal(t, fmt.Sprintf(`Error: failed to fetch metadata for /Volumes/main/%s/doesnotexist: Not Found + at workspace.artifact_path + in databricks.yml:6:18 + +`, schemaName), stdout.String()) + assert.Equal(t, "", stderr.String()) +} + +func TestAccUploadArtifactToVolumeNotYetDeployed(t *testing.T) { + ctx, wt := acc.UcWorkspaceTest(t) + w := wt.W + + schemaName := internal.RandomName("schema-") + + _, err := w.Schemas.Create(ctx, catalog.CreateSchema{ + CatalogName: "main", + Comment: "test schema", + Name: schemaName, }) + require.NoError(t, err) - t.Run("volume in DAB config", func(t *testing.T) { - volumePath := fmt.Sprintf("/Volumes/main/%s/my_volume", schemaName) - dir := t.TempDir() + t.Cleanup(func() { + err = w.Schemas.DeleteByFullName(ctx, "main."+schemaName) + require.NoError(t, err) + }) - b := &bundle.Bundle{ - BundleRootPath: dir, - SyncRootPath: dir, - Config: config.Root{ - Bundle: config.Bundle{ - Target: "whatever", - }, - Workspace: config.Workspace{ - ArtifactPath: volumePath, - }, - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - VolumeType: "MANAGED", - SchemaName: schemaName, - }, - }, - }, - }, - }, - } - - // set location of volume definition in config. - bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{{ - File: filepath.Join(dir, "databricks.yml"), - Line: 1, - Column: 2, - }}) - - diags := bundle.Apply(ctx, b, libraries.Upload()) - 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"), - }, - }) + bundleRoot, err := initTestTemplate(t, ctx, "artifact_path_with_volume", map[string]any{ + "unique_id": uuid.New().String(), + "schema_name": schemaName, + "volume_name": "my_volume", }) + require.NoError(t, err) + + t.Setenv("BUNDLE_ROOT", bundleRoot) + stdout, stderr, err := internal.RequireErrorRun(t, "bundle", "deploy") + + assert.Error(t, err) + assert.Equal(t, fmt.Sprintf(`Error: failed to fetch metadata for /Volumes/main/%s/my_volume: Not Found + at workspace.artifact_path + resources.volumes.foo + in databricks.yml:6:18 + databricks.yml:11:7 + +You are 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. + +`, schemaName), stdout.String()) + assert.Equal(t, "", stderr.String()) } diff --git a/internal/bundle/bundles/artifact_path_with_volume/databricks_template_schema.json b/internal/bundle/bundles/artifact_path_with_volume/databricks_template_schema.json new file mode 100644 index 0000000000..7dd81ef2a1 --- /dev/null +++ b/internal/bundle/bundles/artifact_path_with_volume/databricks_template_schema.json @@ -0,0 +1,16 @@ +{ + "properties": { + "unique_id": { + "type": "string", + "description": "Unique ID for job name" + }, + "schema_name": { + "type": "string", + "description": "schema name to use in the artifact_path" + }, + "volume_name": { + "type": "string", + "description": "volume name to use in the artifact_path" + } + } +} diff --git a/internal/bundle/bundles/artifact_path_with_volume/template/databricks.yml.tmpl b/internal/bundle/bundles/artifact_path_with_volume/template/databricks.yml.tmpl new file mode 100644 index 0000000000..2d87620e3a --- /dev/null +++ b/internal/bundle/bundles/artifact_path_with_volume/template/databricks.yml.tmpl @@ -0,0 +1,14 @@ +bundle: + name: artifact_path_with_volume + +workspace: + root_path: "~/.bundle/{{.unique_id}}" + artifact_path: /Volumes/main/{{.schema_name}}/{{.volume_name}} + +resources: + volumes: + foo: + catalog_name: main + name: my_volume + schema_name: {{.schema_name}} + volume_type: MANAGED From e51b3a17bdef5c5274bc1f5e89fa0cade22cf89b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 19:49:07 +0100 Subject: [PATCH 57/69] add const for internal --- bundle/libraries/filer_volume.go | 2 +- bundle/libraries/filer_workspace.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 6af375022d..e27a88e66b 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -79,7 +79,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, // If the volume exists already, directly return the filer for the path to // upload the artifacts to. if err == nil { - uploadPath := path.Join(artifactPath, ".internal") + uploadPath := path.Join(artifactPath, InternalDirName) f, err := filer.NewFilesClient(w, uploadPath) return f, uploadPath, diag.FromErr(err) } diff --git a/bundle/libraries/filer_workspace.go b/bundle/libraries/filer_workspace.go index 9172fef92b..32d48f38a7 100644 --- a/bundle/libraries/filer_workspace.go +++ b/bundle/libraries/filer_workspace.go @@ -8,8 +8,12 @@ import ( "github.com/databricks/cli/libs/filer" ) +// We upload artifacts to the workspace in a directory named ".internal" to have +// a well defined location for artifacts that have been uploaded by the DABs. +const InternalDirName = ".internal" + func filerForWorkspace(b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { - uploadPath := path.Join(b.Config.Workspace.ArtifactPath, ".internal") + uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName) f, err := filer.NewWorkspaceFilesClient(b.WorkspaceClient(), uploadPath) return f, uploadPath, diag.FromErr(err) } From 01c38303e1e8c4a3bd178a9afb0d349f27f67216 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 19:54:25 +0100 Subject: [PATCH 58/69] add TestFilerForWorkspace --- bundle/libraries/filer_workspace_test.go | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 bundle/libraries/filer_workspace_test.go diff --git a/bundle/libraries/filer_workspace_test.go b/bundle/libraries/filer_workspace_test.go new file mode 100644 index 0000000000..f761592f9b --- /dev/null +++ b/bundle/libraries/filer_workspace_test.go @@ -0,0 +1,27 @@ +package libraries + +import ( + "path" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/filer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFilerForWorkspace(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Workspace/Users/shreyas.goenka@databricks.com/a/b/c", + }, + }, + } + + client, uploadPath, diags := filerForWorkspace(b) + require.NoError(t, diags.Error()) + assert.Equal(t, path.Join("/Workspace/Users/shreyas.goenka@databricks.com/a/b/c/.internal"), uploadPath) + assert.IsType(t, &filer.WorkspaceFilesClient{}, client) +} From e5f561877445dbe2286b061a81b0a1a2ebb68a4f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 19:55:12 +0100 Subject: [PATCH 59/69] - --- bundle/config/mutator/apply_presets_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 19050aee2d..8cc0a1e2c0 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -6,10 +6,10 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" From 23e87d5d24b4c535faa9370e7194a4d9d93158de Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 20:00:41 +0100 Subject: [PATCH 60/69] - --- bundle/libraries/filer_volume.go | 5 +++-- bundle/libraries/filer_volume_test.go | 5 +++-- internal/bundle/artifacts_test.go | 5 +++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index e27a88e66b..7209ffc0d7 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -98,8 +98,9 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, return nil, "", diag.Diagnostics{baseErr} } baseErr.Detail = `You are 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.` +this bundle but which has not been deployed yet. Please first deploy +the UC volume using 'bundle deploy' and then switch over to using it in +the artifact_path.` baseErr.Paths = append(baseErr.Paths, path) baseErr.Locations = append(baseErr.Locations, locations...) } diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 3389211704..ed54d3b679 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -153,8 +153,9 @@ func TestFilerForVolumeInBundle(t *testing.T) { Locations: []dyn.Location{{"config.yml", 1, 2}, {"volume.yml", 1, 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, Detail: `You are 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.`, +this bundle but which has not been deployed yet. Please first deploy +the UC volume using 'bundle deploy' and then switch over to using it in +the artifact_path.`, }, }, diags) } diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index ef7efdf04b..222e837d13 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -303,8 +303,9 @@ func TestAccUploadArtifactToVolumeNotYetDeployed(t *testing.T) { databricks.yml:11:7 You are 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. +this bundle but which has not been deployed yet. Please first deploy +the UC volume using 'bundle deploy' and then switch over to using it in +the artifact_path. `, schemaName), stdout.String()) assert.Equal(t, "", stderr.String()) From d1ec088d706b4ca8d4560fc5cbf9b8136caab6a9 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 29 Nov 2024 20:03:12 +0100 Subject: [PATCH 61/69] remove defensive bit --- bundle/libraries/filer_volume.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 7209ffc0d7..b0b36179e0 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -56,10 +56,6 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, artifactPath := b.Config.Workspace.ArtifactPath w := b.WorkspaceClient() - if !IsVolumesPath(artifactPath) { - return nil, "", diag.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) - } - catalogName, schemaName, volumeName, err := extractVolumeFromPath(artifactPath) if err != nil { return nil, "", diag.Diagnostics{ From 42bf6aeecf1e7fe1a2a7903818032041dabb6cfe Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 11:42:00 +0100 Subject: [PATCH 62/69] revert bundletest move --- bundle/artifacts/expand_globs_test.go | 2 +- bundle/config/mutator/apply_presets_test.go | 2 +- bundle/config/mutator/configure_dashboard_defaults_test.go | 2 +- bundle/config/mutator/expand_pipeline_glob_paths_test.go | 2 +- bundle/config/mutator/rewrite_sync_paths_test.go | 2 +- bundle/config/mutator/sync_infer_root_test.go | 2 +- bundle/config/mutator/translate_paths_dashboards_test.go | 2 +- bundle/config/mutator/translate_paths_test.go | 2 +- bundle/config/validate/single_node_cluster_test.go | 2 +- bundle/deploy/metadata/compute_test.go | 2 +- bundle/{ => internal}/bundletest/location.go | 0 bundle/{ => internal}/bundletest/mutate.go | 0 bundle/libraries/expand_glob_references_test.go | 2 +- bundle/libraries/filer_volume_test.go | 2 +- 14 files changed, 12 insertions(+), 12 deletions(-) rename bundle/{ => internal}/bundletest/location.go (100%) rename bundle/{ => internal}/bundletest/mutate.go (100%) diff --git a/bundle/artifacts/expand_globs_test.go b/bundle/artifacts/expand_globs_test.go index af048f8e52..dc7c77de70 100644 --- a/bundle/artifacts/expand_globs_test.go +++ b/bundle/artifacts/expand_globs_test.go @@ -7,8 +7,8 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 8cc0a1e2c0..91d5b62e5f 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -6,10 +6,10 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" diff --git a/bundle/config/mutator/configure_dashboard_defaults_test.go b/bundle/config/mutator/configure_dashboard_defaults_test.go index a5248219fa..2234f9a732 100644 --- a/bundle/config/mutator/configure_dashboard_defaults_test.go +++ b/bundle/config/mutator/configure_dashboard_defaults_test.go @@ -5,10 +5,10 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/dashboards" "github.com/stretchr/testify/assert" diff --git a/bundle/config/mutator/expand_pipeline_glob_paths_test.go b/bundle/config/mutator/expand_pipeline_glob_paths_test.go index 063c76b35c..9f70b74ae4 100644 --- a/bundle/config/mutator/expand_pipeline_glob_paths_test.go +++ b/bundle/config/mutator/expand_pipeline_glob_paths_test.go @@ -7,9 +7,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/pipelines" diff --git a/bundle/config/mutator/rewrite_sync_paths_test.go b/bundle/config/mutator/rewrite_sync_paths_test.go index 90da879207..0e4dfc4cec 100644 --- a/bundle/config/mutator/rewrite_sync_paths_test.go +++ b/bundle/config/mutator/rewrite_sync_paths_test.go @@ -6,9 +6,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" ) diff --git a/bundle/config/mutator/sync_infer_root_test.go b/bundle/config/mutator/sync_infer_root_test.go index 98c9d36258..f507cbc7f8 100644 --- a/bundle/config/mutator/sync_infer_root_test.go +++ b/bundle/config/mutator/sync_infer_root_test.go @@ -6,9 +6,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/bundle/config/mutator/translate_paths_dashboards_test.go b/bundle/config/mutator/translate_paths_dashboards_test.go index ae74f31438..5e4e69f5d8 100644 --- a/bundle/config/mutator/translate_paths_dashboards_test.go +++ b/bundle/config/mutator/translate_paths_dashboards_test.go @@ -6,10 +6,10 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go/service/dashboards" diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index c8f4055964..bf6ba15d8b 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -10,11 +10,11 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/vfs" diff --git a/bundle/config/validate/single_node_cluster_test.go b/bundle/config/validate/single_node_cluster_test.go index 6c650f6e97..18771cc00d 100644 --- a/bundle/config/validate/single_node_cluster_test.go +++ b/bundle/config/validate/single_node_cluster_test.go @@ -5,9 +5,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" diff --git a/bundle/deploy/metadata/compute_test.go b/bundle/deploy/metadata/compute_test.go index 2d852bf799..2c2c723769 100644 --- a/bundle/deploy/metadata/compute_test.go +++ b/bundle/deploy/metadata/compute_test.go @@ -5,9 +5,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/bundle/metadata" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/jobs" diff --git a/bundle/bundletest/location.go b/bundle/internal/bundletest/location.go similarity index 100% rename from bundle/bundletest/location.go rename to bundle/internal/bundletest/location.go diff --git a/bundle/bundletest/mutate.go b/bundle/internal/bundletest/mutate.go similarity index 100% rename from bundle/bundletest/mutate.go rename to bundle/internal/bundletest/mutate.go diff --git a/bundle/libraries/expand_glob_references_test.go b/bundle/libraries/expand_glob_references_test.go index bdd04216a4..2dfbddb743 100644 --- a/bundle/libraries/expand_glob_references_test.go +++ b/bundle/libraries/expand_glob_references_test.go @@ -6,9 +6,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/compute" diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index ed54d3b679..d7a8743099 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -7,9 +7,9 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/bundletest" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" From 5f2db1e2bdde7087a56dad1b807f6714299c7801 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 11:44:00 +0100 Subject: [PATCH 63/69] move .internal var --- bundle/libraries/filer.go | 4 ++++ bundle/libraries/filer_workspace.go | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle/libraries/filer.go b/bundle/libraries/filer.go index 01d58eae06..4448ed325b 100644 --- a/bundle/libraries/filer.go +++ b/bundle/libraries/filer.go @@ -8,6 +8,10 @@ import ( "github.com/databricks/cli/libs/filer" ) +// We upload artifacts to the workspace in a directory named ".internal" to have +// a well defined location for artifacts that have been uploaded by the DABs. +const InternalDirName = ".internal" + // This function returns a filer for uploading artifacts to the configured location. // Supported locations: // 1. WSFS diff --git a/bundle/libraries/filer_workspace.go b/bundle/libraries/filer_workspace.go index 32d48f38a7..8d54519ff0 100644 --- a/bundle/libraries/filer_workspace.go +++ b/bundle/libraries/filer_workspace.go @@ -8,10 +8,6 @@ import ( "github.com/databricks/cli/libs/filer" ) -// We upload artifacts to the workspace in a directory named ".internal" to have -// a well defined location for artifacts that have been uploaded by the DABs. -const InternalDirName = ".internal" - func filerForWorkspace(b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName) f, err := filer.NewWorkspaceFilesClient(b.WorkspaceClient(), uploadPath) From 07f888c43649b285176ec2792e823c866a865ac9 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 11:45:32 +0100 Subject: [PATCH 64/69] remove UC --- bundle/libraries/filer_volume.go | 2 +- bundle/libraries/filer_volume_test.go | 2 +- internal/bundle/artifacts_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index b0b36179e0..f3a6fdbe77 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -93,7 +93,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, if !ok { return nil, "", diag.Diagnostics{baseErr} } - baseErr.Detail = `You are using a UC volume in your artifact_path that is managed by + baseErr.Detail = `You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy the UC volume using 'bundle deploy' and then switch over to using it in the artifact_path.` diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index d7a8743099..bbd8f20f32 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -152,7 +152,7 @@ func TestFilerForVolumeInBundle(t *testing.T) { Summary: "failed to fetch metadata for /Volumes/main/my_schema/my_volume: error from API", Locations: []dyn.Location{{"config.yml", 1, 2}, {"volume.yml", 1, 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, - Detail: `You are using a UC volume in your artifact_path that is managed by + Detail: `You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy the UC volume using 'bundle deploy' and then switch over to using it in the artifact_path.`, diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 222e837d13..2d3c151fb7 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -302,7 +302,7 @@ func TestAccUploadArtifactToVolumeNotYetDeployed(t *testing.T) { in databricks.yml:6:18 databricks.yml:11:7 -You are using a UC volume in your artifact_path that is managed by +You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy the UC volume using 'bundle deploy' and then switch over to using it in the artifact_path. From 7d544f4af4c11b8f9f4ced487477d17b694b3b57 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 11:46:55 +0100 Subject: [PATCH 65/69] lowercase volumes --- bundle/phases/deploy.go | 2 +- internal/bundle/deploy_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index f5fe42b034..2dc9623bd0 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -96,7 +96,7 @@ properties such as the 'catalog' or 'storage' are changed:` // One or more volumes is being recreated. if len(volumeActions) != 0 { msg := ` -This action will result in the deletion or recreation of the following Volumes. +This action will result in the deletion or recreation of the following volumes. For managed volumes, the files stored in the volume are also deleted from your cloud tenant within 30 days. For external volumes, the metadata about the volume is removed from the catalog, but the underlying files are not deleted:` diff --git a/internal/bundle/deploy_test.go b/internal/bundle/deploy_test.go index 0ddc79f3b7..759e85de5d 100644 --- a/internal/bundle/deploy_test.go +++ b/internal/bundle/deploy_test.go @@ -284,7 +284,7 @@ func TestAccDeployUcVolume(t *testing.T) { t.Setenv("BUNDLE_ROOT", bundleRoot) stdout, stderr, err := internal.NewCobraTestRunnerWithContext(t, ctx, "bundle", "deploy", "--var=schema_name=${resources.schemas.schema2.name}").Run() assert.Error(t, err) - assert.Contains(t, stderr.String(), `This action will result in the deletion or recreation of the following Volumes. + assert.Contains(t, stderr.String(), `This action will result in the deletion or recreation of the following volumes. For managed volumes, the files stored in the volume are also deleted from your cloud tenant within 30 days. For external volumes, the metadata about the volume is removed from the catalog, but the underlying files are not deleted: From 406c0736ffa560739c2ef30982f8d247a4d47a01 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 11:48:04 +0100 Subject: [PATCH 66/69] use apierr.ErrNotFound --- bundle/libraries/filer_volume.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index f3a6fdbe77..1ef8d401fa 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "net/http" "path" "strings" @@ -87,8 +86,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, } - var aerr *apierr.APIError - if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { + if errors.Is(err, apierr.ErrNotFound) { path, locations, ok := findVolumeInBundle(b, catalogName, schemaName, volumeName) if !ok { return nil, "", diag.Diagnostics{baseErr} From 3461018e8a5a921463f9e80a80111642ee90f5fb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 14:01:10 +0100 Subject: [PATCH 67/69] better error message --- bundle/libraries/filer_volume.go | 8 +++++- bundle/libraries/filer_volume_test.go | 41 +++++++++++++++++++++------ internal/bundle/artifacts_test.go | 4 +-- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 1ef8d401fa..962a6f6223 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -81,12 +81,18 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, baseErr := diag.Diagnostic{ Severity: diag.Error, - Summary: fmt.Sprintf("failed to fetch metadata for %s: %s", volumePath, err), + Summary: fmt.Sprintf("unable to determine if volume at %s exists: %s", volumePath, err), Locations: b.Config.GetLocations("workspace.artifact_path"), Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, } if errors.Is(err, apierr.ErrNotFound) { + // Since the API returned a 404, the volume does not exist in the workspace. + // Modify the error message to provide more context. + baseErr.Summary = fmt.Sprintf("volume %s does not exist: %s", volumePath, err) + + // If the volume is defined in the bundle, provide a more helpful error diagnostic, + // with more details and location information. path, locations, ok := findVolumeInBundle(b, catalogName, schemaName, volumeName) if !ok { return nil, "", diag.Diagnostics{baseErr} diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index bbd8f20f32..d084d936a1 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -87,7 +87,33 @@ func TestFindVolumeInBundle(t *testing.T) { assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) } -func TestFilerForVolumeNotInBundle(t *testing.T) { +func TestFilerForVolumeForErrorFromAPI(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + }, + } + + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) + + _, _, diags := filerForVolume(context.Background(), b) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "unable to determine if volume at /Volumes/main/my_schema/my_volume exists: error from API", + Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }}, diags) +} + +func TestFilerForVolumeWithVolumeNotFound(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -100,20 +126,20 @@ func TestFilerForVolumeNotInBundle(t *testing.T) { m := mocks.NewMockWorkspaceClient(t) m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(apierr.NotFound("some error message")) b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := filerForVolume(context.Background(), b) assert.Equal(t, diag.Diagnostics{ { Severity: diag.Error, - Summary: "failed to fetch metadata for /Volumes/main/my_schema/doesnotexist: error from API", + Summary: "volume /Volumes/main/my_schema/doesnotexist does not exist: some error message", Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, }}, diags) } -func TestFilerForVolumeInBundle(t *testing.T) { +func TestFilerForVolumeNotFoundAndInBundle(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -139,17 +165,14 @@ func TestFilerForVolumeInBundle(t *testing.T) { m := mocks.NewMockWorkspaceClient(t) m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(&apierr.APIError{ - StatusCode: 404, - Message: "error from API", - }) + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(apierr.NotFound("error from API")) b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := GetFilerForLibraries(context.Background(), b) assert.Equal(t, diag.Diagnostics{ { Severity: diag.Error, - Summary: "failed to fetch metadata for /Volumes/main/my_schema/my_volume: error from API", + Summary: "volume /Volumes/main/my_schema/my_volume does not exist: error from API", Locations: []dyn.Location{{"config.yml", 1, 2}, {"volume.yml", 1, 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, Detail: `You are using a volume in your artifact_path that is managed by diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 2d3c151fb7..6a3992fb58 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -259,7 +259,7 @@ func TestAccUploadArtifactFileToVolumeThatDoesNotExist(t *testing.T) { stdout, stderr, err := internal.RequireErrorRun(t, "bundle", "deploy") assert.Error(t, err) - assert.Equal(t, fmt.Sprintf(`Error: failed to fetch metadata for /Volumes/main/%s/doesnotexist: Not Found + assert.Equal(t, fmt.Sprintf(`Error: volume /Volumes/main/%s/doesnotexist does not exist: Not Found at workspace.artifact_path in databricks.yml:6:18 @@ -296,7 +296,7 @@ func TestAccUploadArtifactToVolumeNotYetDeployed(t *testing.T) { stdout, stderr, err := internal.RequireErrorRun(t, "bundle", "deploy") assert.Error(t, err) - assert.Equal(t, fmt.Sprintf(`Error: failed to fetch metadata for /Volumes/main/%s/my_volume: Not Found + assert.Equal(t, fmt.Sprintf(`Error: volume /Volumes/main/%s/my_volume does not exist: Not Found at workspace.artifact_path resources.volumes.foo in databricks.yml:6:18 From 8d790efcf9e04140d7492a971bbffc376238072c Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 16:27:23 +0100 Subject: [PATCH 68/69] address comments --- bundle/libraries/filer_volume.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 962a6f6223..aecf68db13 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -87,7 +87,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, } if errors.Is(err, apierr.ErrNotFound) { - // Since the API returned a 404, the volume does not exist in the workspace. + // Since the API returned a 404, the volume does not exist. // Modify the error message to provide more context. baseErr.Summary = fmt.Sprintf("volume %s does not exist: %s", volumePath, err) @@ -99,7 +99,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, } baseErr.Detail = `You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy -the UC volume using 'bundle deploy' and then switch over to using it in +the volume using 'bundle deploy' and then switch over to using it in the artifact_path.` baseErr.Paths = append(baseErr.Paths, path) baseErr.Locations = append(baseErr.Locations, locations...) From d460bd68591f016748d1b06f635e648b82e077e0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 16:29:50 +0100 Subject: [PATCH 69/69] fix --- bundle/libraries/filer_volume_test.go | 2 +- internal/bundle/artifacts_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index d084d936a1..0d886824d9 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -177,7 +177,7 @@ func TestFilerForVolumeNotFoundAndInBundle(t *testing.T) { Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, Detail: `You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy -the UC volume using 'bundle deploy' and then switch over to using it in +the volume using 'bundle deploy' and then switch over to using it in the artifact_path.`, }, }, diags) diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 6a3992fb58..34d101e4f8 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -304,7 +304,7 @@ func TestAccUploadArtifactToVolumeNotYetDeployed(t *testing.T) { You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy -the UC volume using 'bundle deploy' and then switch over to using it in +the volume using 'bundle deploy' and then switch over to using it in the artifact_path. `, schemaName), stdout.String())