From d99bf3ac746d9c425204954c404ab1818156d739 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 5 Dec 2024 10:38:08 +0100 Subject: [PATCH 1/7] Clean up vfs/leaf.go The git module now uses alternative implementation that uses os module + strings as path directly (#1945). The remaining use case is also updated to use git.FindLeafInTree, as it's a better fit. --- internal/bundle/helpers.go | 7 +++---- libs/git/info.go | 4 ++-- libs/vfs/leaf.go | 29 ----------------------------- libs/vfs/leaf_test.go | 38 -------------------------------------- 4 files changed, 5 insertions(+), 73 deletions(-) delete mode 100644 libs/vfs/leaf.go delete mode 100644 libs/vfs/leaf_test.go diff --git a/internal/bundle/helpers.go b/internal/bundle/helpers.go index dd9c841c9e..0129f50c24 100644 --- a/internal/bundle/helpers.go +++ b/internal/bundle/helpers.go @@ -18,8 +18,8 @@ import ( "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/flags" + "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/template" - "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go" "github.com/stretchr/testify/require" ) @@ -144,15 +144,14 @@ func getBundleRemoteRootPath(w *databricks.WorkspaceClient, t *testing.T, unique } func blackBoxRun(t *testing.T, root string, args ...string) (stdout string, stderr string) { - cwd := vfs.MustNew(".") - gitRoot, err := vfs.FindLeafInTree(cwd, ".git") + gitRoot, err := git.FindLeafInTree(".", ".git") require.NoError(t, err) t.Setenv("BUNDLE_ROOT", root) // Create the command cmd := exec.Command("go", append([]string{"run", "main.go"}, args...)...) - cmd.Dir = gitRoot.Native() + cmd.Dir = gitRoot // Create buffers to capture output var outBuffer, errBuffer bytes.Buffer diff --git a/libs/git/info.go b/libs/git/info.go index 13c2981135..01779da774 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -105,7 +105,7 @@ func ensureWorkspacePrefix(p string) string { func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo, error) { result := RepositoryInfo{} - rootDir, err := findLeafInTree(path, GitDirectoryName) + rootDir, err := FindLeafInTree(path, GitDirectoryName) if rootDir == "" { return result, err } @@ -135,7 +135,7 @@ func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo return result, nil } -func findLeafInTree(p string, leafName string) (string, error) { +func FindLeafInTree(p string, leafName string) (string, error) { var err error for i := 0; i < 10000; i++ { _, err = os.Stat(filepath.Join(p, leafName)) diff --git a/libs/vfs/leaf.go b/libs/vfs/leaf.go deleted file mode 100644 index 8c11f9039a..0000000000 --- a/libs/vfs/leaf.go +++ /dev/null @@ -1,29 +0,0 @@ -package vfs - -import ( - "errors" - "io/fs" -) - -// FindLeafInTree returns the first path that holds `name`, -// traversing up to the root of the filesystem, starting at `p`. -func FindLeafInTree(p Path, name string) (Path, error) { - for p != nil { - _, err := fs.Stat(p, name) - - // No error means we found the leaf in p. - if err == nil { - return p, nil - } - - // ErrNotExist means we continue traversal up the tree. - if errors.Is(err, fs.ErrNotExist) { - p = p.Parent() - continue - } - - return nil, err - } - - return nil, fs.ErrNotExist -} diff --git a/libs/vfs/leaf_test.go b/libs/vfs/leaf_test.go deleted file mode 100644 index da9412ec02..0000000000 --- a/libs/vfs/leaf_test.go +++ /dev/null @@ -1,38 +0,0 @@ -package vfs - -import ( - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestFindLeafInTree(t *testing.T) { - wd, err := os.Getwd() - require.NoError(t, err) - - root := filepath.Join(wd, "..", "..") - - // Find from working directory should work. - { - out, err := FindLeafInTree(MustNew(wd), ".git") - assert.NoError(t, err) - assert.Equal(t, root, out.Native()) - } - - // Find from project root itself should work. - { - out, err := FindLeafInTree(MustNew(root), ".git") - assert.NoError(t, err) - assert.Equal(t, root, out.Native()) - } - - // Find for something that doesn't exist should work. - { - out, err := FindLeafInTree(MustNew(root), "this-leaf-doesnt-exist-anywhere") - assert.ErrorIs(t, err, os.ErrNotExist) - assert.Equal(t, nil, out) - } -} From 2e102b7c1a5d0c0ebfc3335a0982b07dd2c26488 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 6 Dec 2024 12:27:17 +0100 Subject: [PATCH 2/7] use filepath.Abs to FindLeafInTree --- libs/git/info.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/git/info.go b/libs/git/info.go index 01779da774..8523df24a2 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -136,7 +136,10 @@ func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo } func FindLeafInTree(p string, leafName string) (string, error) { - var err error + p, err := filepath.Abs(p) + if err != nil { + return "", err + } for i := 0; i < 10000; i++ { _, err = os.Stat(filepath.Join(p, leafName)) From 8b5bb37019f13d191a8aee5b1e584824c40fe48f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 17:43:36 +0100 Subject: [PATCH 3/7] move leaf_test.go to info_test.go --- libs/git/info_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 libs/git/info_test.go diff --git a/libs/git/info_test.go b/libs/git/info_test.go new file mode 100644 index 0000000000..1294935874 --- /dev/null +++ b/libs/git/info_test.go @@ -0,0 +1,38 @@ +package git + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFindLeafInTree(t *testing.T) { + wd, err := os.Getwd() + require.NoError(t, err) + + root := filepath.Join(wd, "..", "..") + + // Find from working directory should work. + { + out, err := FindLeafInTree(wd, ".git") + assert.NoError(t, err) + assert.Equal(t, root, out) + } + + // Find from project root itself should work. + { + out, err := FindLeafInTree(root, ".git") + assert.NoError(t, err) + assert.Equal(t, root, out) + } + + // Find for something that doesn't exist should work. + { + out, err := FindLeafInTree(root, "this-leaf-doesnt-exist-anywhere") + assert.NoError(t, err) + assert.Equal(t, "", out) + } +} From dab674bbdb9bb13679b4b5f64dd4ab2927d968a0 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 10 Dec 2024 14:23:46 +0100 Subject: [PATCH 4/7] switch to folders.FindDirWithLeaf --- internal/bundle/helpers.go | 4 ++-- libs/git/info.go | 3 ++- libs/git/info_test.go | 38 -------------------------------------- 3 files changed, 4 insertions(+), 41 deletions(-) delete mode 100644 libs/git/info_test.go diff --git a/internal/bundle/helpers.go b/internal/bundle/helpers.go index 0129f50c24..00a941c952 100644 --- a/internal/bundle/helpers.go +++ b/internal/bundle/helpers.go @@ -18,7 +18,7 @@ import ( "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/flags" - "github.com/databricks/cli/libs/git" + "github.com/databricks/cli/libs/folders" "github.com/databricks/cli/libs/template" "github.com/databricks/databricks-sdk-go" "github.com/stretchr/testify/require" @@ -144,7 +144,7 @@ func getBundleRemoteRootPath(w *databricks.WorkspaceClient, t *testing.T, unique } func blackBoxRun(t *testing.T, root string, args ...string) (stdout string, stderr string) { - gitRoot, err := git.FindLeafInTree(".", ".git") + gitRoot, err := folders.FindDirWithLeaf(".", ".git") require.NoError(t, err) t.Setenv("BUNDLE_ROOT", root) diff --git a/libs/git/info.go b/libs/git/info.go index 8523df24a2..b958026565 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/databricks/cli/libs/dbr" + "github.com/databricks/cli/libs/folders" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go" @@ -105,7 +106,7 @@ func ensureWorkspacePrefix(p string) string { func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo, error) { result := RepositoryInfo{} - rootDir, err := FindLeafInTree(path, GitDirectoryName) + rootDir, err := folders.FindDirWithLeaf(path, GitDirectoryName) if rootDir == "" { return result, err } diff --git a/libs/git/info_test.go b/libs/git/info_test.go deleted file mode 100644 index 1294935874..0000000000 --- a/libs/git/info_test.go +++ /dev/null @@ -1,38 +0,0 @@ -package git - -import ( - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestFindLeafInTree(t *testing.T) { - wd, err := os.Getwd() - require.NoError(t, err) - - root := filepath.Join(wd, "..", "..") - - // Find from working directory should work. - { - out, err := FindLeafInTree(wd, ".git") - assert.NoError(t, err) - assert.Equal(t, root, out) - } - - // Find from project root itself should work. - { - out, err := FindLeafInTree(root, ".git") - assert.NoError(t, err) - assert.Equal(t, root, out) - } - - // Find for something that doesn't exist should work. - { - out, err := FindLeafInTree(root, "this-leaf-doesnt-exist-anywhere") - assert.NoError(t, err) - assert.Equal(t, "", out) - } -} From 122e6f5a4c0279b03e14b9fd048ad9b8871f9edd Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 10 Dec 2024 14:26:12 +0100 Subject: [PATCH 5/7] Add Abs(); clean up --- libs/folders/folders.go | 4 ++++ libs/git/info.go | 32 -------------------------------- 2 files changed, 4 insertions(+), 32 deletions(-) diff --git a/libs/folders/folders.go b/libs/folders/folders.go index c83c711d3f..e27e315579 100644 --- a/libs/folders/folders.go +++ b/libs/folders/folders.go @@ -9,6 +9,10 @@ import ( // FindDirWithLeaf returns the first directory that holds `leaf`, // traversing up to the root of the filesystem, starting at `dir`. func FindDirWithLeaf(dir string, leaf string) (string, error) { + p, err := filepath.Abs(p) + if err != nil { + return "", err + } for { _, err := os.Stat(filepath.Join(dir, leaf)) diff --git a/libs/git/info.go b/libs/git/info.go index b958026565..ce9c70a5c1 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -2,12 +2,8 @@ package git import ( "context" - "errors" - "io/fs" "net/http" - "os" "path" - "path/filepath" "strings" "github.com/databricks/cli/libs/dbr" @@ -135,31 +131,3 @@ func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo return result, nil } - -func FindLeafInTree(p string, leafName string) (string, error) { - p, err := filepath.Abs(p) - if err != nil { - return "", err - } - for i := 0; i < 10000; i++ { - _, err = os.Stat(filepath.Join(p, leafName)) - - if err == nil { - // Found [leafName] 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 1789cca2cb3b3d2555705a7ea32ba51adc4d7ab5 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 10 Dec 2024 14:28:09 +0100 Subject: [PATCH 6/7] fix --- libs/folders/folders.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/folders/folders.go b/libs/folders/folders.go index e27e315579..81bf7ed01d 100644 --- a/libs/folders/folders.go +++ b/libs/folders/folders.go @@ -9,7 +9,7 @@ import ( // FindDirWithLeaf returns the first directory that holds `leaf`, // traversing up to the root of the filesystem, starting at `dir`. func FindDirWithLeaf(dir string, leaf string) (string, error) { - p, err := filepath.Abs(p) + dir, err := filepath.Abs(dir) if err != nil { return "", err } From 1e7d06cc3e1cd0c28b1d0e83cc7f0a52f18f4311 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 10 Dec 2024 16:20:28 +0100 Subject: [PATCH 7/7] handle os.ErrNotExist; update tests --- bundle/config/mutator/load_git_details.go | 6 +++++- internal/git_fetch_test.go | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 82255552aa..5c263ac035 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -2,6 +2,8 @@ package mutator import ( "context" + "errors" + "os" "path/filepath" "github.com/databricks/cli/bundle" @@ -24,7 +26,9 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn var diags diag.Diagnostics info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot.Native(), b.WorkspaceClient()) if err != nil { - diags = append(diags, diag.WarningFromErr(err)...) + if !errors.Is(err, os.ErrNotExist) { + diags = append(diags, diag.WarningFromErr(err)...) + } } if info.WorktreeRoot == "" { diff --git a/internal/git_fetch_test.go b/internal/git_fetch_test.go index 5dab6be762..3da024a3a3 100644 --- a/internal/git_fetch_test.go +++ b/internal/git_fetch_test.go @@ -101,7 +101,7 @@ func TestAccFetchRepositoryInfoAPI_FromNonRepo(t *testing.T) { assert.NoError(t, err) } else { assert.Error(t, err) - assert.Contains(t, err.Error(), test.msg) + assert.ErrorContains(t, err, test.msg) } assertEmptyGitInfo(t, info) }) @@ -151,7 +151,7 @@ func TestAccFetchRepositoryInfoDotGit_FromNonGitRepo(t *testing.T) { for _, input := range tests { t.Run(input, func(t *testing.T) { info, err := git.FetchRepositoryInfo(ctx, input, wt.W) - assert.NoError(t, err) + assert.ErrorIs(t, err, os.ErrNotExist) assertEmptyGitInfo(t, info) }) }