From d8a9b2f4df2dd8ba4ccf320d7b597c26d4cce11a Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 5 Dec 2024 15:40:34 +0100 Subject: [PATCH] addressed feedback --- bundle/apps/validate.go | 41 +++++++++++++ bundle/apps/validate_test.go | 55 ++++++++++++++++++ bundle/config/mutator/apply_presets.go | 13 +---- bundle/config/mutator/apply_presets_test.go | 57 ------------------- bundle/config/mutator/merge_apps.go | 2 +- bundle/config/mutator/merge_apps_test.go | 11 +++- .../mutator/process_target_mode_test.go | 6 ++ bundle/config/mutator/run_as.go | 10 ++++ bundle/config/resources/apps.go | 3 +- bundle/phases/initialize.go | 3 + libs/dyn/merge/elements_by_key.go | 28 ++++++++- 11 files changed, 155 insertions(+), 74 deletions(-) create mode 100644 bundle/apps/validate.go create mode 100644 bundle/apps/validate_test.go diff --git a/bundle/apps/validate.go b/bundle/apps/validate.go new file mode 100644 index 000000000..5e3082f2d --- /dev/null +++ b/bundle/apps/validate.go @@ -0,0 +1,41 @@ +package apps + +import ( + "context" + "fmt" + "os" + "path/filepath" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" +) + +type validate struct { +} + +func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + for _, app := range b.Config.Resources.Apps { + possibleConfigFiles := []string{"app.yml", "app.yaml"} + for _, configFile := range possibleConfigFiles { + cf := filepath.Join(b.SyncRootPath, app.SourceCodePath, configFile) + if _, err := os.Stat(cf); err == nil { + diags = append(diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: "app.yml detected", + Detail: fmt.Sprintf("remove %s and use 'config' property for app resource '%s' instead", cf, app.Name), + }) + } + } + } + + return diags +} + +func (v *validate) Name() string { + return "apps.Validate" +} + +func Validate() bundle.Mutator { + return &validate{} +} diff --git a/bundle/apps/validate_test.go b/bundle/apps/validate_test.go new file mode 100644 index 000000000..d14d23731 --- /dev/null +++ b/bundle/apps/validate_test.go @@ -0,0 +1,55 @@ +package apps + +import ( + "context" + "path/filepath" + "testing" + + "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" + "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/vfs" + "github.com/databricks/databricks-sdk-go/service/apps" + "github.com/stretchr/testify/require" +) + +func TestAppsValidate(t *testing.T) { + tmpDir := t.TempDir() + testutil.Touch(t, tmpDir, "app1", "app.yml") + testutil.Touch(t, tmpDir, "app2", "app.py") + + b := &bundle.Bundle{ + BundleRootPath: tmpDir, + SyncRootPath: tmpDir, + SyncRoot: vfs.MustNew(tmpDir), + Config: config.Root{ + Resources: config.Resources{ + Apps: map[string]*resources.App{ + "app1": { + App: &apps.App{ + Name: "app1", + }, + SourceCodePath: "./app1", + }, + "app2": { + App: &apps.App{ + Name: "app2", + }, + SourceCodePath: "./app2", + }, + }, + }, + }, + } + + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}}) + + diags := bundle.Apply(context.Background(), b, bundle.Seq(mutator.TranslatePaths(), Validate())) + require.Len(t, diags, 1) + require.Equal(t, "app.yml detected", diags[0].Summary) + require.Contains(t, diags[0].Detail, "app.yml and use 'config' property for app resource") +} diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 57b765a8d..049fa9c8f 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -222,18 +222,7 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos dashboard.DisplayName = prefix + dashboard.DisplayName } - // Apps: Prefix - for key, app := range r.Apps { - if app == nil || app.App == nil { - diags = diags.Extend(diag.Errorf("app %s is not defined", key)) - continue - } - - app.Name = textutil.NormalizeString(prefix + app.Name) - // Normalize the app name to ensure it is a valid identifier. - // App supports only alphanumeric characters and hyphens. - app.Name = strings.ReplaceAll(app.Name, "_", "-") - } + // Apps: No presets if config.IsExplicitlyEnabled((b.Config.Presets.SourceLinkedDeployment)) { isDatabricksWorkspace := dbr.RunsOnRuntime(ctx) && strings.HasPrefix(b.SyncRootPath, "/Workspace/") diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 2af54e74e..c26f20383 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -12,7 +12,6 @@ import ( "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/apps" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/require" @@ -483,59 +482,3 @@ func TestApplyPresetsSourceLinkedDeployment(t *testing.T) { }) } } - -func TestApplyPresetsPrefixForApps(t *testing.T) { - tests := []struct { - name string - prefix string - app *resources.App - want string - }{ - { - name: "add prefix to app", - prefix: "[prefix] ", - app: &resources.App{ - App: &apps.App{ - Name: "app1", - }, - }, - want: "prefix-app1", - }, - { - name: "add empty prefix to app", - prefix: "", - app: &resources.App{ - App: &apps.App{ - Name: "app1", - }, - }, - want: "app1", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Resources: config.Resources{ - Apps: map[string]*resources.App{ - "app1": tt.app, - }, - }, - 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.Apps["app1"].Name) - }) - } -} diff --git a/bundle/config/mutator/merge_apps.go b/bundle/config/mutator/merge_apps.go index 88c745a83..3be9ae6d2 100644 --- a/bundle/config/mutator/merge_apps.go +++ b/bundle/config/mutator/merge_apps.go @@ -37,7 +37,7 @@ func (m *mergeApps) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic } return dyn.Map(v, "resources.apps", dyn.Foreach(func(_ dyn.Path, app dyn.Value) (dyn.Value, error) { - return dyn.Map(app, "resources", merge.ElementsByKey("name", m.resourceName)) + return dyn.Map(app, "resources", merge.ElementsByKeyWithOverride("name", m.resourceName)) })) }) diff --git a/bundle/config/mutator/merge_apps_test.go b/bundle/config/mutator/merge_apps_test.go index 2cdef830e..0a161b845 100644 --- a/bundle/config/mutator/merge_apps_test.go +++ b/bundle/config/mutator/merge_apps_test.go @@ -42,6 +42,13 @@ func TestMergeApps(t *testing.T) { Permission: "CAN_MANAGE", }, }, + { + Name: "sql1", + Job: &apps.AppResourceJob{ + Id: "9876", + Permission: "CAN_MANAGE", + }, + }, }, }, }, @@ -60,5 +67,7 @@ func TestMergeApps(t *testing.T) { assert.Equal(t, "sql1", j.Resources[1].Name) assert.Equal(t, "CAN_MANAGE", string(j.Resources[0].Job.Permission)) - assert.Equal(t, "CAN_USE", string(j.Resources[1].SqlWarehouse.Permission)) + + assert.Nil(t, j.Resources[1].SqlWarehouse) + assert.Equal(t, "CAN_MANAGE", string(j.Resources[1].Job.Permission)) } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index ce75dc2fa..db55e8edc 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -426,6 +426,12 @@ func TestAllNonUcResourcesAreRenamed(t *testing.T) { for _, key := range field.MapKeys() { resource := field.MapIndex(key) nameField := resource.Elem().FieldByName("Name") + + // Skip apps, as they are not renamed + if resourceType == "Apps" { + continue + } + if !nameField.IsValid() || nameField.Kind() != reflect.String { continue } diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 7ffd782c2..3d7391b01 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -119,6 +119,16 @@ func validateRunAs(b *bundle.Bundle) diag.Diagnostics { )) } + // Apps do not support run_as in the API. + if len(b.Config.Resources.Apps) > 0 { + diags = diags.Extend(reportRunAsNotSupported( + "apps", + b.Config.GetLocation("resources.apps"), + b.Config.Workspace.CurrentUser.UserName, + identity, + )) + } + return diags } diff --git a/bundle/config/resources/apps.go b/bundle/config/resources/apps.go index e6d3f00e8..748411096 100644 --- a/bundle/config/resources/apps.go +++ b/bundle/config/resources/apps.go @@ -14,6 +14,7 @@ import ( type App struct { // This represents the id which is the name of the app that can be used // as a reference in other resources. This value is returned by terraform. + // This equals to app name and added for symmetry with other resources. ID string `json:"id,omitempty" bundle:"readonly"` // SourceCodePath is a required field used by DABs to point databricks app source code @@ -23,7 +24,7 @@ type App struct { // Config is an optional field which allows configuring the app following Databricks app configuration format like in app.yml. // When this field is set, DABs read the configuration set in this field and write // it to app.yml in the root of the source code folder in Databricks workspace. - // If there’s app.yml defined already, it will be overridden. + // If there’s app.yml defined locally, DABs will raise an error. Config map[string]interface{} `json:"config,omitempty"` Permissions []Permission `json:"permissions,omitempty"` diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index e8a6c8504..8585912a8 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -2,6 +2,7 @@ package phases import ( "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/apps" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" pythonmutator "github.com/databricks/cli/bundle/config/mutator/python" @@ -82,6 +83,8 @@ func Initialize() bundle.Mutator { mutator.TranslatePaths(), trampoline.WrapperWarning(), + apps.Validate(), + permissions.ValidateSharedRootPermissions(), permissions.ApplyBundlePermissions(), permissions.FilterCurrentUser(), diff --git a/libs/dyn/merge/elements_by_key.go b/libs/dyn/merge/elements_by_key.go index e6e640d14..e94c5fdc7 100644 --- a/libs/dyn/merge/elements_by_key.go +++ b/libs/dyn/merge/elements_by_key.go @@ -7,7 +7,7 @@ type elementsByKey struct { keyFunc func(dyn.Value) string } -func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { +func (e elementsByKey) doMap(_ dyn.Path, v dyn.Value, mergeFunc func(a dyn.Value, b dyn.Value) (dyn.Value, error)) (dyn.Value, error) { // We know the type of this value is a sequence. // For additional defence, return self if it is not. elements, ok := v.AsSequence() @@ -33,7 +33,7 @@ func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { } // Merge this instance into the reference. - nv, err := Merge(ref, elements[i]) + nv, err := mergeFunc(ref, elements[i]) if err != nil { return v, err } @@ -55,6 +55,26 @@ func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return dyn.NewValue(out, v.Locations()), nil } +func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + return e.doMap(nil, v, Merge) +} + +func (e elementsByKey) MapWithOverride(p dyn.Path, v dyn.Value) (dyn.Value, error) { + return e.doMap(nil, v, func(a dyn.Value, b dyn.Value) (dyn.Value, error) { + return Override(a, b, OverrideVisitor{ + VisitInsert: func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + return v, nil + }, + VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { + return nil + }, + VisitUpdate: func(_ dyn.Path, a dyn.Value, b dyn.Value) (dyn.Value, error) { + return b, nil + }, + }) + }) +} + // ElementsByKey returns a [dyn.MapFunc] that operates on a sequence // where each element is a map. It groups elements by a key and merges // elements with the same key. @@ -65,3 +85,7 @@ func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { func ElementsByKey(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { return elementsByKey{key, keyFunc}.Map } + +func ElementsByKeyWithOverride(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { + return elementsByKey{key, keyFunc}.MapWithOverride +}