Repository navigation
Fix lakectl fs rm -r deleting siblings that share the path prefix - #10600
Closed
RyoKusnadi wants to merge 1 commit into
Closed
RyoKusnadi wants to merge 1 commit into
RyoKusnadi wants to merge 1 commit into
Conversation
Signed-off-by: Ryo Kusnadi <ryokusnadi@gmail.com>
Contributor
Author
|
Closing this for now, happy to reopen if it's useful. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10582
Change Description
Background
lakectl fs rm -rshould delete a folder.fs download -ralready adds a trailing/to the path, butfs rm -rdidn't.Bug Fix
lakectl fs rm -r lakefs://repo/main/logsalso deleteslogs.txtandlogs-archive/3.txt, not justlogs/1.txtandlogs/2.txt.How it happens: run the command with a path that has no trailing slash and another object name starts with the same text.
Steps: upload
logs/1.txt,logs/2.txt,logs.txt,logs-archive/3.txt, then runlakectl fs rm -r lakefs://repo/main/logs.Expected: only
logs/1.txtandlogs/2.txtare deleted. Actual: all four are deleted.cmd/lakectl/cmd/fs_rm.gopassed the path unchanged as the list prefix, and the listing matches on a raw string prefix.recursiveDeletePrefixadds a trailing/when the path is not empty and doesn't have one. An empty path (whole branch) is unchanged. This matchesfs download -r. Fixing it in the one place the prefix is built covers everyrm -rcall.Scope:
fs ls -ris read-only and left as is, as the issue notes.fs download -ralready does this.Testing Details
Added
TestRecursiveDeletePrefix(empty, with and without trailing slash, nested).Before the fix it doesn't build (
undefined: recursiveDeletePrefix); after, it passes.Ran:
go vetandgo test ./cmd/lakectl/cmd/ -run 'TestRecursiveDeletePrefix|TestFsRm',gofmt -l, golangci-lint (no findings in the touched files).Not run: esti system tests or a live lakeFS server, so the four-object steps above were not run end to end.
Environment: fix/lakectl-rm-recursive-keep-siblings @ 94dbbe2, go1.27.1, darwin/arm64.
Breaking Change?
Behavior change:
rm -ron a path without/no longer removes siblings that share the prefix. Anyone relying on that would now need to list the sibling paths themselves.