Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 215
Add paths field to bundle sync configuration#1694
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
f01c8cd020d447bee14af13e22a43ad57ff811a08d6f5679d8a3f93b8e568cbd0d2501a96892812e7cc056d1f2f03bb6e364de68bf468f2898ef3f47a6a7eb7ea37a7f18c453File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package mutator | ||
| import ( | ||
| "context" | ||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/libs/diag" | ||
| "github.com/databricks/cli/libs/dyn" | ||
| ) | ||
| type syncDefaultPath struct{} | ||
| // SyncDefaultPath configures the default sync path to be equal to the bundle root. | ||
| func SyncDefaultPath() bundle.Mutator { | ||
| return &syncDefaultPath{} | ||
| } | ||
| func (m *syncDefaultPath) Name() string { | ||
| return "SyncDefaultPath" | ||
| } | ||
| func (m *syncDefaultPath) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
| isset := false | ||
| err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { | ||
| pv, _ := dyn.Get(v, "sync.paths") | ||
| // If the sync paths field is already set, do nothing. | ||
| // We know it is set if its value is either a nil or a sequence (empty or not). | ||
| switch pv.Kind() { | ||
| case dyn.KindNil, dyn.KindSequence: | ||
| isset = true | ||
| } | ||
| return v, nil | ||
| }) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| // If the sync paths field is already set, do nothing. | ||
| if isset { | ||
| return nil | ||
| } | ||
| // Set the sync paths to the default value. | ||
| b.Config.Sync.Paths = []string{"."} | ||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| package mutator_test | ||
| import ( | ||
| "context" | ||
| "testing" | ||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/bundle/config" | ||
| "github.com/databricks/cli/bundle/config/mutator" | ||
| "github.com/databricks/cli/libs/diag" | ||
| "github.com/databricks/cli/libs/dyn" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
| func TestSyncDefaultPath_DefaultIfUnset(t *testing.T) { | ||
| b := &bundle.Bundle{ | ||
| RootPath: "/tmp/some/dir", | ||
| Config: config.Root{}, | ||
| } | ||
| ctx := context.Background() | ||
| diags := bundle.Apply(ctx, b, mutator.SyncDefaultPath()) | ||
| require.NoError(t, diags.Error()) | ||
| assert.Equal(t, []string{"."}, b.Config.Sync.Paths) | ||
| } | ||
| func TestSyncDefaultPath_SkipIfSet(t *testing.T) { | ||
| tcases := []struct { | ||
| name string | ||
| paths dyn.Value | ||
| expect []string | ||
| }{ | ||
| { | ||
| name: "nil", | ||
| paths: dyn.V(nil), | ||
| expect: nil, | ||
| }, | ||
| { | ||
| name: "empty sequence", | ||
| paths: dyn.V([]dyn.Value{}), | ||
| expect: []string{}, | ||
| }, | ||
| { | ||
| name: "non-empty sequence", | ||
| paths: dyn.V([]dyn.Value{dyn.V("something")}), | ||
| expect: []string{"something"}, | ||
| }, | ||
| } | ||
| for _, tcase := range tcases { | ||
| t.Run(tcase.name, func(t *testing.T) { | ||
| b := &bundle.Bundle{ | ||
| RootPath: "/tmp/some/dir", | ||
| Config: config.Root{}, | ||
| } | ||
| diags := bundle.ApplyFunc(context.Background(), b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
| err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { | ||
| v, err := dyn.Set(v, "sync", dyn.V(dyn.NewMapping())) | ||
| if err != nil { | ||
| return dyn.InvalidValue, err | ||
| } | ||
| v, err = dyn.Set(v, "sync.paths", tcase.paths) | ||
| if err != nil { | ||
| return dyn.InvalidValue, err | ||
| } | ||
| return v, nil | ||
| }) | ||
| return diag.FromErr(err) | ||
| }) | ||
| require.NoError(t, diags.Error()) | ||
| ctx := context.Background() | ||
| diags = bundle.Apply(ctx, b, mutator.SyncDefaultPath()) | ||
| require.NoError(t, diags.Error()) | ||
| // If the sync paths field is already set, do nothing. | ||
| assert.Equal(t, tcase.expect, b.Config.Sync.Paths) | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package mutator | ||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "path/filepath" | ||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/libs/diag" | ||
| "github.com/databricks/cli/libs/dyn" | ||
| "github.com/databricks/cli/libs/vfs" | ||
| ) | ||
| type syncInferRoot struct{} | ||
| // SyncInferRoot is a mutator that infers the root path of all files to synchronize by looking at the | ||
| // paths in the sync configuration. The sync root may be different from the bundle root | ||
| // when the user intends to synchronize files outside the bundle root. | ||
| // | ||
| // The sync root can be equivalent to or an ancestor of the bundle root, but not a descendant. | ||
| // That is, the sync root must contain the bundle root. | ||
| // | ||
| // This mutator requires all sync-related paths and patterns to be relative to the bundle root path. | ||
| // This is done by the [RewriteSyncPaths] mutator, which must run before this mutator. | ||
| func SyncInferRoot() bundle.Mutator { | ||
| return &syncInferRoot{} | ||
| } | ||
| func (m *syncInferRoot) Name() string { | ||
| return "SyncInferRoot" | ||
| } | ||
| // computeRoot finds the innermost path that contains the specified path. | ||
| // It traverses up the root path until it finds the innermost path. | ||
| // If the path does not exist, it returns an empty string. | ||
| // | ||
| // See "sync_infer_root_internal_test.go" for examples. | ||
| func (m *syncInferRoot) computeRoot(path string, root string) string { | ||
Contributor 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. If I understood correctly it finds a path to which both Contributor 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. If so, would it be simpler to make both paths absolute, find a common prefix path and then make it relative again? ContributorAuthor 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. Yes, that's correct. It finds the longest prefix of Your suggestion might work, but it will break if Separately, we still allow the bundle root path to be relative or absolute and we don't force it to be absolute anywhere. We could consider doing this but we'd need to pay attention to what happens on the presentation side of errors and warnings. It is possible we'd all of a sudden show full paths where today we show just | ||
| for !filepath.IsLocal(path) { | ||
| // Break if we have reached the root of the filesystem. | ||
| dir := filepath.Dir(root) | ||
| if dir == root { | ||
| return "" | ||
| } | ||
| // Update the sync path as we navigate up the directory tree. | ||
| path = filepath.Join(filepath.Base(root), path) | ||
| // Move up the directory tree. | ||
| root = dir | ||
| } | ||
| return filepath.Clean(root) | ||
| } | ||
| func (m *syncInferRoot) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
| var diags diag.Diagnostics | ||
| // Use the bundle root path as the starting point for inferring the sync root path. | ||
| bundleRootPath := filepath.Clean(b.RootPath) | ||
| // Infer the sync root path by looking at each one of the sync paths. | ||
| // Every sync path must be a descendant of the final sync root path. | ||
| syncRootPath := bundleRootPath | ||
| for _, path := range b.Config.Sync.Paths { | ||
| computedPath := m.computeRoot(path, bundleRootPath) | ||
| if computedPath == "" { | ||
| continue | ||
| } | ||
| // Update sync root path if the computed root path is an ancestor of the current sync root path. | ||
| if len(computedPath) < len(syncRootPath) { | ||
| syncRootPath = computedPath | ||
| } | ||
| } | ||
| // The new sync root path can only be an ancestor of the previous root path. | ||
| // Compute the relative path from the sync root to the bundle root. | ||
| rel, err := filepath.Rel(syncRootPath, bundleRootPath) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| // If during computation of the sync root path we hit the root of the filesystem, | ||
| // then one or more of the sync paths are outside the filesystem. | ||
| // Check if this happened by verifying that none of the paths escape the root | ||
| // when joined with the sync root path. | ||
| for i, path := range b.Config.Sync.Paths { | ||
| if filepath.IsLocal(filepath.Join(rel, path)) { | ||
| continue | ||
| } | ||
| diags = append(diags, diag.Diagnostic{ | ||
| Severity: diag.Error, | ||
| Summary: fmt.Sprintf("invalid sync path %q", path), | ||
| Locations: b.Config.GetLocations(fmt.Sprintf("sync.paths[%d]", i)), | ||
| Paths: []dyn.Path{dyn.NewPath(dyn.Key("sync"), dyn.Key("paths"), dyn.Index(i))}, | ||
| }) | ||
| } | ||
| if diags.HasError() { | ||
| return diags | ||
| } | ||
| // Update all paths in the sync configuration to be relative to the sync root. | ||
| for i, p := range b.Config.Sync.Paths { | ||
| b.Config.Sync.Paths[i] = filepath.Join(rel, p) | ||
| } | ||
| for i, p := range b.Config.Sync.Include { | ||
| b.Config.Sync.Include[i] = filepath.Join(rel, p) | ||
| } | ||
| for i, p := range b.Config.Sync.Exclude { | ||
| b.Config.Sync.Exclude[i] = filepath.Join(rel, p) | ||
| } | ||
| // Configure the sync root path. | ||
| b.SyncRoot = vfs.MustNew(syncRootPath) | ||
| b.SyncRootPath = syncRootPath | ||
| return nil | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
SyncRootPath is the remote path were files will be synced to, correct? The naming seems to be a bit confusing when I read the rest of the code
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.
I added a comment to clarify; this is the local path to the sync root (the same as
SyncRoot.Native()).