-
Notifications
You must be signed in to change notification settings - Fork 72
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
Source-linked deployments for bundles in the workspace #1884
Merged
Merged
Changes from all 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 ae26bea
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db fbb9be9
fix: Add SyncRoot mock to tests
ilyakuz-db 086dfbc
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db a43f7dd
feat: Use global FilePath instead of locally overriden paths
ilyakuz-db 5a22151
feat: Move FilePath update to apply preset step
ilyakuz-db 6c12308
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db b2164c0
fix: Use SyncRootPath to allow using parent directories
ilyakuz-db 0004ed2
Merge branch 'main' of github.com:databricks/cli into tmp/in-place-pr…
ilyakuz-db 26f2453
feat: Use dbr package for runtime check
ilyakuz-db 51c8ef5
fix: Applies autoformat
ilyakuz-db 4cf6929
fix: Missing root variable
ilyakuz-db 8cd95eb
test: Apply presets unit test
ilyakuz-db e9b7289
test: Process target mode
ilyakuz-db a3c6d57
fix: Use SyncRootPath
ilyakuz-db 80ea3a0
fix: Move SyncRoot field to the top og the struct
ilyakuz-db e7165ec
feat: Add Databricks Workspace conditions to dev mode setting. Add wa…
ilyakuz-db 49f6bc9
fix: Skipping in-place tests on Windows
ilyakuz-db e65b50c
fix: Wrong condition
ilyakuz-db 53e1f6d
fix: Skip permissions set and check for in-place
ilyakuz-db 00bb683
fix: Remove unnecessary fields in apply presets test
ilyakuz-db e8825d5
fix: Reuse existing `root` variable
ilyakuz-db 1d7b27e
feat: Rename "in-place" to "source-linked"
ilyakuz-db 55f715d
fix: Broken test
ilyakuz-db 518aa14
Revert "fix: Skip permissions set and check for in-place"
ilyakuz-db aeb9813
feat: Use path translations instead of overriding config
ilyakuz-db 8c8fb35
fix: Add explicit warning when using python wheel wrappers with sourc…
ilyakuz-db 234d971
fix: Cleanup in test
ilyakuz-db 5d09070
fix: Cleanup
ilyakuz-db 8ee8de5
test: Added path translation test cases for source-linked
ilyakuz-db 5d04569
fix: Windows tests
ilyakuz-db 530a2a9
fix: Skipping path translation tests for windows
ilyakuz-db aba35ae
fix: Replaced diag warning message with logger and removed unnecessar…
ilyakuz-db ddb68a7
fix: Update skip uploading log entry with more clear message
ilyakuz-db eede522
Update bundle/config/presets.go
ilyakuz-db 6a036d1
Merge branch 'tmp/in-place-prototype' of github.com:databricks/cli in…
ilyakuz-db 66bf5f3
fix: Use warning instead of log in python wheel wrapper message
ilyakuz-db File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,14 @@ package mutator_test | |
|
||
import ( | ||
"context" | ||
"runtime" | ||
"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/libs/dbr" | ||
"github.com/databricks/databricks-sdk-go/service/catalog" | ||
"github.com/databricks/databricks-sdk-go/service/jobs" | ||
"github.com/stretchr/testify/require" | ||
|
@@ -364,3 +366,86 @@ func TestApplyPresetsResourceNotDefined(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestApplyPresetsSourceLinkedDeployment(t *testing.T) { | ||
if runtime.GOOS == "windows" { | ||
t.Skip("this test is not applicable on Windows because source-linked mode works only in the Databricks Workspace") | ||
} | ||
|
||
testContext := context.Background() | ||
enabled := true | ||
disabled := false | ||
workspacePath := "/Workspace/[email protected]" | ||
|
||
tests := []struct { | ||
bundlePath string | ||
ctx context.Context | ||
name string | ||
initialValue *bool | ||
expectedValue *bool | ||
expectedWarning string | ||
}{ | ||
{ | ||
name: "preset enabled, bundle in Workspace, databricks runtime", | ||
bundlePath: workspacePath, | ||
ctx: dbr.MockRuntime(testContext, true), | ||
initialValue: &enabled, | ||
expectedValue: &enabled, | ||
}, | ||
{ | ||
name: "preset enabled, bundle not in Workspace, databricks runtime", | ||
bundlePath: "/Users/[email protected]", | ||
ctx: dbr.MockRuntime(testContext, true), | ||
initialValue: &enabled, | ||
expectedValue: &disabled, | ||
expectedWarning: "source-linked deployment is available only in the Databricks Workspace", | ||
}, | ||
{ | ||
name: "preset enabled, bundle in Workspace, not databricks runtime", | ||
bundlePath: workspacePath, | ||
ctx: dbr.MockRuntime(testContext, false), | ||
initialValue: &enabled, | ||
expectedValue: &disabled, | ||
expectedWarning: "source-linked deployment is available only in the Databricks Workspace", | ||
}, | ||
{ | ||
name: "preset disabled, bundle in Workspace, databricks runtime", | ||
bundlePath: workspacePath, | ||
ctx: dbr.MockRuntime(testContext, true), | ||
initialValue: &disabled, | ||
expectedValue: &disabled, | ||
}, | ||
{ | ||
name: "preset nil, bundle in Workspace, databricks runtime", | ||
bundlePath: workspacePath, | ||
ctx: dbr.MockRuntime(testContext, true), | ||
initialValue: nil, | ||
expectedValue: nil, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
b := &bundle.Bundle{ | ||
SyncRootPath: tt.bundlePath, | ||
Config: config.Root{ | ||
Presets: config.Presets{ | ||
SourceLinkedDeployment: tt.initialValue, | ||
}, | ||
}, | ||
} | ||
|
||
diags := bundle.Apply(tt.ctx, b, mutator.ApplyPresets()) | ||
if diags.HasError() { | ||
t.Fatalf("unexpected error: %v", diags) | ||
} | ||
|
||
if tt.expectedWarning != "" { | ||
require.Equal(t, tt.expectedWarning, diags[0].Summary) | ||
} | ||
|
||
require.Equal(t, tt.expectedValue, b.Config.Presets.SourceLinkedDeployment) | ||
}) | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,17 @@ package mutator | |
import ( | ||
"context" | ||
"reflect" | ||
"runtime" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/databricks/cli/bundle" | ||
"github.com/databricks/cli/bundle/config" | ||
"github.com/databricks/cli/bundle/config/resources" | ||
"github.com/databricks/cli/libs/dbr" | ||
"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" | ||
|
@@ -133,6 +136,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { | |
}, | ||
}, | ||
}, | ||
SyncRoot: vfs.MustNew("/Users/[email protected]"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably fails on Windows. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
@@ -515,3 +519,32 @@ func TestPipelinesDevelopmentDisabled(t *testing.T) { | |
|
||
assert.False(t, b.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) | ||
} | ||
|
||
func TestSourceLinkedDeploymentEnabled(t *testing.T) { | ||
b, diags := processSourceLinkedBundle(t, true) | ||
require.NoError(t, diags.Error()) | ||
assert.True(t, *b.Config.Presets.SourceLinkedDeployment) | ||
} | ||
|
||
func TestSourceLinkedDeploymentDisabled(t *testing.T) { | ||
b, diags := processSourceLinkedBundle(t, false) | ||
require.NoError(t, diags.Error()) | ||
assert.False(t, *b.Config.Presets.SourceLinkedDeployment) | ||
} | ||
|
||
func processSourceLinkedBundle(t *testing.T, presetEnabled bool) (*bundle.Bundle, diag.Diagnostics) { | ||
if runtime.GOOS == "windows" { | ||
t.Skip("this test is not applicable on Windows because source-linked mode works only in the Databricks Workspace") | ||
} | ||
|
||
b := mockBundle(config.Development) | ||
|
||
workspacePath := "/Workspace/[email protected]/" | ||
b.SyncRootPath = workspacePath | ||
b.Config.Presets.SourceLinkedDeployment = &presetEnabled | ||
|
||
ctx := dbr.MockRuntime(context.Background(), true) | ||
m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) | ||
diags := bundle.Apply(ctx, b, m) | ||
return b, diags | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings should include position/file/line information where possible.
This condition triggers only if the user explicitly configured the setting, so we should have this information. You can call
b.Config.GetLocations()
with the path to the setting to get these.Not blocking for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this setting in
targets.[target].presets.source_linked_deployment
and this should be in the config indeed in that case, but we have nil inb.Config.Targets
which makes location lookup not possibleIt is assigned here
cli/bundle/config/mutator/select_target.go
Line 53 in ed19466
I can try to remove this nil assingment from there, not sure why it is needed. If I remove this line locations are available
Also we can show something like this without locations
![image](https://private-user-images.githubusercontent.com/181845775/388080226-80ccf1a5-9e8f-43bf-af5a-0cd6bf7ee74d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk5MzMwMzksIm5iZiI6MTczOTkzMjczOSwicGF0aCI6Ii8xODE4NDU3NzUvMzg4MDgwMjI2LTgwY2NmMWE1LTllOGYtNDNiZi1hZjVhLTBjZDZiZjdlZTc0ZC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxOVQwMjM4NTlaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT04ODAzY2VjYTU2NTM1ZTgzNmQ1ODVmNWE5N2FkZWU4YmJjMjgyODkzMWFhMjMzYjgyOWZjODE5NTZlMDNlNzNjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.klEZhVpEWXCC6kTTJhoQEZ5KMhkLDtiTGd64mo9Gxaw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just be able to use the locations from top-level
presets.source_linked_deployment
. That should include the target override locations as well IIRC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works indeed, thanks!
Will make another PR later