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.

Sign up for GitHub

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

Source-linked deployments for bundles in the workspace #1884

Merged
merged 37 commits into from
Nov 20, 2024
Merged

Source-linked deployments for bundles in the workspace #1884

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
6b22fde
Prototype for in-place deployments in DEV mode
ilyakuz-db Nov 5, 2024
ae26bea
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db Nov 5, 2024
fbb9be9
fix: Add SyncRoot mock to tests
ilyakuz-db Nov 5, 2024
086dfbc
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db Nov 11, 2024
a43f7dd
feat: Use global FilePath instead of locally overriden paths
ilyakuz-db Nov 11, 2024
5a22151
feat: Move FilePath update to apply preset step
ilyakuz-db Nov 12, 2024
6c12308
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db Nov 12, 2024
b2164c0
fix: Use SyncRootPath to allow using parent directories
ilyakuz-db Nov 12, 2024
0004ed2
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db Nov 14, 2024
26f2453
feat: Use dbr package for runtime check
ilyakuz-db Nov 14, 2024
51c8ef5
fix: Applies autoformat
ilyakuz-db Nov 14, 2024
4cf6929
fix: Missing root variable
ilyakuz-db Nov 14, 2024
8cd95eb
test: Apply presets unit test
ilyakuz-db Nov 15, 2024
e9b7289
test: Process target mode
ilyakuz-db Nov 15, 2024
a3c6d57
fix: Use SyncRootPath
ilyakuz-db Nov 18, 2024
80ea3a0
fix: Move SyncRoot field to the top og the struct
ilyakuz-db Nov 18, 2024
e7165ec
feat: Add Databricks Workspace conditions to dev mode setting. Add wa…
ilyakuz-db Nov 18, 2024
49f6bc9
fix: Skipping in-place tests on Windows
ilyakuz-db Nov 18, 2024
e65b50c
fix: Wrong condition
ilyakuz-db Nov 18, 2024
53e1f6d
fix: Skip permissions set and check for in-place
ilyakuz-db Nov 18, 2024
00bb683
fix: Remove unnecessary fields in apply presets test
ilyakuz-db Nov 18, 2024
e8825d5
fix: Reuse existing `root` variable
ilyakuz-db Nov 18, 2024
1d7b27e
feat: Rename "in-place" to "source-linked"
ilyakuz-db Nov 18, 2024
55f715d
fix: Broken test
ilyakuz-db Nov 18, 2024
518aa14
Revert "fix: Skip permissions set and check for in-place"
ilyakuz-db Nov 18, 2024
aeb9813
feat: Use path translations instead of overriding config
ilyakuz-db Nov 19, 2024
8c8fb35
fix: Add explicit warning when using python wheel wrappers with sourc…
ilyakuz-db Nov 19, 2024
234d971
fix: Cleanup in test
ilyakuz-db Nov 19, 2024
5d09070
fix: Cleanup
ilyakuz-db Nov 19, 2024
8ee8de5
test: Added path translation test cases for source-linked
ilyakuz-db Nov 19, 2024
5d04569
fix: Windows tests
ilyakuz-db Nov 19, 2024
530a2a9
fix: Skipping path translation tests for windows
ilyakuz-db Nov 19, 2024
aba35ae
fix: Replaced diag warning message with logger and removed unnecessar…
ilyakuz-db Nov 19, 2024
ddb68a7
fix: Update skip uploading log entry with more clear message
ilyakuz-db Nov 20, 2024
eede522
Update bundle/config/presets.go
ilyakuz-db Nov 20, 2024
6a036d1
Merge branch 'tmp/in-place-prototype' of github.com:databricks/cli in…
ilyakuz-db Nov 20, 2024
66bf5f3
fix: Use warning instead of log in python wheel wrapper message
ilyakuz-db Nov 20, 2024
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
14 changes: 14 additions & 0 deletions bundle/config/mutator/apply_presets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/dyn"
"github.com/databricks/cli/libs/env"
"github.com/databricks/cli/libs/textutil"
"github.com/databricks/databricks-sdk-go/service/catalog"
"github.com/databricks/databricks-sdk-go/service/jobs"
Expand Down Expand Up @@ -221,6 +222,19 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos
dashboard.DisplayName = prefix + dashboard.DisplayName
}

if config.IsExplicitlyEnabled((b.Config.Presets.InPlaceDeployment)) {
ilyakuz-db marked this conversation as resolved.
Show resolved Hide resolved
ilyakuz-db marked this conversation as resolved.
Show resolved Hide resolved
root := b.SyncRoot.Native()
Copy link
Contributor

Choose a reason for hiding this comment

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

b.SyncRootPath contains the same (for convenience)

_, ok := env.Lookup(ctx, envDatabricksRuntimeVersion)
isInWorkspace := ok && strings.HasPrefix(root, "/Workspace/")
ilyakuz-db marked this conversation as resolved.
Show resolved Hide resolved

if isInWorkspace {
b.Config.Workspace.FilePath = b.BundleRootPath
ilyakuz-db marked this conversation as resolved.
Show resolved Hide resolved
ilyakuz-db marked this conversation as resolved.
Show resolved Hide resolved
} else {
disabled := false
b.Config.Presets.InPlaceDeployment = &disabled
}
}

return diags
}

Expand Down
7 changes: 7 additions & 0 deletions bundle/config/mutator/apply_presets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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/libs/vfs"
"github.com/databricks/databricks-sdk-go/service/catalog"
"github.com/databricks/databricks-sdk-go/service/jobs"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -55,6 +56,7 @@ func TestApplyPresetsPrefix(t *testing.T) {
NamePrefix: tt.prefix,
},
},
SyncRoot: vfs.MustNew(t.TempDir()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move all instances to the top of the struct initialization (I see a mix of both).

}

ctx := context.Background()
Expand Down Expand Up @@ -101,6 +103,7 @@ func TestApplyPresetsPrefixForUcSchema(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
b := &bundle.Bundle{
SyncRoot: vfs.MustNew(t.TempDir()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to add for existing tests? The change above does not seem to affect these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, it was left from prev implementation where sync root was called unconditionally

Thanks!

Config: config.Root{
Resources: config.Resources{
Schemas: map[string]*resources.Schema{
Expand Down Expand Up @@ -180,6 +183,7 @@ func TestApplyPresetsTags(t *testing.T) {
Tags: tt.tags,
},
},
SyncRoot: vfs.MustNew(t.TempDir()),
}

ctx := context.Background()
Expand Down Expand Up @@ -239,6 +243,7 @@ func TestApplyPresetsJobsMaxConcurrentRuns(t *testing.T) {
JobsMaxConcurrentRuns: tt.setting,
},
},
SyncRoot: vfs.MustNew(t.TempDir()),
}
ctx := context.Background()
diag := bundle.Apply(ctx, b, mutator.ApplyPresets())
Expand All @@ -264,6 +269,7 @@ func TestApplyPresetsPrefixWithoutJobSettings(t *testing.T) {
NamePrefix: "prefix-",
},
},
SyncRoot: vfs.MustNew(t.TempDir()),
}

ctx := context.Background()
Expand Down Expand Up @@ -355,6 +361,7 @@ func TestApplyPresetsResourceNotDefined(t *testing.T) {
TriggerPauseStatus: config.Paused,
},
},
SyncRoot: vfs.MustNew(t.TempDir()),
}

ctx := context.Background()
Expand Down
5 changes: 5 additions & 0 deletions bundle/config/mutator/process_target_mode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) {
t.TriggerPauseStatus = config.Paused
}

if !config.IsExplicitlyDisabled(t.InPlaceDeployment) {
enabled := true
t.InPlaceDeployment = &enabled
}

pietern marked this conversation as resolved.
Show resolved Hide resolved
if !config.IsExplicitlyDisabled(t.PipelinesDevelopment) {
enabled := true
t.PipelinesDevelopment = &enabled
Expand Down
2 changes: 2 additions & 0 deletions bundle/config/mutator/process_target_mode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/databricks/cli/bundle/config/resources"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/tags"
"github.com/databricks/cli/libs/vfs"
sdkconfig "github.com/databricks/databricks-sdk-go/config"
"github.com/databricks/databricks-sdk-go/service/catalog"
"github.com/databricks/databricks-sdk-go/service/compute"
Expand Down Expand Up @@ -133,6 +134,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle {
},
},
},
SyncRoot: vfs.MustNew("/Users/[email protected]"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably fails on Windows.

Copy link
Contributor Author

@ilyakuz-db ilyakuz-db Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added skipping for windows since we don't need to test the behavior there (unless I'm missing something)

// Use AWS implementation for testing.
Tagging: tags.ForCloud(&sdkconfig.Config{
Host: "https://company.cloud.databricks.com",
Expand Down
5 changes: 5 additions & 0 deletions bundle/config/presets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ type Presets struct {
// JobsMaxConcurrentRuns is the default value for the max concurrent runs of jobs.
JobsMaxConcurrentRuns int `json:"jobs_max_concurrent_runs,omitempty"`

// InPlaceDeployment indicates whether in-place deployment is enabled. Works only in workspace
// When set to true, resources created during deployment will point to source files in the workspace instead of their workspace copies.
// No resources will be uploaded to workspace
ilyakuz-db marked this conversation as resolved.
Show resolved Hide resolved
InPlaceDeployment *bool `json:"in_place_deployment,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is a bit confusion tbh, I'd better use something like NoFilesUpload or UseWorkspaceFiles or smth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to go with SourceLinkedDeployment


// Tags to add to all resources.
Tags map[string]string `json:"tags,omitempty"`
}
Expand Down
6 changes: 6 additions & 0 deletions bundle/deploy/files/upload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io/fs"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/permissions"
"github.com/databricks/cli/libs/cmdio"
"github.com/databricks/cli/libs/diag"
Expand All @@ -23,6 +24,11 @@ func (m *upload) Name() string {
}

func (m *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
if config.IsExplicitlyEnabled(b.Config.Presets.InPlaceDeployment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we skip it for libraries.Upload as well? This might get used when t have a path to local libraries in their bundle configuration.
There it gets a bit tricky because libraries support glob patterns like "*.whl" and etc. and later replaced with the path where artifact is actually uploaded.

We might as well disable local libraries support if in place deployment is used. @pietern thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to keep libraries as is to simplify in-place mode, all whl and jar files will still be uploaded (both with and without build steps)

cmdio.LogString(ctx, "Bundle files uploading skipped: in-place deployment is enabled")
return nil
}

cmdio.LogString(ctx, fmt.Sprintf("Uploading bundle files to %s...", b.Config.Workspace.FilePath))
opts, err := GetSyncOptions(ctx, bundle.ReadOnly(b))
if err != nil {
Expand Down
Loading