addressed feedback

This commit is contained in:
Andrew Nester 2024-12-05 15:40:34 +01:00
parent d8f210a2ad
commit d8a9b2f4df
No known key found for this signature in database
GPG Key ID: 12BC628A44B7DA57
11 changed files with 155 additions and 74 deletions

41
bundle/apps/validate.go Normal file
View File

@ -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{}
}

View File

@ -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")
}

View File

@ -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/")

View File

@ -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)
})
}
}

View File

@ -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))
}))
})

View File

@ -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))
}

View File

@ -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
}

View File

@ -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
}

View File

@ -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 theres app.yml defined already, it will be overridden.
// If theres app.yml defined locally, DABs will raise an error.
Config map[string]interface{} `json:"config,omitempty"`
Permissions []Permission `json:"permissions,omitempty"`

View File

@ -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(),

View File

@ -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
}