From 31db9239abfddc43f6deaec82d3c48e95ab80698 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 27 Nov 2024 14:29:18 +0100 Subject: [PATCH 01/25] Properly read git metadata when inside Workspace Since there is no .git directory in Workspace file system, we need to make an API call to fetch git checkout status (root of the repo, current branch, etc). (api/2.0/workspace/get-status?return_git_info=true). Refactor Repository to accept repository root rather than calculate it. This helps, because Repository is currently created in multiple places and finding the repository root is expensive. --- bundle/bundle.go | 3 + bundle/bundle_read_only.go | 4 + bundle/config/mutator/load_git_details.go | 37 ++---- bundle/config/validate/files_to_sync_test.go | 1 + bundle/deploy/files/sync.go | 9 +- cmd/sync/sync.go | 22 +++- libs/git/fileset.go | 10 +- libs/git/fileset_test.go | 8 +- libs/git/info.go | 124 +++++++++++++++++++ libs/git/repository.go | 14 +-- libs/git/view.go | 8 +- libs/git/view_test.go | 30 ++--- libs/sync/snapshot_test.go | 12 +- libs/sync/sync.go | 11 +- libs/sync/sync_test.go | 6 +- 15 files changed, 216 insertions(+), 83 deletions(-) create mode 100644 libs/git/info.go diff --git a/bundle/bundle.go b/bundle/bundle.go index 46710538a8..9de6dc88d5 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -48,6 +48,9 @@ type Bundle struct { // Exclusively use this field for filesystem operations. SyncRoot vfs.Path + // Path to root of git worktree + WorktreeRoot vfs.Path + // Config contains the bundle configuration. // It is loaded from the bundle configuration files and mutators may update it. Config config.Root diff --git a/bundle/bundle_read_only.go b/bundle/bundle_read_only.go index ceab95c0b4..4bdd94e598 100644 --- a/bundle/bundle_read_only.go +++ b/bundle/bundle_read_only.go @@ -32,6 +32,10 @@ func (r ReadOnlyBundle) SyncRoot() vfs.Path { return r.b.SyncRoot } +func (r ReadOnlyBundle) WorktreeRoot() vfs.Path { + return r.b.WorktreeRoot +} + func (r ReadOnlyBundle) WorkspaceClient() *databricks.WorkspaceClient { return r.b.WorkspaceClient() } diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 77558d9b50..dfb2a02ad7 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -7,7 +7,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/git" - "github.com/databricks/cli/libs/log" ) type loadGitDetails struct{} @@ -21,45 +20,35 @@ func (m *loadGitDetails) Name() string { } func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Load relevant git repository - repo, err := git.NewRepository(b.BundleRoot) + info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot, b.WorkspaceClient()) if err != nil { return diag.FromErr(err) } - // Read branch name of current checkout - branch, err := repo.CurrentBranch() - if err == nil { - b.Config.Bundle.Git.ActualBranch = branch - if b.Config.Bundle.Git.Branch == "" { - // Only load branch if there's no user defined value - b.Config.Bundle.Git.Inferred = true - b.Config.Bundle.Git.Branch = branch - } - } else { - log.Warnf(ctx, "failed to load current branch: %s", err) + b.WorktreeRoot = info.WorktreeRoot + + b.Config.Bundle.Git.ActualBranch = info.CurrentBranch + if b.Config.Bundle.Git.Branch == "" { + // Only load branch if there's no user defined value + b.Config.Bundle.Git.Inferred = true + b.Config.Bundle.Git.Branch = info.CurrentBranch } // load commit hash if undefined if b.Config.Bundle.Git.Commit == "" { - commit, err := repo.LatestCommit() - if err != nil { - log.Warnf(ctx, "failed to load latest commit: %s", err) - } else { - b.Config.Bundle.Git.Commit = commit - } + b.Config.Bundle.Git.Commit = info.LatestCommit } + // load origin url if undefined if b.Config.Bundle.Git.OriginURL == "" { - remoteUrl := repo.OriginUrl() - b.Config.Bundle.Git.OriginURL = remoteUrl + b.Config.Bundle.Git.OriginURL = info.OriginURL } - // repo.Root() returns the absolute path of the repo - relBundlePath, err := filepath.Rel(repo.Root(), b.BundleRoot.Native()) + relBundlePath, err := filepath.Rel(info.WorktreeRoot.Native(), b.BundleRoot.Native()) if err != nil { return diag.FromErr(err) } + b.Config.Bundle.Git.BundleRootPath = filepath.ToSlash(relBundlePath) return nil } diff --git a/bundle/config/validate/files_to_sync_test.go b/bundle/config/validate/files_to_sync_test.go index 2a598fa723..30af9026d1 100644 --- a/bundle/config/validate/files_to_sync_test.go +++ b/bundle/config/validate/files_to_sync_test.go @@ -44,6 +44,7 @@ func setupBundleForFilesToSyncTest(t *testing.T) *bundle.Bundle { BundleRoot: vfs.MustNew(dir), SyncRootPath: dir, SyncRoot: vfs.MustNew(dir), + WorktreeRoot: vfs.MustNew(dir), Config: config.Root{ Bundle: config.Bundle{ Target: "default", diff --git a/bundle/deploy/files/sync.go b/bundle/deploy/files/sync.go index 347ed30793..e3abc5fefa 100644 --- a/bundle/deploy/files/sync.go +++ b/bundle/deploy/files/sync.go @@ -28,10 +28,11 @@ func GetSyncOptions(ctx context.Context, rb bundle.ReadOnlyBundle) (*sync.SyncOp } opts := &sync.SyncOptions{ - LocalRoot: rb.SyncRoot(), - Paths: rb.Config().Sync.Paths, - Include: includes, - Exclude: rb.Config().Sync.Exclude, + WorktreeRoot: rb.WorktreeRoot(), + LocalRoot: rb.SyncRoot(), + Paths: rb.Config().Sync.Paths, + Include: includes, + Exclude: rb.Config().Sync.Exclude, RemotePath: rb.Config().Workspace.FilePath, Host: rb.WorkspaceClient().Config.Host, diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index 2092d9e330..307e0b34ad 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -12,6 +12,7 @@ import ( "github.com/databricks/cli/bundle/deploy/files" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/flags" + "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/sync" "github.com/databricks/cli/libs/vfs" "github.com/spf13/cobra" @@ -37,6 +38,7 @@ func (f *syncFlags) syncOptionsFromBundle(cmd *cobra.Command, args []string, b * opts.Full = f.full opts.PollInterval = f.interval + opts.WorktreeRoot = b.WorktreeRoot return opts, nil } @@ -60,11 +62,21 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn } } + ctx := cmd.Context() + client := root.WorkspaceClient(ctx) + + localRoot := vfs.MustNew(args[0]) + info, err := git.FetchRepositoryInfo(ctx, localRoot, client) + if err != nil { + return nil, err + } + opts := sync.SyncOptions{ - LocalRoot: vfs.MustNew(args[0]), - Paths: []string{"."}, - Include: nil, - Exclude: nil, + WorktreeRoot: info.WorktreeRoot, + LocalRoot: localRoot, + Paths: []string{"."}, + Include: nil, + Exclude: nil, RemotePath: args[1], Full: f.full, @@ -75,7 +87,7 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn // The sync code will automatically create this directory if it doesn't // exist and add it to the `.gitignore` file in the root. SnapshotBasePath: filepath.Join(args[0], ".databricks"), - WorkspaceClient: root.WorkspaceClient(cmd.Context()), + WorkspaceClient: client, OutputHandler: outputHandler, } diff --git a/libs/git/fileset.go b/libs/git/fileset.go index bb1cd4692f..c2802ee262 100644 --- a/libs/git/fileset.go +++ b/libs/git/fileset.go @@ -13,10 +13,10 @@ type FileSet struct { view *View } -// NewFileSet returns [FileSet] for the Git repository located at `root`. -func NewFileSet(root vfs.Path, paths ...[]string) (*FileSet, error) { +// NewFileSet returns [FileSet] for the directory `root` which is contained within Git repository located at `worktreeRoot`. +func NewFileSet(worktreeRoot, root vfs.Path, paths ...[]string) (*FileSet, error) { fs := fileset.New(root, paths...) - v, err := NewView(root) + v, err := NewView(worktreeRoot, root) if err != nil { return nil, err } @@ -27,6 +27,10 @@ func NewFileSet(root vfs.Path, paths ...[]string) (*FileSet, error) { }, nil } +func NewFileSetAtRoot(root vfs.Path, paths ...[]string) (*FileSet, error) { + return NewFileSet(root, root, paths...) +} + func (f *FileSet) IgnoreFile(file string) (bool, error) { return f.view.IgnoreFile(file) } diff --git a/libs/git/fileset_test.go b/libs/git/fileset_test.go index 37f3611d1f..d8ee6b8592 100644 --- a/libs/git/fileset_test.go +++ b/libs/git/fileset_test.go @@ -13,7 +13,7 @@ import ( ) func testFileSetAll(t *testing.T, root string) { - fileSet, err := NewFileSet(vfs.MustNew(root)) + fileSet, err := NewFileSetAtRoot(vfs.MustNew(root)) require.NoError(t, err) files, err := fileSet.Files() require.NoError(t, err) @@ -35,7 +35,7 @@ func TestFileSetNonCleanRoot(t *testing.T) { // Test what happens if the root directory can be simplified. // Path simplification is done by most filepath functions. // This should yield the same result as above test. - fileSet, err := NewFileSet(vfs.MustNew("./testdata/../testdata")) + fileSet, err := NewFileSetAtRoot(vfs.MustNew("./testdata/../testdata")) require.NoError(t, err) files, err := fileSet.Files() require.NoError(t, err) @@ -44,7 +44,7 @@ func TestFileSetNonCleanRoot(t *testing.T) { func TestFileSetAddsCacheDirToGitIgnore(t *testing.T) { projectDir := t.TempDir() - fileSet, err := NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) fileSet.EnsureValidGitIgnoreExists() @@ -59,7 +59,7 @@ func TestFileSetDoesNotCacheDirToGitIgnoreIfAlreadyPresent(t *testing.T) { projectDir := t.TempDir() gitIgnorePath := filepath.Join(projectDir, ".gitignore") - fileSet, err := NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) err = os.WriteFile(gitIgnorePath, []byte(".databricks"), 0o644) require.NoError(t, err) diff --git a/libs/git/info.go b/libs/git/info.go new file mode 100644 index 0000000000..9b533ba309 --- /dev/null +++ b/libs/git/info.go @@ -0,0 +1,124 @@ +package git + +import ( + "context" + "errors" + "io/fs" + "net/http" + "strings" + + "github.com/databricks/cli/libs/dbr" + "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/vfs" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/client" +) + +type GitRepositoryInfo struct { + OriginURL string + LatestCommit string + CurrentBranch string + WorktreeRoot vfs.Path +} + +type gitInfo struct { + Branch string `json:"branch"` + HeadCommitID string `json:"head_commit_id"` + Path string `json:"path"` + URL string `json:"url"` +} + +type response struct { + GitInfo *gitInfo `json:"git_info,omitempty"` +} + +func FetchRepositoryInfo(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { + if strings.HasPrefix(path.Native(), "/Workspace/") && dbr.RunsOnRuntime(ctx) { + return FetchRepositoryInfoAPI(ctx, path, w) + } else { + return FetchRepositoryInfoDotGit(ctx, path) + } +} + +func FetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { + apiClient, err := client.New(w.Config) + if err != nil { + return GitRepositoryInfo{}, err + } + + var response response + const apiEndpoint = "/api/2.0/workspace/get-status" + + err = apiClient.Do( + ctx, + http.MethodGet, + apiEndpoint, + nil, + map[string]string{ + "path": path.Native(), + "return_git_info": "true", + }, + &response, + ) + + if err != nil { + return GitRepositoryInfo{}, err + } + + // Check if GitInfo is present and extract relevant fields + gi := response.GitInfo + if gi == nil { + log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) + } else { + fixedPath := fixResponsePath(gi.Path) + return GitRepositoryInfo{ + OriginURL: gi.URL, + LatestCommit: gi.HeadCommitID, + CurrentBranch: gi.Branch, + WorktreeRoot: vfs.MustNew(fixedPath), + }, nil + } + + return GitRepositoryInfo{ + WorktreeRoot: path, + }, nil +} + +func fixResponsePath(path string) string { + if !strings.HasPrefix(path, "/Workspace/") { + return "/Workspace/" + path + } + return path +} + +func FetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { + rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) + if err != nil { + if !errors.Is(err, fs.ErrNotExist) { + return GitRepositoryInfo{}, err + } + rootDir = path + } + + repo, err := NewRepository(rootDir) + if err != nil { + return GitRepositoryInfo{}, err + } + + branch, err := repo.CurrentBranch() + if err != nil { + return GitRepositoryInfo{}, nil + } + + commit, err := repo.LatestCommit() + if err != nil { + return GitRepositoryInfo{}, nil + } + + return GitRepositoryInfo{ + OriginURL: repo.OriginUrl(), + LatestCommit: commit, + CurrentBranch: branch, + WorktreeRoot: rootDir, + }, nil +} diff --git a/libs/git/repository.go b/libs/git/repository.go index f0e9e1eb23..b94297ab98 100644 --- a/libs/git/repository.go +++ b/libs/git/repository.go @@ -1,9 +1,7 @@ package git import ( - "errors" "fmt" - "io/fs" "net/url" "path" "path/filepath" @@ -204,17 +202,7 @@ func (r *Repository) Ignore(relPath string) (bool, error) { return false, nil } -func NewRepository(path vfs.Path) (*Repository, error) { - rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) - if err != nil { - if !errors.Is(err, fs.ErrNotExist) { - return nil, err - } - // Cannot find `.git` directory. - // Treat the specified path as a potential repository root checkout. - rootDir = path - } - +func NewRepository(rootDir vfs.Path) (*Repository, error) { // Derive $GIT_DIR and $GIT_COMMON_DIR paths if this is a real repository. // If it isn't a real repository, they'll point to the (non-existent) `.git` directory. gitDir, gitCommonDir, err := resolveGitDirs(rootDir) diff --git a/libs/git/view.go b/libs/git/view.go index 2d2e39a60b..2eaba1f8b4 100644 --- a/libs/git/view.go +++ b/libs/git/view.go @@ -72,8 +72,8 @@ func (v *View) IgnoreDirectory(dir string) (bool, error) { return v.Ignore(dir + "/") } -func NewView(root vfs.Path) (*View, error) { - repo, err := NewRepository(root) +func NewView(worktreeRoot, root vfs.Path) (*View, error) { + repo, err := NewRepository(worktreeRoot) if err != nil { return nil, err } @@ -96,6 +96,10 @@ func NewView(root vfs.Path) (*View, error) { }, nil } +func NewViewAtRoot(root vfs.Path) (*View, error) { + return NewView(root, root) +} + func (v *View) EnsureValidGitIgnoreExists() error { ign, err := v.IgnoreDirectory(".databricks") if err != nil { diff --git a/libs/git/view_test.go b/libs/git/view_test.go index 76fba34584..06f6f94194 100644 --- a/libs/git/view_test.go +++ b/libs/git/view_test.go @@ -90,19 +90,19 @@ func testViewAtRoot(t *testing.T, tv testView) { } func TestViewRootInBricksRepo(t *testing.T) { - v, err := NewView(vfs.MustNew("./testdata")) + v, err := NewViewAtRoot(vfs.MustNew("./testdata")) require.NoError(t, err) testViewAtRoot(t, testView{t, v}) } func TestViewRootInTempRepo(t *testing.T) { - v, err := NewView(vfs.MustNew(createFakeRepo(t, "testdata"))) + v, err := NewViewAtRoot(vfs.MustNew(createFakeRepo(t, "testdata"))) require.NoError(t, err) testViewAtRoot(t, testView{t, v}) } func TestViewRootInTempDir(t *testing.T) { - v, err := NewView(vfs.MustNew(copyTestdata(t, "testdata"))) + v, err := NewViewAtRoot(vfs.MustNew(copyTestdata(t, "testdata"))) require.NoError(t, err) testViewAtRoot(t, testView{t, v}) } @@ -125,20 +125,21 @@ func testViewAtA(t *testing.T, tv testView) { } func TestViewAInBricksRepo(t *testing.T) { - v, err := NewView(vfs.MustNew("./testdata/a")) + v, err := NewView(vfs.MustNew("."), vfs.MustNew("./testdata/a")) require.NoError(t, err) testViewAtA(t, testView{t, v}) } func TestViewAInTempRepo(t *testing.T) { - v, err := NewView(vfs.MustNew(filepath.Join(createFakeRepo(t, "testdata"), "a"))) + repo := createFakeRepo(t, "testdata") + v, err := NewView(vfs.MustNew(repo), vfs.MustNew(filepath.Join(repo, "a"))) require.NoError(t, err) testViewAtA(t, testView{t, v}) } func TestViewAInTempDir(t *testing.T) { // Since this is not a fake repo it should not traverse up the tree. - v, err := NewView(vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a"))) + v, err := NewViewAtRoot(vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a"))) require.NoError(t, err) tv := testView{t, v} @@ -175,20 +176,21 @@ func testViewAtAB(t *testing.T, tv testView) { } func TestViewABInBricksRepo(t *testing.T) { - v, err := NewView(vfs.MustNew("./testdata/a/b")) + v, err := NewView(vfs.MustNew("."), vfs.MustNew("./testdata/a/b")) require.NoError(t, err) testViewAtAB(t, testView{t, v}) } func TestViewABInTempRepo(t *testing.T) { - v, err := NewView(vfs.MustNew(filepath.Join(createFakeRepo(t, "testdata"), "a", "b"))) + repo := createFakeRepo(t, "testdata") + v, err := NewView(vfs.MustNew(repo), vfs.MustNew(filepath.Join(repo, "a", "b"))) require.NoError(t, err) testViewAtAB(t, testView{t, v}) } func TestViewABInTempDir(t *testing.T) { // Since this is not a fake repo it should not traverse up the tree. - v, err := NewView(vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a", "b"))) + v, err := NewViewAtRoot(vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a", "b"))) tv := testView{t, v} require.NoError(t, err) @@ -215,7 +217,7 @@ func TestViewDoesNotChangeGitignoreIfCacheDirAlreadyIgnoredAtRoot(t *testing.T) // Since root .gitignore already has .databricks, there should be no edits // to root .gitignore - v, err := NewView(vfs.MustNew(repoPath)) + v, err := NewViewAtRoot(vfs.MustNew(repoPath)) require.NoError(t, err) err = v.EnsureValidGitIgnoreExists() @@ -235,7 +237,7 @@ func TestViewDoesNotChangeGitignoreIfCacheDirAlreadyIgnoredInSubdir(t *testing.T // Since root .gitignore already has .databricks, there should be no edits // to a/.gitignore - v, err := NewView(vfs.MustNew(filepath.Join(repoPath, "a"))) + v, err := NewView(vfs.MustNew(repoPath), vfs.MustNew(filepath.Join(repoPath, "a"))) require.NoError(t, err) err = v.EnsureValidGitIgnoreExists() @@ -253,7 +255,7 @@ func TestViewAddsGitignoreWithCacheDir(t *testing.T) { assert.NoError(t, err) // Since root .gitignore was deleted, new view adds .databricks to root .gitignore - v, err := NewView(vfs.MustNew(repoPath)) + v, err := NewViewAtRoot(vfs.MustNew(repoPath)) require.NoError(t, err) err = v.EnsureValidGitIgnoreExists() @@ -271,7 +273,7 @@ func TestViewAddsGitignoreWithCacheDirAtSubdir(t *testing.T) { require.NoError(t, err) // Since root .gitignore was deleted, new view adds .databricks to a/.gitignore - v, err := NewView(vfs.MustNew(filepath.Join(repoPath, "a"))) + v, err := NewView(vfs.MustNew(repoPath), vfs.MustNew(filepath.Join(repoPath, "a"))) require.NoError(t, err) err = v.EnsureValidGitIgnoreExists() @@ -288,7 +290,7 @@ func TestViewAddsGitignoreWithCacheDirAtSubdir(t *testing.T) { func TestViewAlwaysIgnoresCacheDir(t *testing.T) { repoPath := createFakeRepo(t, "testdata") - v, err := NewView(vfs.MustNew(repoPath)) + v, err := NewViewAtRoot(vfs.MustNew(repoPath)) require.NoError(t, err) err = v.EnsureValidGitIgnoreExists() diff --git a/libs/sync/snapshot_test.go b/libs/sync/snapshot_test.go index b7830406de..eef526e583 100644 --- a/libs/sync/snapshot_test.go +++ b/libs/sync/snapshot_test.go @@ -30,7 +30,7 @@ func TestDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -94,7 +94,7 @@ func TestSymlinkDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -125,7 +125,7 @@ func TestFolderDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -170,7 +170,7 @@ func TestPythonNotebookDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -245,7 +245,7 @@ func TestErrorWhenIdenticalRemoteName(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -282,7 +282,7 @@ func TestNoErrorRenameWithIdenticalRemoteName(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSet(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ diff --git a/libs/sync/sync.go b/libs/sync/sync.go index cc9c739446..6bd26f2241 100644 --- a/libs/sync/sync.go +++ b/libs/sync/sync.go @@ -19,10 +19,11 @@ import ( type OutputHandler func(context.Context, <-chan Event) type SyncOptions struct { - LocalRoot vfs.Path - Paths []string - Include []string - Exclude []string + WorktreeRoot vfs.Path + LocalRoot vfs.Path + Paths []string + Include []string + Exclude []string RemotePath string @@ -62,7 +63,7 @@ type Sync struct { // New initializes and returns a new [Sync] instance. func New(ctx context.Context, opts SyncOptions) (*Sync, error) { - fileSet, err := git.NewFileSet(opts.LocalRoot, opts.Paths) + fileSet, err := git.NewFileSet(opts.WorktreeRoot, opts.LocalRoot, opts.Paths) if err != nil { return nil, err } diff --git a/libs/sync/sync_test.go b/libs/sync/sync_test.go index 2d800f4668..6168dc2172 100644 --- a/libs/sync/sync_test.go +++ b/libs/sync/sync_test.go @@ -37,7 +37,7 @@ func TestGetFileSet(t *testing.T) { dir := setupFiles(t) root := vfs.MustNew(dir) - fileSet, err := git.NewFileSet(root) + fileSet, err := git.NewFileSetAtRoot(root) require.NoError(t, err) err = fileSet.EnsureValidGitIgnoreExists() @@ -103,7 +103,7 @@ func TestRecursiveExclude(t *testing.T) { dir := setupFiles(t) root := vfs.MustNew(dir) - fileSet, err := git.NewFileSet(root) + fileSet, err := git.NewFileSetAtRoot(root) require.NoError(t, err) err = fileSet.EnsureValidGitIgnoreExists() @@ -133,7 +133,7 @@ func TestNegateExclude(t *testing.T) { dir := setupFiles(t) root := vfs.MustNew(dir) - fileSet, err := git.NewFileSet(root) + fileSet, err := git.NewFileSetAtRoot(root) require.NoError(t, err) err = fileSet.EnsureValidGitIgnoreExists() From edac9601836d02848287c7f2de685713a1d1bd52 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 11:30:43 +0100 Subject: [PATCH 02/25] Update bundle/bundle.go Co-authored-by: Andrew Nester --- bundle/bundle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 9de6dc88d5..18cbba7252 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -48,7 +48,7 @@ type Bundle struct { // Exclusively use this field for filesystem operations. SyncRoot vfs.Path - // Path to root of git worktree + // Path to the root of git worktree WorktreeRoot vfs.Path // Config contains the bundle configuration. From 4fa99d0aeb747a6f1a28fad7feaf1e70e7b5748a Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 11:36:21 +0100 Subject: [PATCH 03/25] update comment --- libs/git/fileset.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/git/fileset.go b/libs/git/fileset.go index c2802ee262..8391548c9c 100644 --- a/libs/git/fileset.go +++ b/libs/git/fileset.go @@ -13,7 +13,7 @@ type FileSet struct { view *View } -// NewFileSet returns [FileSet] for the directory `root` which is contained within Git repository located at `worktreeRoot`. +// NewFileSet returns [FileSet] for the directory `root` which is contained within Git worktree located at `worktreeRoot`. func NewFileSet(worktreeRoot, root vfs.Path, paths ...[]string) (*FileSet, error) { fs := fileset.New(root, paths...) v, err := NewView(worktreeRoot, root) From 0ba23142a29ec2b1c0f1e8480e3d49861b96ac74 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 15:00:26 +0100 Subject: [PATCH 04/25] add test for FetchRepositoryInfo --- internal/git_fetch_test.go | 76 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 internal/git_fetch_test.go diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go new file mode 100644 index 0000000000..cf3015adf1 --- /dev/null +++ b/internal/git_fetch_test.go @@ -0,0 +1,76 @@ +package internal + +import ( + "os/exec" + "path/filepath" + "testing" + + "github.com/databricks/cli/internal/acc" + "github.com/databricks/cli/libs/dbr" + "github.com/databricks/cli/libs/git" + "github.com/databricks/cli/libs/vfs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const examplesRepoUrl = "https://github.com/databricks/bundle-examples" +const examplesRepoProvider = "gitHub" + +func assertGitInfo(t *testing.T, info git.GitRepositoryInfo, expectedRoot string) { + assert.Equal(t, "main", info.CurrentBranch) + assert.NotEmpty(t, info.LatestCommit) + assert.Equal(t, examplesRepoUrl, info.OriginURL) + assert.Equal(t, expectedRoot, info.WorktreeRoot.Native()) +} + +func TestFetchRepositoryInfoAPI(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + me, err := wt.W.CurrentUser.Me(ctx) + require.NoError(t, err) + + targetPath := acc.RandomName("/Workspace/Users/" + me.UserName + "/testing-clone-bundle-examples-") + stdout, stderr := RequireSuccessfulRun(t, "repos", "create", examplesRepoUrl, examplesRepoProvider, "--path", targetPath) + defer RequireSuccessfulRun(t, "repos", "delete", targetPath) + + assert.Empty(t, stderr.String()) + assert.NotEmpty(t, stdout.String()) + ctx = dbr.MockRuntime(ctx, true) + + for _, inputPath := range []string{ + targetPath + "/knowledge_base/dashboard_nyc_taxi", + targetPath, + } { + t.Run(inputPath, func(t *testing.T) { + info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) + assert.NoError(t, err) + assertGitInfo(t, info, targetPath) + }) + } +} + +func TestFetchRepositoryInfoDotGit(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + + repo := cloneRepoLocally(t, examplesRepoUrl) + + for _, inputPath := range []string{ + repo + "/knowledge_base/dashboard_nyc_taxi", + repo, + } { + t.Run(inputPath, func(t *testing.T) { + info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) + assert.NoError(t, err) + assertGitInfo(t, info, repo) + }) + } +} + +func cloneRepoLocally(t *testing.T, repoUrl string) string { + tempDir := t.TempDir() + localRoot := filepath.Join(tempDir, "repo") + + cmd := exec.Command("git", "clone", "--depth=1", examplesRepoUrl, localRoot) + err := cmd.Run() + require.NoError(t, err) + return localRoot +} From b4251d7b00e8e5bf9c8fc3e367f75eb4726c7120 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 15:43:44 +0100 Subject: [PATCH 05/25] make methods private --- libs/git/info.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/git/info.go b/libs/git/info.go index 9b533ba309..36d35caa48 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -34,13 +34,13 @@ type response struct { func FetchRepositoryInfo(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { if strings.HasPrefix(path.Native(), "/Workspace/") && dbr.RunsOnRuntime(ctx) { - return FetchRepositoryInfoAPI(ctx, path, w) + return fetchRepositoryInfoAPI(ctx, path, w) } else { - return FetchRepositoryInfoDotGit(ctx, path) + return fetchRepositoryInfoDotGit(ctx, path) } } -func FetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { +func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { apiClient, err := client.New(w.Config) if err != nil { return GitRepositoryInfo{}, err @@ -91,7 +91,7 @@ func fixResponsePath(path string) string { return path } -func FetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { +func fetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) if err != nil { if !errors.Is(err, fs.ErrNotExist) { From e49f2f3110532dd4bf816bb0616248bda35ac674 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 15:44:35 +0100 Subject: [PATCH 06/25] rename fixResponsePath to ensureWorkspacePrefix --- libs/git/info.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/git/info.go b/libs/git/info.go index 36d35caa48..eb8525f8c4 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -70,7 +70,7 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo if gi == nil { log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) } else { - fixedPath := fixResponsePath(gi.Path) + fixedPath := ensureWorkspacePrefix(gi.Path) return GitRepositoryInfo{ OriginURL: gi.URL, LatestCommit: gi.HeadCommitID, @@ -84,7 +84,7 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo }, nil } -func fixResponsePath(path string) string { +func ensureWorkspacePrefix(path string) string { if !strings.HasPrefix(path, "/Workspace/") { return "/Workspace/" + path } From 97c8042fd66c75ed629aa413a5c52f7c854a3d88 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 15:49:28 +0100 Subject: [PATCH 07/25] reorder to improve readability --- libs/git/info.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/git/info.go b/libs/git/info.go index eb8525f8c4..f2e0d548dd 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -67,9 +67,7 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo // Check if GitInfo is present and extract relevant fields gi := response.GitInfo - if gi == nil { - log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) - } else { + if gi != nil { fixedPath := ensureWorkspacePrefix(gi.Path) return GitRepositoryInfo{ OriginURL: gi.URL, @@ -79,6 +77,7 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo }, nil } + log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) return GitRepositoryInfo{ WorktreeRoot: path, }, nil From 80d9b6ae17de1d22f2ec71ece5034096af22cc68 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 15:52:43 +0100 Subject: [PATCH 08/25] Expand comment on WorktreeRoot --- bundle/bundle.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 18cbba7252..b3ef917dc8 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -48,7 +48,10 @@ type Bundle struct { // Exclusively use this field for filesystem operations. SyncRoot vfs.Path - // Path to the root of git worktree + // Path to the root of git worktree containing the bundle. + // This is the same as git repository root if worktrees are not used, + // otherwise it's a descedant of git repository root. + // https://git-scm.com/docs/git-worktree WorktreeRoot vfs.Path // Config contains the bundle configuration. From 42502513bffc366a34e210139a0b8d887b6a6f73 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 16:04:12 +0100 Subject: [PATCH 09/25] use t.Cleanup instead of defer --- internal/git_fetch_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index cf3015adf1..a6496a2156 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -30,7 +30,9 @@ func TestFetchRepositoryInfoAPI(t *testing.T) { targetPath := acc.RandomName("/Workspace/Users/" + me.UserName + "/testing-clone-bundle-examples-") stdout, stderr := RequireSuccessfulRun(t, "repos", "create", examplesRepoUrl, examplesRepoProvider, "--path", targetPath) - defer RequireSuccessfulRun(t, "repos", "delete", targetPath) + t.Cleanup(func() { + RequireSuccessfulRun(t, "repos", "delete", targetPath) + }) assert.Empty(t, stderr.String()) assert.NotEmpty(t, stdout.String()) From 017836f1a7cd8f707320a9d05bf9710c71b559d0 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 16:58:03 +0100 Subject: [PATCH 10/25] prefix integration tests with TestAcc --- internal/git_fetch_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index a6496a2156..aca4f17958 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -23,7 +23,7 @@ func assertGitInfo(t *testing.T, info git.GitRepositoryInfo, expectedRoot string assert.Equal(t, expectedRoot, info.WorktreeRoot.Native()) } -func TestFetchRepositoryInfoAPI(t *testing.T) { +func TestAccFetchRepositoryInfoAPI(t *testing.T) { ctx, wt := acc.WorkspaceTest(t) me, err := wt.W.CurrentUser.Me(ctx) require.NoError(t, err) @@ -50,7 +50,7 @@ func TestFetchRepositoryInfoAPI(t *testing.T) { } } -func TestFetchRepositoryInfoDotGit(t *testing.T) { +func TestAccFetchRepositoryInfoDotGit(t *testing.T) { ctx, wt := acc.WorkspaceTest(t) repo := cloneRepoLocally(t, examplesRepoUrl) From 242b53a23dcf7f953652dd72ae29c3919065fe9c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 17:02:29 +0100 Subject: [PATCH 11/25] remove incorrect info from the comment --- bundle/bundle.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index b3ef917dc8..76c87c24cd 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -49,8 +49,6 @@ type Bundle struct { SyncRoot vfs.Path // Path to the root of git worktree containing the bundle. - // This is the same as git repository root if worktrees are not used, - // otherwise it's a descedant of git repository root. // https://git-scm.com/docs/git-worktree WorktreeRoot vfs.Path From d204a9380a442e421ec75a05529eab31f9c983a5 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 22:18:59 +0100 Subject: [PATCH 12/25] use path.Join to add /Workspace prefix --- libs/git/info.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libs/git/info.go b/libs/git/info.go index f2e0d548dd..41412c49e0 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -5,6 +5,7 @@ import ( "errors" "io/fs" "net/http" + "path" "strings" "github.com/databricks/cli/libs/dbr" @@ -83,11 +84,11 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo }, nil } -func ensureWorkspacePrefix(path string) string { - if !strings.HasPrefix(path, "/Workspace/") { - return "/Workspace/" + path +func ensureWorkspacePrefix(p string) string { + if !strings.HasPrefix(p, "/Workspace/") { + return path.Join("/Workspace", p) } - return path + return p } func fetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { From 8fe958f67bc5f5d42ad40b2b9b6a1a46ec61f908 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 2 Dec 2024 22:55:29 +0100 Subject: [PATCH 13/25] Do not fail on git operations in bundle and sync, log warnings instead; add more tests. - Change GitRepositoryInfo.WorktreeRoot to string - Restore original warnings logs for when branch and commit could not be read - Add new logs for when repository could not be read. However, if we can figure out git root then use it, even if parsing later fails. --- bundle/config/mutator/load_git_details.go | 16 ++-- cmd/sync/sync.go | 10 ++- internal/git_fetch_test.go | 105 ++++++++++++++++++++-- libs/diag/diagnostic.go | 13 +++ libs/git/info.go | 42 ++++----- 5 files changed, 153 insertions(+), 33 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index dfb2a02ad7..b69e849037 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/git" + "github.com/databricks/cli/libs/vfs" ) type loadGitDetails struct{} @@ -20,12 +21,17 @@ func (m *loadGitDetails) Name() string { } func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot, b.WorkspaceClient()) if err != nil { - return diag.FromErr(err) + diags = append(diags, diag.WarningFromErr(err)...) } - b.WorktreeRoot = info.WorktreeRoot + if info.WorktreeRoot == "" { + b.WorktreeRoot = b.BundleRoot + } else { + b.WorktreeRoot = vfs.MustNew(info.WorktreeRoot) + } b.Config.Bundle.Git.ActualBranch = info.CurrentBranch if b.Config.Bundle.Git.Branch == "" { @@ -44,11 +50,11 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn b.Config.Bundle.Git.OriginURL = info.OriginURL } - relBundlePath, err := filepath.Rel(info.WorktreeRoot.Native(), b.BundleRoot.Native()) + relBundlePath, err := filepath.Rel(b.WorktreeRoot.Native(), b.BundleRoot.Native()) if err != nil { - return diag.FromErr(err) + diags = append(diags, diag.FromErr(err)...) } b.Config.Bundle.Git.BundleRootPath = filepath.ToSlash(relBundlePath) - return nil + return diags } diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index 307e0b34ad..70c23031b4 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -13,6 +13,7 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/flags" "github.com/databricks/cli/libs/git" + "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/sync" "github.com/databricks/cli/libs/vfs" "github.com/spf13/cobra" @@ -67,12 +68,17 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn localRoot := vfs.MustNew(args[0]) info, err := git.FetchRepositoryInfo(ctx, localRoot, client) + if err != nil { - return nil, err + log.Warnf(ctx, "Failed to read git info: %s", err) + } + + if info.WorktreeRoot == "" { + info.WorktreeRoot = localRoot.Native() } opts := sync.SyncOptions{ - WorktreeRoot: info.WorktreeRoot, + WorktreeRoot: vfs.MustNew(info.WorktreeRoot), LocalRoot: localRoot, Paths: []string{"."}, Include: nil, diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index aca4f17958..ceba021634 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -1,6 +1,7 @@ package internal import ( + "os" "os/exec" "path/filepath" "testing" @@ -16,14 +17,14 @@ import ( const examplesRepoUrl = "https://github.com/databricks/bundle-examples" const examplesRepoProvider = "gitHub" -func assertGitInfo(t *testing.T, info git.GitRepositoryInfo, expectedRoot string) { +func assertFullGitInfo(t *testing.T, info git.GitRepositoryInfo, expectedRoot string) { assert.Equal(t, "main", info.CurrentBranch) assert.NotEmpty(t, info.LatestCommit) assert.Equal(t, examplesRepoUrl, info.OriginURL) - assert.Equal(t, expectedRoot, info.WorktreeRoot.Native()) + assert.Equal(t, expectedRoot, info.WorktreeRoot) } -func TestAccFetchRepositoryInfoAPI(t *testing.T) { +func TestAccFetchRepositoryInfoAPI_FromRepo(t *testing.T) { ctx, wt := acc.WorkspaceTest(t) me, err := wt.W.CurrentUser.Me(ctx) require.NoError(t, err) @@ -45,12 +46,62 @@ func TestAccFetchRepositoryInfoAPI(t *testing.T) { t.Run(inputPath, func(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) assert.NoError(t, err) - assertGitInfo(t, info, targetPath) + assertFullGitInfo(t, info, targetPath) }) } } -func TestAccFetchRepositoryInfoDotGit(t *testing.T) { +func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + me, err := wt.W.CurrentUser.Me(ctx) + require.NoError(t, err) + + rootPath := acc.RandomName("/Workspace/Users/" + me.UserName + "/testing-nonrepo-") + _, stderr := RequireSuccessfulRun(t, "workspace", "mkdirs", rootPath+"/a/b/c") + t.Cleanup(func() { + RequireSuccessfulRun(t, "workspace", "delete", "--recursive", rootPath) + }) + + assert.Empty(t, stderr.String()) + //assert.NotEmpty(t, stdout.String()) + ctx = dbr.MockRuntime(ctx, true) + + tests := []struct { + input string + msg string + }{ + { + input: rootPath + "/a/b/c", + msg: "", + }, + { + input: rootPath, + msg: "", + }, + { + input: rootPath + "/non-existent", + msg: "doesn't exist", + }, + } + + for _, test := range tests { + t.Run(test.input+" <==> "+test.msg, func(t *testing.T) { + info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(test.input), wt.W) + if test.msg == "" { + assert.NoError(t, err) + } else { + assert.Error(t, err) + assert.Contains(t, err.Error(), test.msg) + } + assert.Equal(t, "", info.CurrentBranch) + assert.Equal(t, "", info.LatestCommit) + assert.Equal(t, "", info.OriginURL) + assert.Equal(t, "", info.WorktreeRoot) + }) + } +} + +func TestAccFetchRepositoryInfoDotGit_FromGitRepo(t *testing.T) { ctx, wt := acc.WorkspaceTest(t) repo := cloneRepoLocally(t, examplesRepoUrl) @@ -62,7 +113,7 @@ func TestAccFetchRepositoryInfoDotGit(t *testing.T) { t.Run(inputPath, func(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) assert.NoError(t, err) - assertGitInfo(t, info, repo) + assertFullGitInfo(t, info, repo) }) } } @@ -76,3 +127,45 @@ func cloneRepoLocally(t *testing.T, repoUrl string) string { require.NoError(t, err) return localRoot } + +func TestAccFetchRepositoryInfoDotGit_FromNonGitRepo(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + + tempDir := t.TempDir() + root := filepath.Join(tempDir, "repo") + require.NoError(t, os.MkdirAll(root+"/a/b/c", 0700)) + + tests := []string{ + root + "/a/b/c", + root, + root + "/non-existent", + } + + for _, input := range tests { + t.Run(input, func(t *testing.T) { + info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(input), wt.W) + assert.NoError(t, err) + assert.Equal(t, "", info.CurrentBranch) + assert.Equal(t, "", info.LatestCommit) + assert.Equal(t, "", info.OriginURL) + assert.Equal(t, "", info.WorktreeRoot) + }) + } +} + +func TestAccFetchRepositoryInfoDotGit_FromBrokenGitRepo(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + + tempDir := t.TempDir() + root := filepath.Join(tempDir, "repo") + path := root + "/a/b/c" + require.NoError(t, os.MkdirAll(path, 0700)) + require.NoError(t, os.WriteFile(root+"/.git", []byte(""), 0000)) + + info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(path), wt.W) + assert.NoError(t, err) + assert.Equal(t, root, info.WorktreeRoot) + assert.Equal(t, "", info.CurrentBranch) + assert.Equal(t, "", info.LatestCommit) + assert.Equal(t, "", info.OriginURL) +} diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index 254ecbd7d3..a4f8c7b6b7 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -53,6 +53,19 @@ func FromErr(err error) Diagnostics { } } +// FromErr returns a new warning diagnostic from the specified error, if any. +func WarningFromErr(err error) Diagnostics { + if err == nil { + return nil + } + return []Diagnostic{ + { + Severity: Warning, + Summary: err.Error(), + }, + } +} + // Warningf creates a new warning diagnostic. func Warningf(format string, args ...any) Diagnostics { return []Diagnostic{ diff --git a/libs/git/info.go b/libs/git/info.go index 41412c49e0..16c1a6a0b5 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -19,7 +19,7 @@ type GitRepositoryInfo struct { OriginURL string LatestCommit string CurrentBranch string - WorktreeRoot vfs.Path + WorktreeRoot string } type gitInfo struct { @@ -74,14 +74,12 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo OriginURL: gi.URL, LatestCommit: gi.HeadCommitID, CurrentBranch: gi.Branch, - WorktreeRoot: vfs.MustNew(fixedPath), + WorktreeRoot: fixedPath, }, nil } log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) - return GitRepositoryInfo{ - WorktreeRoot: path, - }, nil + return GitRepositoryInfo{}, nil } func ensureWorkspacePrefix(p string) string { @@ -93,32 +91,36 @@ func ensureWorkspacePrefix(p string) string { func fetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) - if err != nil { - if !errors.Is(err, fs.ErrNotExist) { - return GitRepositoryInfo{}, err + if err != nil || rootDir == nil { + if errors.Is(err, fs.ErrNotExist) { + return GitRepositoryInfo{}, nil } - rootDir = path + return GitRepositoryInfo{}, err + } + + result := GitRepositoryInfo{ + WorktreeRoot: rootDir.Native(), } repo, err := NewRepository(rootDir) if err != nil { - return GitRepositoryInfo{}, err + log.Warnf(ctx, "failed to read .git: %s", err) + + // return early since operations below won't work + return result, nil } - branch, err := repo.CurrentBranch() + result.OriginURL = repo.OriginUrl() + + result.CurrentBranch, err = repo.CurrentBranch() if err != nil { - return GitRepositoryInfo{}, nil + log.Warnf(ctx, "failed to load current branch: %s", err) } - commit, err := repo.LatestCommit() + result.LatestCommit, err = repo.LatestCommit() if err != nil { - return GitRepositoryInfo{}, nil + log.Warnf(ctx, "failed to load latest commit: %s", err) } - return GitRepositoryInfo{ - OriginURL: repo.OriginUrl(), - LatestCommit: commit, - CurrentBranch: branch, - WorktreeRoot: rootDir, - }, nil + return result, nil } From 4d38475ff302d1ae31122133e30d7c3e0d651e7c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 3 Dec 2024 09:19:45 +0100 Subject: [PATCH 14/25] refactor test code a bit - extract helper assert groups --- internal/git_fetch_test.go | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index ceba021634..0623f5cc54 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -17,13 +17,24 @@ import ( const examplesRepoUrl = "https://github.com/databricks/bundle-examples" const examplesRepoProvider = "gitHub" -func assertFullGitInfo(t *testing.T, info git.GitRepositoryInfo, expectedRoot string) { +func assertFullGitInfo(t *testing.T, expectedRoot string, info git.GitRepositoryInfo) { assert.Equal(t, "main", info.CurrentBranch) assert.NotEmpty(t, info.LatestCommit) assert.Equal(t, examplesRepoUrl, info.OriginURL) assert.Equal(t, expectedRoot, info.WorktreeRoot) } +func assertEmptyGitInfo(t *testing.T, info git.GitRepositoryInfo) { + assertSparseGitInfo(t, "", info) +} + +func assertSparseGitInfo(t *testing.T, expectedRoot string, info git.GitRepositoryInfo) { + assert.Equal(t, "", info.CurrentBranch) + assert.Equal(t, "", info.LatestCommit) + assert.Equal(t, "", info.OriginURL) + assert.Equal(t, expectedRoot, info.WorktreeRoot) +} + func TestAccFetchRepositoryInfoAPI_FromRepo(t *testing.T) { ctx, wt := acc.WorkspaceTest(t) me, err := wt.W.CurrentUser.Me(ctx) @@ -46,7 +57,7 @@ func TestAccFetchRepositoryInfoAPI_FromRepo(t *testing.T) { t.Run(inputPath, func(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) assert.NoError(t, err) - assertFullGitInfo(t, info, targetPath) + assertFullGitInfo(t, targetPath, info) }) } } @@ -93,10 +104,7 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), test.msg) } - assert.Equal(t, "", info.CurrentBranch) - assert.Equal(t, "", info.LatestCommit) - assert.Equal(t, "", info.OriginURL) - assert.Equal(t, "", info.WorktreeRoot) + assertEmptyGitInfo(t, info) }) } } @@ -113,7 +121,7 @@ func TestAccFetchRepositoryInfoDotGit_FromGitRepo(t *testing.T) { t.Run(inputPath, func(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) assert.NoError(t, err) - assertFullGitInfo(t, info, repo) + assertFullGitInfo(t, repo, info) }) } } @@ -145,10 +153,7 @@ func TestAccFetchRepositoryInfoDotGit_FromNonGitRepo(t *testing.T) { t.Run(input, func(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(input), wt.W) assert.NoError(t, err) - assert.Equal(t, "", info.CurrentBranch) - assert.Equal(t, "", info.LatestCommit) - assert.Equal(t, "", info.OriginURL) - assert.Equal(t, "", info.WorktreeRoot) + assertEmptyGitInfo(t, info) }) } } @@ -164,8 +169,5 @@ func TestAccFetchRepositoryInfoDotGit_FromBrokenGitRepo(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(path), wt.W) assert.NoError(t, err) - assert.Equal(t, root, info.WorktreeRoot) - assert.Equal(t, "", info.CurrentBranch) - assert.Equal(t, "", info.LatestCommit) - assert.Equal(t, "", info.OriginURL) + assertSparseGitInfo(t, root, info) } From 8a31be78d4901d94248663df7c6828351e13f088 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 3 Dec 2024 09:35:26 +0100 Subject: [PATCH 15/25] Add GuessedWorktreeRoot field; this simplifies usage on the caller side; also add comment doc. --- bundle/config/mutator/load_git_details.go | 7 +-- cmd/sync/sync.go | 6 +-- libs/git/info.go | 55 ++++++++++++++++------- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index b69e849037..a325da5497 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -7,7 +7,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/git" - "github.com/databricks/cli/libs/vfs" ) type loadGitDetails struct{} @@ -27,11 +26,7 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn diags = append(diags, diag.WarningFromErr(err)...) } - if info.WorktreeRoot == "" { - b.WorktreeRoot = b.BundleRoot - } else { - b.WorktreeRoot = vfs.MustNew(info.WorktreeRoot) - } + b.WorktreeRoot = info.GuessedWorktreeRoot b.Config.Bundle.Git.ActualBranch = info.CurrentBranch if b.Config.Bundle.Git.Branch == "" { diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index 70c23031b4..220f0939f1 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -73,12 +73,8 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn log.Warnf(ctx, "Failed to read git info: %s", err) } - if info.WorktreeRoot == "" { - info.WorktreeRoot = localRoot.Native() - } - opts := sync.SyncOptions{ - WorktreeRoot: vfs.MustNew(info.WorktreeRoot), + WorktreeRoot: info.GuessedWorktreeRoot, LocalRoot: localRoot, Paths: []string{"."}, Include: nil, diff --git a/libs/git/info.go b/libs/git/info.go index 16c1a6a0b5..88398eb848 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -16,10 +16,16 @@ import ( ) type GitRepositoryInfo struct { + // Various metadata about the repo. Each could be "" if it could not be read. No error is returned for such case. OriginURL string LatestCommit string CurrentBranch string - WorktreeRoot string + + // Absolute path to determined worktree root or "" if worktree root could not be determined. + WorktreeRoot string + + // vfs.Path variant of WorktreeRoot if WorktreeRoot is set; otherwise defaults to input path. + GuessedWorktreeRoot vfs.Path } type gitInfo struct { @@ -33,6 +39,15 @@ type response struct { GitInfo *gitInfo `json:"git_info,omitempty"` } +// Fetch repository information either by quering .git or by fetching it from API (for dabs-in-workspace case). +// - In case we could not find git repository, all string fields of GitRepositoryInfo will be "" and err will be nil. +// - If there were any errors when trying to determine git root (e.g. API call returned an error or there were permission issues +// reading the file system), all strings fields of GitRepositoryInfo will be "" and err will be non-nil. +// - For convenience, GuessedWorktreeRoot parameter will be set to path in the above two cases. +// - If we could determine git worktree root but there were errors when reading metadata (origin, branch, commit), those errors will be logged +// as warnings, GitRepositoryInfo will have non-empty WorktreeRoot and corresponding GuessedWorktreeRoot. +// Other strings fields will be "" and err will be nil. +// - In successful case, all fields are set to proper git repository metadata. func FetchRepositoryInfo(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { if strings.HasPrefix(path.Native(), "/Workspace/") && dbr.RunsOnRuntime(ctx) { return fetchRepositoryInfoAPI(ctx, path, w) @@ -42,9 +57,14 @@ func FetchRepositoryInfo(ctx context.Context, path vfs.Path, w *databricks.Works } func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { + result := GitRepositoryInfo{ + // For convenience, this field defaults to input path, even if err is also set. + GuessedWorktreeRoot: path, + } + apiClient, err := client.New(w.Config) if err != nil { - return GitRepositoryInfo{}, err + return result, err } var response response @@ -63,23 +83,23 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo ) if err != nil { - return GitRepositoryInfo{}, err + return result, err } // Check if GitInfo is present and extract relevant fields gi := response.GitInfo if gi != nil { fixedPath := ensureWorkspacePrefix(gi.Path) - return GitRepositoryInfo{ - OriginURL: gi.URL, - LatestCommit: gi.HeadCommitID, - CurrentBranch: gi.Branch, - WorktreeRoot: fixedPath, - }, nil + result.OriginURL = gi.URL + result.LatestCommit = gi.HeadCommitID + result.CurrentBranch = gi.Branch + result.WorktreeRoot = fixedPath + result.GuessedWorktreeRoot = vfs.MustNew(fixedPath) + } else { + log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) } - log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) - return GitRepositoryInfo{}, nil + return result, nil } func ensureWorkspacePrefix(p string) string { @@ -90,17 +110,20 @@ func ensureWorkspacePrefix(p string) string { } func fetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { + result := GitRepositoryInfo{ + GuessedWorktreeRoot: path, + } + rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) if err != nil || rootDir == nil { if errors.Is(err, fs.ErrNotExist) { - return GitRepositoryInfo{}, nil + return result, nil } - return GitRepositoryInfo{}, err + return result, err } - result := GitRepositoryInfo{ - WorktreeRoot: rootDir.Native(), - } + result.WorktreeRoot = rootDir.Native() + result.GuessedWorktreeRoot = rootDir repo, err := NewRepository(rootDir) if err != nil { From e7a49a249daa582dce8f74b4e9be775c21d70d61 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 3 Dec 2024 09:42:09 +0100 Subject: [PATCH 16/25] clarify comment --- libs/git/info.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/git/info.go b/libs/git/info.go index 88398eb848..5df69f7ea0 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -44,9 +44,9 @@ type response struct { // - If there were any errors when trying to determine git root (e.g. API call returned an error or there were permission issues // reading the file system), all strings fields of GitRepositoryInfo will be "" and err will be non-nil. // - For convenience, GuessedWorktreeRoot parameter will be set to path in the above two cases. -// - If we could determine git worktree root but there were errors when reading metadata (origin, branch, commit), those errors will be logged -// as warnings, GitRepositoryInfo will have non-empty WorktreeRoot and corresponding GuessedWorktreeRoot. -// Other strings fields will be "" and err will be nil. +// - If we could determine git worktree root but there were errors when reading metadata (origin, branch, commit), those errors +// will be logged as warnings, GitRepositoryInfo is guaranteed to have non-empty WorktreeRoot and corresponding GuessedWorktreeRoot +// and other fields on best effort basis. The err will be nil. // - In successful case, all fields are set to proper git repository metadata. func FetchRepositoryInfo(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { if strings.HasPrefix(path.Native(), "/Workspace/") && dbr.RunsOnRuntime(ctx) { From c4c7a872a9f467934c9037b7c9df52f12796856f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 3 Dec 2024 09:52:53 +0100 Subject: [PATCH 17/25] expand FileSet tests with cases root != worktreeRoot --- libs/git/fileset_test.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/libs/git/fileset_test.go b/libs/git/fileset_test.go index d8ee6b8592..f4fd931fd1 100644 --- a/libs/git/fileset_test.go +++ b/libs/git/fileset_test.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/require" ) -func testFileSetAll(t *testing.T, root string) { - fileSet, err := NewFileSetAtRoot(vfs.MustNew(root)) +func testFileSetAll(t *testing.T, worktreeRoot, root string) { + fileSet, err := NewFileSet(vfs.MustNew(worktreeRoot), vfs.MustNew(root)) require.NoError(t, err) files, err := fileSet.Files() require.NoError(t, err) @@ -24,11 +24,21 @@ func testFileSetAll(t *testing.T, root string) { } func TestFileSetListAllInRepo(t *testing.T) { - testFileSetAll(t, "./testdata") + testFileSetAll(t, "./testdata", "./testdata") +} + +func TestFileSetListAllInRepoDifferentRoot(t *testing.T) { + testFileSetAll(t, ".", "./testdata") } func TestFileSetListAllInTempDir(t *testing.T) { - testFileSetAll(t, copyTestdata(t, "./testdata")) + dir := copyTestdata(t, "./testdata") + testFileSetAll(t, dir, dir) +} + +func TestFileSetListAllInTempDirDifferentRoot(t *testing.T) { + dir := copyTestdata(t, "./testdata") + testFileSetAll(t, filepath.Dir(dir), dir) } func TestFileSetNonCleanRoot(t *testing.T) { From a623cfdd23fe9de5ffe7327379a4c4c91c16f165 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 3 Dec 2024 12:46:42 +0100 Subject: [PATCH 18/25] Switch to string from vfs.Path in the signature It seems that because vfs.Path does filepath.Abs (OS-specific) transformation that mangles the path even when it's already in good state, e.g. /Workspace/Something and that causes the tests to fail on Windows. --- bundle/config/mutator/load_git_details.go | 2 +- cmd/sync/sync.go | 2 +- internal/git_fetch_test.go | 11 +++-- libs/git/info.go | 55 ++++++++++++++++------- 4 files changed, 47 insertions(+), 23 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index a325da5497..45e88f6ef7 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -21,7 +21,7 @@ func (m *loadGitDetails) Name() string { func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics - info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot, b.WorkspaceClient()) + info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot.Native(), b.WorkspaceClient()) if err != nil { diags = append(diags, diag.WarningFromErr(err)...) } diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index 220f0939f1..e6d529101a 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -67,7 +67,7 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn client := root.WorkspaceClient(ctx) localRoot := vfs.MustNew(args[0]) - info, err := git.FetchRepositoryInfo(ctx, localRoot, client) + info, err := git.FetchRepositoryInfo(ctx, localRoot.Native(), client) if err != nil { log.Warnf(ctx, "Failed to read git info: %s", err) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index 0623f5cc54..2088d23e70 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/internal/acc" "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/git" - "github.com/databricks/cli/libs/vfs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -55,7 +54,7 @@ func TestAccFetchRepositoryInfoAPI_FromRepo(t *testing.T) { targetPath, } { t.Run(inputPath, func(t *testing.T) { - info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) + info, err := git.FetchRepositoryInfo(ctx, inputPath, wt.W) assert.NoError(t, err) assertFullGitInfo(t, targetPath, info) }) @@ -97,7 +96,7 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { for _, test := range tests { t.Run(test.input+" <==> "+test.msg, func(t *testing.T) { - info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(test.input), wt.W) + info, err := git.FetchRepositoryInfo(ctx, test.input, wt.W) if test.msg == "" { assert.NoError(t, err) } else { @@ -119,7 +118,7 @@ func TestAccFetchRepositoryInfoDotGit_FromGitRepo(t *testing.T) { repo, } { t.Run(inputPath, func(t *testing.T) { - info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(inputPath), wt.W) + info, err := git.FetchRepositoryInfo(ctx, inputPath, wt.W) assert.NoError(t, err) assertFullGitInfo(t, repo, info) }) @@ -151,7 +150,7 @@ func TestAccFetchRepositoryInfoDotGit_FromNonGitRepo(t *testing.T) { for _, input := range tests { t.Run(input, func(t *testing.T) { - info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(input), wt.W) + info, err := git.FetchRepositoryInfo(ctx, input, wt.W) assert.NoError(t, err) assertEmptyGitInfo(t, info) }) @@ -167,7 +166,7 @@ func TestAccFetchRepositoryInfoDotGit_FromBrokenGitRepo(t *testing.T) { require.NoError(t, os.MkdirAll(path, 0700)) require.NoError(t, os.WriteFile(root+"/.git", []byte(""), 0000)) - info, err := git.FetchRepositoryInfo(ctx, vfs.MustNew(path), wt.W) + info, err := git.FetchRepositoryInfo(ctx, path, wt.W) assert.NoError(t, err) assertSparseGitInfo(t, root, info) } diff --git a/libs/git/info.go b/libs/git/info.go index 5df69f7ea0..3b03835f4c 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -5,7 +5,9 @@ import ( "errors" "io/fs" "net/http" + "os" "path" + "path/filepath" "strings" "github.com/databricks/cli/libs/dbr" @@ -48,18 +50,18 @@ type response struct { // will be logged as warnings, GitRepositoryInfo is guaranteed to have non-empty WorktreeRoot and corresponding GuessedWorktreeRoot // and other fields on best effort basis. The err will be nil. // - In successful case, all fields are set to proper git repository metadata. -func FetchRepositoryInfo(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { - if strings.HasPrefix(path.Native(), "/Workspace/") && dbr.RunsOnRuntime(ctx) { +func FetchRepositoryInfo(ctx context.Context, path string, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { + if strings.HasPrefix(path, "/Workspace/") && dbr.RunsOnRuntime(ctx) { return fetchRepositoryInfoAPI(ctx, path, w) } else { return fetchRepositoryInfoDotGit(ctx, path) } } -func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { +func fetchRepositoryInfoAPI(ctx context.Context, path string, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { result := GitRepositoryInfo{ // For convenience, this field defaults to input path, even if err is also set. - GuessedWorktreeRoot: path, + GuessedWorktreeRoot: vfs.MustNew(path), } apiClient, err := client.New(w.Config) @@ -76,7 +78,7 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo apiEndpoint, nil, map[string]string{ - "path": path.Native(), + "path": path, "return_git_info": "true", }, &response, @@ -94,6 +96,7 @@ func fetchRepositoryInfoAPI(ctx context.Context, path vfs.Path, w *databricks.Wo result.LatestCommit = gi.HeadCommitID result.CurrentBranch = gi.Branch result.WorktreeRoot = fixedPath + // Note, this won't work on Windows since vfs.MustNew will call filepath.Abs result.GuessedWorktreeRoot = vfs.MustNew(fixedPath) } else { log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) @@ -109,23 +112,20 @@ func ensureWorkspacePrefix(p string) string { return p } -func fetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositoryInfo, error) { +func fetchRepositoryInfoDotGit(ctx context.Context, path string) (GitRepositoryInfo, error) { result := GitRepositoryInfo{ - GuessedWorktreeRoot: path, + GuessedWorktreeRoot: vfs.MustNew(path), } - rootDir, err := vfs.FindLeafInTree(path, GitDirectoryName) - if err != nil || rootDir == nil { - if errors.Is(err, fs.ErrNotExist) { - return result, nil - } + rootDir, err := findLeafInTree(path, GitDirectoryName) + if rootDir == "" { return result, err } - result.WorktreeRoot = rootDir.Native() - result.GuessedWorktreeRoot = rootDir + result.WorktreeRoot = rootDir + result.GuessedWorktreeRoot = vfs.MustNew(rootDir) - repo, err := NewRepository(rootDir) + repo, err := NewRepository(result.GuessedWorktreeRoot) if err != nil { log.Warnf(ctx, "failed to read .git: %s", err) @@ -147,3 +147,28 @@ func fetchRepositoryInfoDotGit(ctx context.Context, path vfs.Path) (GitRepositor return result, nil } + +func findLeafInTree(p string, leafName string) (string, error) { + var err error + for i := 0; i < 10000; i++ { + _, err = os.Stat(filepath.Join(p, leafName)) + + if err == nil { + // found .git in p + return p, nil + } + + // ErrNotExist means we continue traversal up the tree. + if errors.Is(err, fs.ErrNotExist) { + parent := filepath.Dir(p) + if parent == p { + return "", nil + } + p = parent + continue + } + break + } + + return "", err +} From d0b088af9ab3964c08a0cda77176f357854cf8be Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 12:13:25 +0100 Subject: [PATCH 19/25] s/.git/[leafName] Co-authored-by: Pieter Noordhuis --- libs/git/info.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/git/info.go b/libs/git/info.go index 3b03835f4c..ac583c1dd8 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -154,7 +154,7 @@ func findLeafInTree(p string, leafName string) (string, error) { _, err = os.Stat(filepath.Join(p, leafName)) if err == nil { - // found .git in p + // Found [leafName] in p return p, nil } From 5131488b16e1c95ae60bcb8854c228284da36d9c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 12:16:59 +0100 Subject: [PATCH 20/25] clean up comment --- internal/git_fetch_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index 2088d23e70..394c95a6ef 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -73,7 +73,6 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { }) assert.Empty(t, stderr.String()) - //assert.NotEmpty(t, stdout.String()) ctx = dbr.MockRuntime(ctx, true) tests := []struct { From 1c3f5f2aa13877df9e998b66418cd3f9dcd42497 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 12:31:18 +0100 Subject: [PATCH 21/25] clean up GuessedWorktreeRoot field; let the caller do the guessing --- bundle/config/mutator/load_git_details.go | 7 ++++++- cmd/sync/sync.go | 10 +++++++++- libs/git/info.go | 21 ++++----------------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 45e88f6ef7..8289a3b724 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/git" + "github.com/databricks/cli/libs/vfs" ) type loadGitDetails struct{} @@ -26,7 +27,11 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn diags = append(diags, diag.WarningFromErr(err)...) } - b.WorktreeRoot = info.GuessedWorktreeRoot + if info.WorktreeRoot == "" { + b.WorktreeRoot = b.BundleRoot + } else { + b.WorktreeRoot = vfs.MustNew(info.WorktreeRoot) + } b.Config.Bundle.Git.ActualBranch = info.CurrentBranch if b.Config.Bundle.Git.Branch == "" { diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index e6d529101a..f464ec0840 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -73,8 +73,16 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn log.Warnf(ctx, "Failed to read git info: %s", err) } + var WorktreeRoot vfs.Path + + if info.WorktreeRoot == "" { + WorktreeRoot = localRoot + } else { + WorktreeRoot = vfs.MustNew(info.WorktreeRoot) + } + opts := sync.SyncOptions{ - WorktreeRoot: info.GuessedWorktreeRoot, + WorktreeRoot: WorktreeRoot, LocalRoot: localRoot, Paths: []string{"."}, Include: nil, diff --git a/libs/git/info.go b/libs/git/info.go index ac583c1dd8..8b2b26294f 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -25,9 +25,6 @@ type GitRepositoryInfo struct { // Absolute path to determined worktree root or "" if worktree root could not be determined. WorktreeRoot string - - // vfs.Path variant of WorktreeRoot if WorktreeRoot is set; otherwise defaults to input path. - GuessedWorktreeRoot vfs.Path } type gitInfo struct { @@ -45,10 +42,8 @@ type response struct { // - In case we could not find git repository, all string fields of GitRepositoryInfo will be "" and err will be nil. // - If there were any errors when trying to determine git root (e.g. API call returned an error or there were permission issues // reading the file system), all strings fields of GitRepositoryInfo will be "" and err will be non-nil. -// - For convenience, GuessedWorktreeRoot parameter will be set to path in the above two cases. // - If we could determine git worktree root but there were errors when reading metadata (origin, branch, commit), those errors -// will be logged as warnings, GitRepositoryInfo is guaranteed to have non-empty WorktreeRoot and corresponding GuessedWorktreeRoot -// and other fields on best effort basis. The err will be nil. +// will be logged as warnings, GitRepositoryInfo is guaranteed to have non-empty WorktreeRoot and other fields on best effort basis. // - In successful case, all fields are set to proper git repository metadata. func FetchRepositoryInfo(ctx context.Context, path string, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { if strings.HasPrefix(path, "/Workspace/") && dbr.RunsOnRuntime(ctx) { @@ -59,10 +54,7 @@ func FetchRepositoryInfo(ctx context.Context, path string, w *databricks.Workspa } func fetchRepositoryInfoAPI(ctx context.Context, path string, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { - result := GitRepositoryInfo{ - // For convenience, this field defaults to input path, even if err is also set. - GuessedWorktreeRoot: vfs.MustNew(path), - } + result := GitRepositoryInfo{} apiClient, err := client.New(w.Config) if err != nil { @@ -96,8 +88,6 @@ func fetchRepositoryInfoAPI(ctx context.Context, path string, w *databricks.Work result.LatestCommit = gi.HeadCommitID result.CurrentBranch = gi.Branch result.WorktreeRoot = fixedPath - // Note, this won't work on Windows since vfs.MustNew will call filepath.Abs - result.GuessedWorktreeRoot = vfs.MustNew(fixedPath) } else { log.Warnf(ctx, "Failed to load git info from %s", apiEndpoint) } @@ -113,9 +103,7 @@ func ensureWorkspacePrefix(p string) string { } func fetchRepositoryInfoDotGit(ctx context.Context, path string) (GitRepositoryInfo, error) { - result := GitRepositoryInfo{ - GuessedWorktreeRoot: vfs.MustNew(path), - } + result := GitRepositoryInfo{} rootDir, err := findLeafInTree(path, GitDirectoryName) if rootDir == "" { @@ -123,9 +111,8 @@ func fetchRepositoryInfoDotGit(ctx context.Context, path string) (GitRepositoryI } result.WorktreeRoot = rootDir - result.GuessedWorktreeRoot = vfs.MustNew(rootDir) - repo, err := NewRepository(result.GuessedWorktreeRoot) + repo, err := NewRepository(vfs.MustNew(rootDir)) if err != nil { log.Warnf(ctx, "failed to read .git: %s", err) From 2d3ee1831e5a9c2b8f1ee278988690a49b924c7e Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 14:07:09 +0100 Subject: [PATCH 22/25] don't use result in case of an error --- bundle/config/mutator/load_git_details.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 8289a3b724..82255552aa 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -53,8 +53,8 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn relBundlePath, err := filepath.Rel(b.WorktreeRoot.Native(), b.BundleRoot.Native()) if err != nil { diags = append(diags, diag.FromErr(err)...) + } else { + b.Config.Bundle.Git.BundleRootPath = filepath.ToSlash(relBundlePath) } - - b.Config.Bundle.Git.BundleRootPath = filepath.ToSlash(relBundlePath) return diags } From a689396ebc40218937c1ce728d4394c283ba540c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 14:10:39 +0100 Subject: [PATCH 23/25] s/GitRepositoryInfo/RepositoryInfo --- internal/git_fetch_test.go | 6 +++--- libs/git/info.go | 18 +++++++++--------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index 394c95a6ef..60aa6d72ab 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -16,18 +16,18 @@ import ( const examplesRepoUrl = "https://github.com/databricks/bundle-examples" const examplesRepoProvider = "gitHub" -func assertFullGitInfo(t *testing.T, expectedRoot string, info git.GitRepositoryInfo) { +func assertFullGitInfo(t *testing.T, expectedRoot string, info git.RepositoryInfo) { assert.Equal(t, "main", info.CurrentBranch) assert.NotEmpty(t, info.LatestCommit) assert.Equal(t, examplesRepoUrl, info.OriginURL) assert.Equal(t, expectedRoot, info.WorktreeRoot) } -func assertEmptyGitInfo(t *testing.T, info git.GitRepositoryInfo) { +func assertEmptyGitInfo(t *testing.T, info git.RepositoryInfo) { assertSparseGitInfo(t, "", info) } -func assertSparseGitInfo(t *testing.T, expectedRoot string, info git.GitRepositoryInfo) { +func assertSparseGitInfo(t *testing.T, expectedRoot string, info git.RepositoryInfo) { assert.Equal(t, "", info.CurrentBranch) assert.Equal(t, "", info.LatestCommit) assert.Equal(t, "", info.OriginURL) diff --git a/libs/git/info.go b/libs/git/info.go index 8b2b26294f..13c2981135 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -17,7 +17,7 @@ import ( "github.com/databricks/databricks-sdk-go/client" ) -type GitRepositoryInfo struct { +type RepositoryInfo struct { // Various metadata about the repo. Each could be "" if it could not be read. No error is returned for such case. OriginURL string LatestCommit string @@ -39,13 +39,13 @@ type response struct { } // Fetch repository information either by quering .git or by fetching it from API (for dabs-in-workspace case). -// - In case we could not find git repository, all string fields of GitRepositoryInfo will be "" and err will be nil. +// - In case we could not find git repository, all string fields of RepositoryInfo will be "" and err will be nil. // - If there were any errors when trying to determine git root (e.g. API call returned an error or there were permission issues -// reading the file system), all strings fields of GitRepositoryInfo will be "" and err will be non-nil. +// reading the file system), all strings fields of RepositoryInfo will be "" and err will be non-nil. // - If we could determine git worktree root but there were errors when reading metadata (origin, branch, commit), those errors -// will be logged as warnings, GitRepositoryInfo is guaranteed to have non-empty WorktreeRoot and other fields on best effort basis. +// will be logged as warnings, RepositoryInfo is guaranteed to have non-empty WorktreeRoot and other fields on best effort basis. // - In successful case, all fields are set to proper git repository metadata. -func FetchRepositoryInfo(ctx context.Context, path string, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { +func FetchRepositoryInfo(ctx context.Context, path string, w *databricks.WorkspaceClient) (RepositoryInfo, error) { if strings.HasPrefix(path, "/Workspace/") && dbr.RunsOnRuntime(ctx) { return fetchRepositoryInfoAPI(ctx, path, w) } else { @@ -53,8 +53,8 @@ func FetchRepositoryInfo(ctx context.Context, path string, w *databricks.Workspa } } -func fetchRepositoryInfoAPI(ctx context.Context, path string, w *databricks.WorkspaceClient) (GitRepositoryInfo, error) { - result := GitRepositoryInfo{} +func fetchRepositoryInfoAPI(ctx context.Context, path string, w *databricks.WorkspaceClient) (RepositoryInfo, error) { + result := RepositoryInfo{} apiClient, err := client.New(w.Config) if err != nil { @@ -102,8 +102,8 @@ func ensureWorkspacePrefix(p string) string { return p } -func fetchRepositoryInfoDotGit(ctx context.Context, path string) (GitRepositoryInfo, error) { - result := GitRepositoryInfo{} +func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo, error) { + result := RepositoryInfo{} rootDir, err := findLeafInTree(path, GitDirectoryName) if rootDir == "" { From a064d91ac52f104ef6fa2712c9643e8f53fe7545 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 14:24:43 +0100 Subject: [PATCH 24/25] use Join instead of + --- internal/git_fetch_test.go | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index 60aa6d72ab..5dab6be762 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -3,6 +3,7 @@ package internal import ( "os" "os/exec" + "path" "path/filepath" "testing" @@ -39,7 +40,7 @@ func TestAccFetchRepositoryInfoAPI_FromRepo(t *testing.T) { me, err := wt.W.CurrentUser.Me(ctx) require.NoError(t, err) - targetPath := acc.RandomName("/Workspace/Users/" + me.UserName + "/testing-clone-bundle-examples-") + targetPath := acc.RandomName(path.Join("/Workspace/Users", me.UserName, "/testing-clone-bundle-examples-")) stdout, stderr := RequireSuccessfulRun(t, "repos", "create", examplesRepoUrl, examplesRepoProvider, "--path", targetPath) t.Cleanup(func() { RequireSuccessfulRun(t, "repos", "delete", targetPath) @@ -50,7 +51,7 @@ func TestAccFetchRepositoryInfoAPI_FromRepo(t *testing.T) { ctx = dbr.MockRuntime(ctx, true) for _, inputPath := range []string{ - targetPath + "/knowledge_base/dashboard_nyc_taxi", + path.Join(targetPath, "knowledge_base/dashboard_nyc_taxi"), targetPath, } { t.Run(inputPath, func(t *testing.T) { @@ -66,8 +67,8 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { me, err := wt.W.CurrentUser.Me(ctx) require.NoError(t, err) - rootPath := acc.RandomName("/Workspace/Users/" + me.UserName + "/testing-nonrepo-") - _, stderr := RequireSuccessfulRun(t, "workspace", "mkdirs", rootPath+"/a/b/c") + rootPath := acc.RandomName(path.Join("/Workspace/Users", me.UserName, "testing-nonrepo-")) + _, stderr := RequireSuccessfulRun(t, "workspace", "mkdirs", path.Join(rootPath, "a/b/c")) t.Cleanup(func() { RequireSuccessfulRun(t, "workspace", "delete", "--recursive", rootPath) }) @@ -80,7 +81,7 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { msg string }{ { - input: rootPath + "/a/b/c", + input: path.Join(rootPath, "a/b/c"), msg: "", }, { @@ -88,7 +89,7 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { msg: "", }, { - input: rootPath + "/non-existent", + input: path.Join(rootPath, "/non-existent"), msg: "doesn't exist", }, } @@ -113,7 +114,7 @@ func TestAccFetchRepositoryInfoDotGit_FromGitRepo(t *testing.T) { repo := cloneRepoLocally(t, examplesRepoUrl) for _, inputPath := range []string{ - repo + "/knowledge_base/dashboard_nyc_taxi", + filepath.Join(repo, "knowledge_base/dashboard_nyc_taxi"), repo, } { t.Run(inputPath, func(t *testing.T) { @@ -139,12 +140,12 @@ func TestAccFetchRepositoryInfoDotGit_FromNonGitRepo(t *testing.T) { tempDir := t.TempDir() root := filepath.Join(tempDir, "repo") - require.NoError(t, os.MkdirAll(root+"/a/b/c", 0700)) + require.NoError(t, os.MkdirAll(filepath.Join(root, "a/b/c"), 0700)) tests := []string{ - root + "/a/b/c", + filepath.Join(root, "a/b/c"), root, - root + "/non-existent", + filepath.Join(root, "/non-existent"), } for _, input := range tests { @@ -161,9 +162,9 @@ func TestAccFetchRepositoryInfoDotGit_FromBrokenGitRepo(t *testing.T) { tempDir := t.TempDir() root := filepath.Join(tempDir, "repo") - path := root + "/a/b/c" + path := filepath.Join(root, "a/b/c") require.NoError(t, os.MkdirAll(path, 0700)) - require.NoError(t, os.WriteFile(root+"/.git", []byte(""), 0000)) + require.NoError(t, os.WriteFile(filepath.Join(root, ".git"), []byte(""), 0000)) info, err := git.FetchRepositoryInfo(ctx, path, wt.W) assert.NoError(t, err) From 43da63bc50fd4f5a9b9da515db83ecd9249cdfe3 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 5 Dec 2024 09:52:42 +0100 Subject: [PATCH 25/25] lowercase variable --- cmd/sync/sync.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go index f464ec0840..6d722fb08a 100644 --- a/cmd/sync/sync.go +++ b/cmd/sync/sync.go @@ -73,16 +73,16 @@ func (f *syncFlags) syncOptionsFromArgs(cmd *cobra.Command, args []string) (*syn log.Warnf(ctx, "Failed to read git info: %s", err) } - var WorktreeRoot vfs.Path + var worktreeRoot vfs.Path if info.WorktreeRoot == "" { - WorktreeRoot = localRoot + worktreeRoot = localRoot } else { - WorktreeRoot = vfs.MustNew(info.WorktreeRoot) + worktreeRoot = vfs.MustNew(info.WorktreeRoot) } opts := sync.SyncOptions{ - WorktreeRoot: WorktreeRoot, + WorktreeRoot: worktreeRoot, LocalRoot: localRoot, Paths: []string{"."}, Include: nil,