From 4b4cc42e5d97ba0178c618d976f03d2fd55706af Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 30 Dec 2024 12:44:09 +0100 Subject: [PATCH] fixs --- bundle/apps/interpolate_variables.go | 17 +--- bundle/apps/upload_config.go | 2 +- bundle/apps/upload_config_test.go | 8 +- bundle/apps/validate.go | 12 +-- bundle/apps/validate_test.go | 40 ++------ bundle/config/mutator/run_as_test.go | 132 ++++++++++++++++++--------- bundle/config/resources.go | 110 ++++++++++++---------- bundle/deploy/terraform/util.go | 8 +- integration/bundle/apps_test.go | 3 + 9 files changed, 182 insertions(+), 150 deletions(-) diff --git a/bundle/apps/interpolate_variables.go b/bundle/apps/interpolate_variables.go index f4860127b..f88e7e9db 100644 --- a/bundle/apps/interpolate_variables.go +++ b/bundle/apps/interpolate_variables.go @@ -4,6 +4,7 @@ import ( "context" "github.com/databricks/cli/bundle" + "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" @@ -19,19 +20,9 @@ func (i *interpolateVariables) Apply(ctx context.Context, b *bundle.Bundle) diag dyn.Key("config"), ) - tfToConfigMap := map[string]string{ - "databricks_pipeline": "pipelines", - "databricks_job": "jobs", - "databricks_mlflow_model": "models", - "databricks_mlflow_experiment": "experiments", - "databricks_model_serving": "model_serving_endpoints", - "databricks_registered_model": "registered_models", - "databricks_quality_monitor": "quality_monitors", - "databricks_schema": "schemas", - "databricks_volume": "volumes", - "databricks_cluster": "clusters", - "databricks_dashboard": "dashboards", - "databricks_app": "apps", + tfToConfigMap := map[string]string{} + for k, r := range config.SupportedResources() { + tfToConfigMap[r.TerraformResourceName] = k } err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { diff --git a/bundle/apps/upload_config.go b/bundle/apps/upload_config.go index 14893a169..0e3db269e 100644 --- a/bundle/apps/upload_config.go +++ b/bundle/apps/upload_config.go @@ -45,7 +45,7 @@ func (u *uploadConfig) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } errGroup.Go(func() error { - err = f.Write(ctx, path.Join(appPath, "app.yml"), buf, filer.OverwriteIfExists) + err := f.Write(ctx, path.Join(appPath, "app.yml"), buf, filer.OverwriteIfExists) if err != nil { mu.Lock() diags = append(diags, diag.Diagnostic{ diff --git a/bundle/apps/upload_config_test.go b/bundle/apps/upload_config_test.go index 2d9b1c70d..a1a6b3afb 100644 --- a/bundle/apps/upload_config_test.go +++ b/bundle/apps/upload_config_test.go @@ -9,8 +9,11 @@ import ( "github.com/databricks/cli/bundle" "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" mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go/service/apps" @@ -25,6 +28,7 @@ func TestAppUploadConfig(t *testing.T) { b := &bundle.Bundle{ BundleRootPath: root, + SyncRootPath: root, SyncRoot: vfs.MustNew(root), Config: config.Root{ Workspace: config.Workspace{ @@ -64,6 +68,8 @@ env: }, } - diags := bundle.Apply(context.Background(), b, &u) + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(root, "databricks.yml")}}) + + diags := bundle.Apply(context.Background(), b, bundle.Seq(mutator.TranslatePaths(), &u)) require.NoError(t, diags.Error()) } diff --git a/bundle/apps/validate.go b/bundle/apps/validate.go index da776aa05..d3221a948 100644 --- a/bundle/apps/validate.go +++ b/bundle/apps/validate.go @@ -29,7 +29,8 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics usedSourceCodePaths[app.SourceCodePath] = key for _, configFile := range possibleConfigFiles { - cf := path.Join(app.SourceCodePath, configFile) + appPath := strings.TrimPrefix(app.SourceCodePath, b.Config.Workspace.FilePath) + cf := path.Join(appPath, configFile) if _, err := b.SyncRoot.Stat(cf); err == nil { diags = append(diags, diag.Diagnostic{ Severity: diag.Error, @@ -38,15 +39,6 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics }) } } - - if !strings.HasPrefix(app.SourceCodePath, b.Config.Workspace.FilePath) { - diags = append(diags, diag.Diagnostic{ - Severity: diag.Error, - Summary: "App source code invalid", - Detail: fmt.Sprintf("App source code path %s is not within file path %s", app.SourceCodePath, b.Config.Workspace.FilePath), - Locations: b.Config.GetLocations(fmt.Sprintf("resources.apps.%s.source_code_path", key)), - }) - } } return diags diff --git a/bundle/apps/validate_test.go b/bundle/apps/validate_test.go index 3d860f3c4..6c3a88191 100644 --- a/bundle/apps/validate_test.go +++ b/bundle/apps/validate_test.go @@ -11,7 +11,6 @@ import ( "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/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go/service/apps" @@ -28,6 +27,9 @@ func TestAppsValidate(t *testing.T) { SyncRootPath: tmpDir, SyncRoot: vfs.MustNew(tmpDir), Config: config.Root{ + Workspace: config.Workspace{ + FilePath: "/foo/bar/", + }, Resources: config.Resources{ Apps: map[string]*resources.App{ "app1": { @@ -64,6 +66,9 @@ func TestAppsValidateSameSourcePath(t *testing.T) { SyncRootPath: tmpDir, SyncRoot: vfs.MustNew(tmpDir), Config: config.Root{ + Workspace: config.Workspace{ + FilePath: "/foo/bar/", + }, Resources: config.Resources{ Apps: map[string]*resources.App{ "app1": { @@ -90,36 +95,3 @@ func TestAppsValidateSameSourcePath(t *testing.T) { require.Equal(t, "Duplicate app source code path", diags[0].Summary) require.Contains(t, diags[0].Detail, "has the same source code path as app resource") } - -func TestAppsValidateIncorrectSourceCodePath(t *testing.T) { - tmpDir := t.TempDir() - - b := &bundle.Bundle{ - BundleRootPath: tmpDir, - SyncRootPath: tmpDir, - SyncRoot: vfs.MustNew(tmpDir), - Config: config.Root{ - Workspace: config.Workspace{ - FilePath: "/Workspace/Users/foo@bar.com/files", - }, - Resources: config.Resources{ - Apps: map[string]*resources.App{ - "app1": { - App: &apps.App{ - Name: "app1", - }, - SourceCodePath: "/Workspace/Random/app1", - }, - }, - }, - }, - } - - bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}}) - - diags := bundle.Apply(context.Background(), b, bundle.Seq(Validate())) - require.Len(t, diags, 1) - require.Equal(t, diag.Error, diags[0].Severity) - require.Equal(t, "App source code invalid", diags[0].Summary) - require.Contains(t, diags[0].Detail, "App source code path /Workspace/Random/app1 is not within file path /Workspace/Users/foo@bar.com/files") -} diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index afd10e748..650b65d61 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -105,48 +105,47 @@ func TestRunAsWorksForAllowedResources(t *testing.T) { } } -func TestRunAsErrorForUnsupportedResources(t *testing.T) { - // Bundle "run_as" has two modes of operation, each with a different set of - // resources that are supported. - // Cases: - // 1. When the bundle "run_as" identity is same as the current deployment - // identity. In this case all resources are supported. - // 2. When the bundle "run_as" identity is different from the current - // deployment identity. In this case only a subset of resources are - // supported. This subset of resources are defined in the allow list below. - // - // To be a part of the allow list, the resource must satisfy one of the following - // two conditions: - // 1. The resource supports setting a run_as identity to a different user - // from the owner/creator of the resource. For example, jobs. - // 2. Run as semantics do not apply to the resource. We do not plan to add - // platform side support for `run_as` for these resources. For example, - // experiments or registered models. - // - // Any resource that is not on the allow list cannot be used when the bundle - // run_as is different from the current deployment user. "bundle validate" must - // return an error if such a resource has been defined, and the run_as identity - // is different from the current deployment identity. - // - // Action Item: If you are adding a new resource to DABs, please check in with - // the relevant owning team whether the resource should be on the allow list or (implicitly) on - // the deny list. Any resources that could have run_as semantics in the future - // should be on the deny list. - // For example: Teams for pipelines, model serving endpoints or Lakeview dashboards - // are planning to add platform side support for `run_as` for these resources at - // some point in the future. These resources are (implicitly) on the deny list, since - // they are not on the allow list below. - allowList := []string{ - "clusters", - "jobs", - "models", - "registered_models", - "experiments", - "schemas", - "volumes", - "apps", - } +// Bundle "run_as" has two modes of operation, each with a different set of +// resources that are supported. +// Cases: +// 1. When the bundle "run_as" identity is same as the current deployment +// identity. In this case all resources are supported. +// 2. When the bundle "run_as" identity is different from the current +// deployment identity. In this case only a subset of resources are +// supported. This subset of resources are defined in the allow list below. +// +// To be a part of the allow list, the resource must satisfy one of the following +// two conditions: +// 1. The resource supports setting a run_as identity to a different user +// from the owner/creator of the resource. For example, jobs. +// 2. Run as semantics do not apply to the resource. We do not plan to add +// platform side support for `run_as` for these resources. For example, +// experiments or registered models. +// +// Any resource that is not on the allow list cannot be used when the bundle +// run_as is different from the current deployment user. "bundle validate" must +// return an error if such a resource has been defined, and the run_as identity +// is different from the current deployment identity. +// +// Action Item: If you are adding a new resource to DABs, please check in with +// the relevant owning team whether the resource should be on the allow list or (implicitly) on +// the deny list. Any resources that could have run_as semantics in the future +// should be on the deny list. +// For example: Teams for pipelines, model serving endpoints or Lakeview dashboards +// are planning to add platform side support for `run_as` for these resources at +// some point in the future. These resources are (implicitly) on the deny list, since +// they are not on the allow list below. +var allowList = []string{ + "clusters", + "jobs", + "models", + "registered_models", + "experiments", + "schemas", + "volumes", +} +func TestRunAsErrorForUnsupportedResources(t *testing.T) { base := config.Root{ Workspace: config.Workspace{ CurrentUser: &config.User{ @@ -199,3 +198,54 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { "See https://docs.databricks.com/dev-tools/bundles/run-as.html to learn more about the run_as property.", rt) } } + +func TestRunAsNoErrorForSupportedResources(t *testing.T) { + base := config.Root{ + Workspace: config.Workspace{ + CurrentUser: &config.User{ + User: &iam.User{ + UserName: "alice", + }, + }, + }, + RunAs: &jobs.JobRunAs{ + UserName: "bob", + }, + } + + v, err := convert.FromTyped(base, dyn.NilValue) + require.NoError(t, err) + + // Define top level resources key in the bundle configuration. + // This is not part of the typed configuration, so we need to add it manually. + v, err = dyn.Set(v, "resources", dyn.V(map[string]dyn.Value{})) + require.NoError(t, err) + + for _, rt := range allResourceTypes(t) { + // Skip unsupported resources + if !slices.Contains(allowList, rt) { + continue + } + + // Add an instance of the resource type that is not on the allow list to + // the bundle configuration. + nv, err := dyn.SetByPath(v, dyn.NewPath(dyn.Key("resources"), dyn.Key(rt)), dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "name": dyn.V("bar"), + }), + })) + require.NoError(t, err) + + // Get back typed configuration from the newly created invalid bundle configuration. + r := &config.Root{} + err = convert.ToTyped(r, nv) + require.NoError(t, err) + + // Assert this configuration passes validation. + b := &bundle.Bundle{ + Config: *r, + } + diags := bundle.Apply(context.Background(), b, SetRunAs()) + require.NoError(t, diags.Error()) + } +} diff --git a/bundle/config/resources.go b/bundle/config/resources.go index b06a44b4d..1f523fed3 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -135,82 +135,96 @@ type ResourceDescription struct { // Singular and plural title when used in summaries / terminal UI. SingularTitle string PluralTitle string + + TerraformResourceName string } // The keys of the map corresponds to the resource key in the bundle configuration. func SupportedResources() map[string]ResourceDescription { return map[string]ResourceDescription{ "jobs": { - SingularName: "job", - PluralName: "jobs", - SingularTitle: "Job", - PluralTitle: "Jobs", + SingularName: "job", + PluralName: "jobs", + SingularTitle: "Job", + PluralTitle: "Jobs", + TerraformResourceName: "databricks_job", }, "pipelines": { - SingularName: "pipeline", - PluralName: "pipelines", - SingularTitle: "Pipeline", - PluralTitle: "Pipelines", + SingularName: "pipeline", + PluralName: "pipelines", + SingularTitle: "Pipeline", + PluralTitle: "Pipelines", + TerraformResourceName: "databricks_pipeline", }, "models": { - SingularName: "model", - PluralName: "models", - SingularTitle: "Model", - PluralTitle: "Models", + SingularName: "model", + PluralName: "models", + SingularTitle: "Model", + PluralTitle: "Models", + TerraformResourceName: "databricks_mlflow_model", }, "experiments": { - SingularName: "experiment", - PluralName: "experiments", - SingularTitle: "Experiment", - PluralTitle: "Experiments", + SingularName: "experiment", + PluralName: "experiments", + SingularTitle: "Experiment", + PluralTitle: "Experiments", + TerraformResourceName: "databricks_mlflow_experiment", }, "model_serving_endpoints": { - SingularName: "model_serving_endpoint", - PluralName: "model_serving_endpoints", - SingularTitle: "Model Serving Endpoint", - PluralTitle: "Model Serving Endpoints", + SingularName: "model_serving_endpoint", + PluralName: "model_serving_endpoints", + SingularTitle: "Model Serving Endpoint", + PluralTitle: "Model Serving Endpoints", + TerraformResourceName: "databricks_model_serving_endpoint", }, "registered_models": { - SingularName: "registered_model", - PluralName: "registered_models", - SingularTitle: "Registered Model", - PluralTitle: "Registered Models", + SingularName: "registered_model", + PluralName: "registered_models", + SingularTitle: "Registered Model", + PluralTitle: "Registered Models", + TerraformResourceName: "databricks_registered_model", }, "quality_monitors": { - SingularName: "quality_monitor", - PluralName: "quality_monitors", - SingularTitle: "Quality Monitor", - PluralTitle: "Quality Monitors", + SingularName: "quality_monitor", + PluralName: "quality_monitors", + SingularTitle: "Quality Monitor", + PluralTitle: "Quality Monitors", + TerraformResourceName: "databricks_quality_monitor", }, "schemas": { - SingularName: "schema", - PluralName: "schemas", - SingularTitle: "Schema", - PluralTitle: "Schemas", + SingularName: "schema", + PluralName: "schemas", + SingularTitle: "Schema", + PluralTitle: "Schemas", + TerraformResourceName: "databricks_schema", }, "clusters": { - SingularName: "cluster", - PluralName: "clusters", - SingularTitle: "Cluster", - PluralTitle: "Clusters", + SingularName: "cluster", + PluralName: "clusters", + SingularTitle: "Cluster", + PluralTitle: "Clusters", + TerraformResourceName: "databricks_cluster", }, "dashboards": { - SingularName: "dashboard", - PluralName: "dashboards", - SingularTitle: "Dashboard", - PluralTitle: "Dashboards", + SingularName: "dashboard", + PluralName: "dashboards", + SingularTitle: "Dashboard", + PluralTitle: "Dashboards", + TerraformResourceName: "databricks_dashboard", }, "volumes": { - SingularName: "volume", - PluralName: "volumes", - SingularTitle: "Volume", - PluralTitle: "Volumes", + SingularName: "volume", + PluralName: "volumes", + SingularTitle: "Volume", + PluralTitle: "Volumes", + TerraformResourceName: "databricks_volume", }, "apps": { - SingularName: "app", - PluralName: "apps", - SingularTitle: "App", - PluralTitle: "Apps", + SingularName: "app", + PluralName: "apps", + SingularTitle: "App", + PluralTitle: "Apps", + TerraformResourceName: "databricks_app", }, } } diff --git a/bundle/deploy/terraform/util.go b/bundle/deploy/terraform/util.go index 953554916..90dfe37b2 100644 --- a/bundle/deploy/terraform/util.go +++ b/bundle/deploy/terraform/util.go @@ -33,8 +33,12 @@ type stateResourceInstance struct { } type stateInstanceAttributes struct { - ID string `json:"id"` - Name string `json:"name,omitempty"` // Some resources such as Apps do not have an ID, so we use the name instead. + ID string `json:"id"` + + // Some resources such as Apps do not have an ID, so we use the name instead. + // We need this for cases when such resource is removed from bundle config but + // exists in the workspace still so we can correctly display its summary. + Name string `json:"name,omitempty"` ETag string `json:"etag,omitempty"` } diff --git a/integration/bundle/apps_test.go b/integration/bundle/apps_test.go index 687384219..6d77db492 100644 --- a/integration/bundle/apps_test.go +++ b/integration/bundle/apps_test.go @@ -107,4 +107,7 @@ env: require.NoError(t, err) require.NotNil(t, app) require.Equal(t, apps.ApplicationStateRunning, app.AppStatus.State) + + // Redeploy it again just to check that it can be redeployed + deployBundle(t, ctx, root) }