Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for Databricks Apps in DABs #1928

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions bundle/apps/interpolate_variables.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package apps

import (
"context"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/dyn"
"github.com/databricks/cli/libs/dyn/dynvar"
)

type interpolateVariables struct{}

func (i *interpolateVariables) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
pattern := dyn.NewPattern(
dyn.Key("resources"),
dyn.Key("apps"),
dyn.AnyKey(),
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",
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
}

err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) {
return dyn.MapByPattern(root, pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
return dynvar.Resolve(v, func(path dyn.Path) (dyn.Value, error) {
key, ok := tfToConfigMap[path[0].Key()]
if ok {
path = dyn.NewPath(dyn.Key("resources"), dyn.Key(key)).Append(path[1:]...)
}

return dyn.GetByPath(root, path)
})
})
})

return diag.FromErr(err)
}

func (i *interpolateVariables) Name() string {
return "apps.InterpolateVariables"
}

func InterpolateVariables() bundle.Mutator {
return &interpolateVariables{}
}
49 changes: 49 additions & 0 deletions bundle/apps/interpolate_variables_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package apps

import (
"context"
"testing"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/config/resources"
"github.com/databricks/databricks-sdk-go/service/apps"
"github.com/stretchr/testify/require"
)

func TestAppInterpolateVariables(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Resources: config.Resources{
Apps: map[string]*resources.App{
"my_app_1": {
App: &apps.App{
Name: "my_app_1",
},
Config: map[string]any{
"command": []string{"echo", "hello"},
"env": []map[string]string{
{"name": "JOB_ID", "value": "${resources.jobs.my_job.id}"},
},
},
},
"my_app_2": {
App: &apps.App{
Name: "my_app_2",
},
},
},
Jobs: map[string]*resources.Job{
"my_job": {
ID: "123",
},
},
},
},
}

diags := bundle.Apply(context.Background(), b, InterpolateVariables())
require.Empty(t, diags)
require.Equal(t, []any([]any{map[string]any{"name": "JOB_ID", "value": "123"}}), b.Config.Resources.Apps["my_app_1"].Config["env"])
require.Nil(t, b.Config.Resources.Apps["my_app_2"].Config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test works but doesn't actually use the translation map.

I suspect you intend to make sure that the conversion of bundle-internal field references to Terraform resource field references is undone so that we can interpolate the IDs ourselves. But the test runs against a bundle-internal field reference.

Is there an alternative approach such that we don't have to convert back-and-forth? Maybe we completely skip this path during variable resolution? Or only skip resources.* references under resources.apps.*.config.**? Then we have less coupling with the TF resource interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep conversion to TF for all resources and then having this separate mutator which converts it back. The reasons are:

  1. We will still need to keep InterpolateVariables mutator in both cases (with or without the conversion)
  2. If we decide to add more fields to be processed this way we will need to add 2 changes in 2 places: a conditional skip in TF mutator, and the processing of the field in InterpolateVariables mutator. Without the skip we just need to add the change in 1 place (InterpolateVariables)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me.

The test now uses the TF notation, so it will fail when that is changed.

}
111 changes: 111 additions & 0 deletions bundle/apps/upload_config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package apps

import (
"bytes"
"context"
"fmt"
"path"
"strings"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/resources"
"github.com/databricks/cli/bundle/deploy"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/filer"
"golang.org/x/sync/errgroup"

"gopkg.in/yaml.v3"
)

type uploadConfig struct {
filerFactory deploy.FilerFactory
}

func (u *uploadConfig) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
var diags diag.Diagnostics
errGroup, ctx := errgroup.WithContext(ctx)

diagsPerApp := make(map[string]diag.Diagnostic)

for key, app := range b.Config.Resources.Apps {
// If the app has a config, we need to deploy it first.
// It means we need to write app.yml file with the content of the config field
// to the remote source code path of the app.
if app.Config != nil {
if !strings.HasPrefix(app.SourceCodePath, b.Config.Workspace.FilePath) {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
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)),
})

continue
}

appPath := strings.TrimPrefix(app.SourceCodePath, b.Config.Workspace.FilePath)
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjakobs @ilyakuz-db FYI, this is a potential problem for source-linked deployments. We assume here that files are deployed under ${workspace.file_path} and then write a file into it. If we were to make this work for source-linked deployments, this would have to write a file to the source folder itself, which in turn means we'll get validation errors (we check that the user does not have both an app.yml and a configuration section in the bundle configuration for their app.

The fact that we have to think about this (and potentially work around it) illustrates the problem that this duality brings.


buf, err := configToYaml(app)
if err != nil {
return diag.FromErr(err)
}

// When the app is started, create a new app deployment and wait for it to complete.
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
f, err := u.filerFactory(b)
if err != nil {
return diag.FromErr(err)
}

errGroup.Go(func() error {
err = f.Write(ctx, path.Join(appPath, "app.yml"), buf, filer.OverwriteIfExists)
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
diagsPerApp[key] = diag.Diagnostic{
Severity: diag.Error,
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
Summary: "Failed to save config",
Detail: fmt.Sprintf("Failed to write %s file: %s", path.Join(app.SourceCodePath, "app.yml"), err),
Locations: b.Config.GetLocations(fmt.Sprintf("resources.apps.%s", key)),
}
}
return nil
})
}
}

if err := errGroup.Wait(); err != nil {
return diags.Extend(diag.FromErr(err))
}

for _, diag := range diagsPerApp {
diags = append(diags, diag)
}
andrewnester marked this conversation as resolved.
Show resolved Hide resolved

return diags
}

// Name implements bundle.Mutator.
func (u *uploadConfig) Name() string {
return "apps:UploadConfig"
}

func UploadConfig() bundle.Mutator {
return &uploadConfig{
filerFactory: func(b *bundle.Bundle) (filer.Filer, error) {
return filer.NewWorkspaceFilesClient(b.WorkspaceClient(), b.Config.Workspace.FilePath)
},
}
}

func configToYaml(app *resources.App) (*bytes.Buffer, error) {
buf := bytes.NewBuffer(nil)
enc := yaml.NewEncoder(buf)
enc.SetIndent(2)

err := enc.Encode(app.Config)
defer enc.Close()

if err != nil {
return nil, fmt.Errorf("failed to encode app config to yaml: %w", err)
}

return buf, nil
}
69 changes: 69 additions & 0 deletions bundle/apps/upload_config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package apps

import (
"bytes"
"context"
"os"
"path/filepath"
"testing"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/config/resources"
mockfiler "github.com/databricks/cli/internal/mocks/libs/filer"
"github.com/databricks/cli/libs/filer"
"github.com/databricks/cli/libs/vfs"
"github.com/databricks/databricks-sdk-go/service/apps"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func TestAppUploadConfig(t *testing.T) {
root := t.TempDir()
err := os.MkdirAll(filepath.Join(root, "my_app"), 0o700)
require.NoError(t, err)

b := &bundle.Bundle{
BundleRootPath: root,
SyncRoot: vfs.MustNew(root),
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/Workspace/Users/[email protected]/",
},
Resources: config.Resources{
Apps: map[string]*resources.App{
"my_app": {
App: &apps.App{
Name: "my_app",
},
SourceCodePath: "./my_app",
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
Config: map[string]any{
"command": []string{"echo", "hello"},
"env": []map[string]string{
{"name": "MY_APP", "value": "my value"},
},
},
},
},
},
},
}

mockFiler := mockfiler.NewMockFiler(t)
mockFiler.EXPECT().Write(mock.Anything, "my_app/app.yml", bytes.NewBufferString(`command:
- echo
- hello
env:
- name: MY_APP
value: my value
`), filer.OverwriteIfExists).Return(nil)

u := uploadConfig{
filerFactory: func(b *bundle.Bundle) (filer.Filer, error) {
return mockFiler, nil
},
}

diags := bundle.Apply(context.Background(), b, &u)
require.NoError(t, diags.Error())
}
40 changes: 40 additions & 0 deletions bundle/apps/validate.go
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package apps

import (
"context"
"fmt"
"path"

"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
possibleConfigFiles := []string{"app.yml", "app.yaml"}

for _, app := range b.Config.Resources.Apps {
for _, configFile := range possibleConfigFiles {
cf := path.Join(app.SourceCodePath, configFile)
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
if _, err := b.SyncRoot.Stat(cf); err == nil {
diags = append(diags, diag.Diagnostic{
Severity: diag.Error,
Summary: fmt.Sprintf("%s detected", configFile),
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{}
}
55 changes: 55 additions & 0 deletions bundle/apps/validate_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
Loading
Loading