Skip to content

Commit

Permalink
Remove empty local folders when using lakectl local sync (#7977)
Browse files Browse the repository at this point in the history 
* Remove empty local dirs when using lakectl local sync

* Add esti tests

* Handle remove dir deletion

* Simplify
  • Loading branch information
@itaigilo
itaigilo authored Jul 14, 2024
1 parent 0c68257 commit 1e2a3f7
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 4 deletions.
57 changes: 56 additions & 1 deletion esti/lakectl_local_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,12 +255,37 @@ func TestLakectlLocal_clone(t *testing.T) {
require.Contains(t, sanitizedResult, vars["PREFIX"]+"test2.txt")
require.NotContains(t, sanitizedResult, vars["PREFIX"]+"nodiff.txt")
})
}

func TestLakectlLocal_posix_permissions(t *testing.T) {
tmpDir := t.TempDir()
fd, err := os.CreateTemp(tmpDir, "")
require.NoError(t, err)
require.NoError(t, fd.Close())
repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)
vars := map[string]string{
"REPO": repoName,
"STORAGE": storage,
"BRANCH": mainBranch,
"REF": mainBranch,
}

// No repo
vars["LOCAL_DIR"] = tmpDir
RunCmdAndVerifyFailureWithFile(t, Lakectl()+" local clone lakefs://"+repoName+"/"+mainBranch+"/ "+tmpDir, false, "lakectl_local_clone_non_empty", vars)

runCmd(t, Lakectl()+" repo create lakefs://"+repoName+" "+storage, false, false, vars)
runCmd(t, Lakectl()+" log lakefs://"+repoName+"/"+mainBranch, false, false, vars)

// Bad ref
RunCmdAndVerifyFailureWithFile(t, Lakectl()+" local init lakefs://"+repoName+"/bad_ref/ "+tmpDir, false, "lakectl_local_commit_not_found", vars)

t.Run("diff with posix permissions", func(t *testing.T) {
dataDir, err := os.MkdirTemp(tmpDir, "")
require.NoError(t, err)
vars["LOCAL_DIR"] = dataDir
vars["PREFIX"] = "posix"
vars["PREFIX"] = "posix-diff"

lakectl := LakectlWithPosixPerms()
RunCmdAndVerifyContainsText(t, lakectl+" local clone lakefs://"+repoName+"/"+mainBranch+"/"+vars["PREFIX"]+" "+dataDir, false, "Successfully cloned lakefs://${REPO}/${REF}/${PREFIX}/ to ${LOCAL_DIR}.", vars)
Expand Down Expand Up @@ -290,6 +315,36 @@ func TestLakectlLocal_clone(t *testing.T) {
require.Contains(t, sanitizedResult, "with-diff.txt")
require.NotContains(t, sanitizedResult, "no-diff.txt")
})

t.Run("sync folders deletion", func(t *testing.T) {
dataDir, err := os.MkdirTemp(tmpDir, "")
require.NoError(t, err)
vars["LOCAL_DIR"] = dataDir
vars["PREFIX"] = "posix-folder-deletion"

lakectl := LakectlWithPosixPerms()
RunCmdAndVerifyContainsText(t, lakectl+" local clone lakefs://"+repoName+"/"+mainBranch+"/"+vars["PREFIX"]+" "+dataDir, false, "Successfully cloned lakefs://${REPO}/${REF}/${PREFIX}/ to ${LOCAL_DIR}.", vars)
localVerifyDirContents(t, dataDir, []string{})

// upload a new empty folder
localDirPath := filepath.Join(dataDir, "empty_local_folder")
err = os.Mkdir(localDirPath, fileutil.DefaultDirectoryMask)
require.NoError(t, err)
commitMessage := "add empty folder"
res := runCmd(t, lakectl+" local commit "+dataDir+" -m \""+commitMessage+"\"", false, false, vars)
require.Contains(t, res, "upload empty_local_folder")

// remove the empty folder locally, and validate it's removed from the remote repo
err = os.Remove(localDirPath)
require.NoError(t, err)
commitMessage = "remove empty folder"
res = runCmd(t, lakectl+" local commit "+dataDir+" -m \""+commitMessage+"\"", false, false, vars)
require.Contains(t, res, "delete remote path: empty_local_folder/")

res = runCmd(t, lakectl+" local status "+dataDir, false, false, vars)
require.Contains(t, res, "No diff found")
require.NotContains(t, res, "empty_local_folder")
})
}

func TestLakectlLocal_pull(t *testing.T) {
Expand Down
12 changes: 9 additions & 3 deletions pkg/local/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ func DiffLocalWithHead(left <-chan apigen.ObjectStats, rightPath string, include
return err
}

if !includeInDiff(info, includeDirs) {
if !includeLocalFileInDiff(info, includeDirs) {
return nil
}

Expand All @@ -299,7 +299,9 @@ func DiffLocalWithHead(left <-chan apigen.ObjectStats, rightPath string, include
}
switch {
case currentRemoteFile.Path < localPath: // We removed a file locally
changes = append(changes, &Change{ChangeSourceLocal, currentRemoteFile.Path, ChangeTypeRemoved})
if includeRemoteFileInDiff(currentRemoteFile, includeDirs) {
changes = append(changes, &Change{ChangeSourceLocal, currentRemoteFile.Path, ChangeTypeRemoved})
}
currentRemoteFile.Path = ""
case currentRemoteFile.Path == localPath:
remoteMtime, err := getMtimeFromStats(currentRemoteFile)
Expand Down Expand Up @@ -383,7 +385,7 @@ func ListRemote(ctx context.Context, client apigen.ClientWithResponsesInterface,
return nil
}

func includeInDiff(info fs.FileInfo, includeDirs bool) bool {
func includeLocalFileInDiff(info fs.FileInfo, includeDirs bool) bool {
if info.IsDir() {
return includeDirs
} else {
Expand All @@ -395,3 +397,7 @@ func includeInDiff(info fs.FileInfo, includeDirs bool) bool {
}
}
}

func includeRemoteFileInDiff(currentRemoteFile apigen.ObjectStats, includeDirs bool) bool {
return includeDirs || !strings.HasSuffix(currentRemoteFile.Path, uri.PathSeparator)
}
90 changes: 90 additions & 0 deletions pkg/local/diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ func TestDiffLocal(t *testing.T) {
Name string
IncludeUnixPermissions bool
LocalPath string
InitLocalPath func() string
CleanLocalPath func(localPath string)
RemoteList []apigen.ObjectStats
Expected []*local.Change
}{
Expand Down Expand Up @@ -266,10 +268,86 @@ func TestDiffLocal(t *testing.T) {
},
},
},
{
Name: "t1_empty_folder_removed",
IncludeUnixPermissions: true,
LocalPath: "testdata/localdiff/t1/sub/folder",
RemoteList: []apigen.ObjectStats{
{
Path: "empty-folder/",
SizeBytes: swag.Int64(1),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid+1, osGid, local.DefaultDirectoryPermissions-umask),
}, {
Path: "f.txt",
SizeBytes: swag.Int64(6),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
},
},
Expected: []*local.Change{
{
Path: "empty-folder/",
Type: local.ChangeTypeRemoved,
},
},
},
{
Name: "t1_empty_folder_ignored",
IncludeUnixPermissions: false,
LocalPath: "testdata/localdiff/t1/sub/folder",
RemoteList: []apigen.ObjectStats{
{
Path: "empty-folder/",
SizeBytes: swag.Int64(1),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid+1, osGid, local.DefaultDirectoryPermissions-umask),
}, {
Path: "f.txt",
SizeBytes: swag.Int64(6),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
},
},
Expected: []*local.Change{},
},
{
Name: "empty_folder_added",
IncludeUnixPermissions: true,
InitLocalPath: func() string {
return createTempEmptyFolder(t)
},
CleanLocalPath: func(localPath string) {
_ = os.RemoveAll(localPath)
},
RemoteList: []apigen.ObjectStats{},
Expected: []*local.Change{
{
Path: "empty-folder/",
Type: local.ChangeTypeAdded,
},
},
},
{
Name: "empty_folder_ignored",
IncludeUnixPermissions: false,
InitLocalPath: func() string {
return createTempEmptyFolder(t)
},
CleanLocalPath: func(localPath string) {
_ = os.RemoveAll(localPath)
},
RemoteList: []apigen.ObjectStats{},
Expected: []*local.Change{},
},
}

for _, tt := range cases {
t.Run(tt.Name, func(t *testing.T) {
if tt.InitLocalPath != nil {
tt.LocalPath = tt.InitLocalPath()
}

fixTime(t, tt.LocalPath, tt.IncludeUnixPermissions)
fixUnixPermissions(t, tt.LocalPath)

Expand All @@ -281,6 +359,11 @@ func TestDiffLocal(t *testing.T) {
makeChan(lc, left)

changes, err := local.DiffLocalWithHead(lc, tt.LocalPath, tt.IncludeUnixPermissions, tt.IncludeUnixPermissions)

if tt.CleanLocalPath != nil {
tt.CleanLocalPath(tt.LocalPath)
}

if err != nil {
t.Fatal(err)
}
Expand All @@ -296,6 +379,13 @@ func TestDiffLocal(t *testing.T) {
}
}

func createTempEmptyFolder(t *testing.T) string {
dataDir, err := os.MkdirTemp("", "")
require.NoError(t, err)
_ = os.Mkdir(filepath.Join(dataDir, "empty-folder"), 0755)
return dataDir
}

func getPermissionsMetadata(uid, gid, mode int) *apigen.ObjectUserMetadata {
return &apigen.ObjectUserMetadata{
AdditionalProperties: map[string]string{
Expand Down
3 changes: 3 additions & 0 deletions pkg/local/sync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,9 @@ func (s *SyncManager) deleteRemote(ctx context.Context, remote *uri.URI, change
}
}()
dest := filepath.ToSlash(filepath.Join(remote.GetPath(), change.Path))
if strings.HasSuffix(change.Path, uri.PathSeparator) { // handle directory marker
dest += uri.PathSeparator
}
resp, err := s.client.DeleteObjectWithResponse(ctx, remote.Repository, remote.Ref, &apigen.DeleteObjectParams{
Path: dest,
})
Expand Down

0 comments on commit 1e2a3f7

Please sign in to comment.