Fix lakectl local diff #7563
Fix lakectl local diff #7563
Conversation
E2E Test Results - DynamoDB Local - Local Block Adapter |
idanovo
added
the
include-changelog
pkg/local/diff.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if info.IsDir() || diffShouldIgnore(info.Name()) { | ||
| if info.IsDir() || diffShouldIgnore(info.Name()) || strings.HasPrefix(e.RelativeKey, local.CacheDirName) { |
There was a problem hiding this comment.
We don't actually use the cache so we can just pass an empty path and remove this
pkg/local/diff.go
Outdated
| return nil | ||
| } | ||
| localPath := strings.TrimPrefix(path, rightPath) | ||
| filePath := e.RelativeKey | ||
| localPath := strings.TrimPrefix(filePath, rightPath) |
There was a problem hiding this comment.
Why do we need this? Also if we do, do we need the next line then?
pkg/block/local/walker.go
Outdated
| @@ -19,7 +19,7 @@ import ( | |||
| "github.com/treeverse/lakefs/pkg/block/params" | |||
| ) | |||
|
|
|||
| const cacheDirName = "_lakefs_cache" | |||
| const CacheDirName = "_lakefs_cache" | |||
There was a problem hiding this comment.
Can revert this as we don't really use the cache
pkg/local/diff.go
Outdated
| return nil, err | ||
| } | ||
| uri := url.URL{Scheme: "local", Path: absPath} | ||
| adapter, err := local.NewAdapter(absPath, local.WithRemoveEmptyDir(false), local.WithAllowedExternalPrefixes([]string{"/"})) |
There was a problem hiding this comment.
| adapter, err := local.NewAdapter(absPath, local.WithRemoveEmptyDir(false), local.WithAllowedExternalPrefixes([]string{"/"})) | |
| adapter, err := local.NewAdapter("", local.WithRemoveEmptyDir(false), local.WithAllowedExternalPrefixes([]string{absPath})) |
| fd, err = os.Create(filepath.Join(dataDir, "subdir", "test.txt")) | ||
| require.NoError(t, err) | ||
| fd, err = os.Create(filepath.Join(dataDir, "subdir-a", "test.txt")) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
I think this is missing verification for this test case (on the status)
There was a problem hiding this comment.
At the end of the test, we verify that lakectl status returned no diff.
In the original bug, we've got changes after committing
There was a problem hiding this comment.
Understood, but we still need to fix the verify status output
| @@ -557,7 +557,7 @@ func TestLakectlFsUpload(t *testing.T) { | |||
| }) | |||
| t.Run("single_file_with_recursive", func(t *testing.T) { | |||
| vars["FILE_PATH"] = "data/ro/ro_1k.0" | |||
| RunCmdAndVerifyFailure(t, Lakectl()+" fs upload --recursive -s files/ro_1k lakefs://"+repoName+"/"+mainBranch+"/"+vars["FILE_PATH"], false, "\ndiff 'local://files/ro_1k/' <--> 'lakefs://${REPO}/${BRANCH}/${FILE_PATH}'...\nlstat files/ro_1k/: not a directory\nError executing command.\n", vars) | |||
There was a problem hiding this comment.
The walk function of local storage tries to create a new folder for caching
There was a problem hiding this comment.
Let's not do that, let's not use caching (see other comments)
Closes #(Insert issue number closed by this PR)
Change Description
Background
Share context and relevant information for the PR: offline discussions, considerations, design decisions etc.
Bug Fix
If this PR is a bug fix, please let us know about:
New Feature
If this PR introduces a new feature, describe it here.
Testing Details
How were the changes tested?
Breaking Change?
Does this change break any existing functionality? (API, CLI, Clients)
Additional info
Logs, outputs, screenshots of changes if applicable (CLI / GUI changes)
Contact Details
How can we get in touch with you if we need more info? (ex. email@example.com)