lakectl local: add option to skip non regular files #8055
Conversation
N-o-Z
added
area/lakectl
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
cmd/lakectl/cmd/root.go
Outdated
| @@ -94,6 +94,10 @@ type Configuration struct { | |||
| // setting FixSparkPlaceholder to true will change spark placeholder with the actual location. for more information see https://github.com/treeverse/lakeFS/issues/2213 | |||
| FixSparkPlaceholder bool `mapstructure:"fix_spark_placeholder"` | |||
| } | |||
| Local struct { | |||
| // IgnoreSymLinks - By default lakectl local fails if local directory contains a symbolic link. When set, lakectl will ignore the symbolic links instead. | |||
| IgnoreSymLinks bool `mapstructure:"ignore_symlinks"` | |||
There was a problem hiding this comment.
Consider renaming to SkipSymlinks. "Ignore" is a bit unclear?
pkg/fileutil/io.go
Outdated
| @@ -179,11 +179,19 @@ func VerifySafeFilename(absPath string) error { | |||
| if !filepath.IsAbs(absPath) { | |||
| return fmt.Errorf("relative path not allowed: %w", ErrInvalidPath) | |||
| } | |||
| filename, err := filepath.EvalSymlinks(absPath) | |||
| if err != nil { | |||
| if err := EvalSymlink(absPath); err != nil { | |||
There was a problem hiding this comment.
This can be messy and slow. Also the implementation may miss the case of a dumping to itself - depending on how cyclic symlink expansion is handled.
Prefer to use the Symlink bit on FileMode returned by Stat. It is faster, precise, and does not depend on precisely understanding the implementation.
There was a problem hiding this comment.
Added tests + modified configuration param and check for all irregular files
| @@ -179,12 +179,5 @@ func VerifySafeFilename(absPath string) error { | |||
| if !filepath.IsAbs(absPath) { | |||
| return fmt.Errorf("relative path not allowed: %w", ErrInvalidPath) | |||
| } | |||
| filename, err := filepath.EvalSymlinks(absPath) | |||
There was a problem hiding this comment.
Function comment needs a do over
| } | ||
| return false, nil |
There was a problem hiding this comment.
Can in Irregular file be in the ignoreFileList?
There was a problem hiding this comment.
The ignore file list is a list of files we decided to ignore. It should not contain any irregular files and if at some point it would - we'd need to resolve that internally (it's not something controlled by the user)
cmd/lakectl/cmd/root.go
Outdated
| @@ -94,6 +94,10 @@ type Configuration struct { | |||
| // setting FixSparkPlaceholder to true will change spark placeholder with the actual location. for more information see https://github.com/treeverse/lakeFS/issues/2213 | |||
| FixSparkPlaceholder bool `mapstructure:"fix_spark_placeholder"` | |||
| } | |||
| Local struct { | |||
| // SkipIrregularFiles - By default lakectl local fails if local directory contains a symbolic link. When set, lakectl will ignore the symbolic links instead. | |||
| SkipIrregularFiles bool `mapstructure:"skip_irregular_files"` | |||
There was a problem hiding this comment.
I would prefer skip_not_regular_files. I cannot find any references to "irregular files", even we say "not a regular file" in the error message.
Or reverse the flag and give it a positive description and default true with fail_on_non_regular_file.
cmd/lakectl/cmd/root.go
Outdated
| @@ -94,6 +94,10 @@ type Configuration struct { | |||
| // setting FixSparkPlaceholder to true will change spark placeholder with the actual location. for more information see https://github.com/treeverse/lakeFS/issues/2213 | |||
| FixSparkPlaceholder bool `mapstructure:"fix_spark_placeholder"` | |||
| } | |||
| Local struct { | |||
| // SkipIrregularFiles - By default lakectl local fails if local directory contains a symbolic link. When set, lakectl will ignore the symbolic links instead. | |||
There was a problem hiding this comment.
| // SkipIrregularFiles - By default lakectl local fails if local directory contains a symbolic link. When set, lakectl will ignore the symbolic links instead. | |
| // SkipNotRegularegularFiles - If set, skip any non-regular files encountered but keep working. If | |
| // clear, fail if any non-regular file is encountered. Non-regular files include symbolic links, | |
| // UNIX-domain sockets, devices, and FIFOs (named pipes). |
N-o-Z
changed the title
N-o-Z
changed the title
Closes #8054
Change Description
Background
Add option which allows ignoring symbolic links found in local directory instead of failing the sync.
This will allow users to use lakectl local even if they have symlinks in their sync directory
New Feature
Added new configuration parameter:
lakectl.local.skip_non_regular_files- which is disabled by default.When set, lakectl local will ignore non regular files found in local directory when performing diff and when syncing local path with remote.
Testing Details
Tested manually
Breaking Change?
No
Additional info
Full context: https://github.com/treeverse/product/issues/450