fix databricks fs cp command #2901
Conversation
mukundkumarjha
requested review from
pietern,
andrewnester,
shreyas-goenka,
denik and
anton-107
as code owners
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
@shreyas-goenka Can you take a look? |
There was a problem hiding this comment.
Hi @mukundkumarjha! Thanks for looking into this issue. This issue is more complex than it appears at first glance. In order to acceptant this contribution we would need:
- To think about what this means for users on Windows. We should also have the fix work on Windows.
- To match linux
cpsemantics, the command should not try to create a directory and should just error if the directory does not exist. - Once resolved, please also consider adding a unit test. For example you can search for:
TestFsCpFileToDir
|
|
||
| // Check if target directory exists | ||
| _, err := c.targetFiler.Stat(c.ctx, targetDir) | ||
| if err != nil { |
There was a problem hiding this comment.
If the path has a trailing / but does not exist, we should error instead to match the semantics of the linux cp command. Could you please update this PR to do so?
| @@ -196,6 +214,11 @@ func newCpCommand() *cobra.Command { | |||
| return c.cpFileToDir(sourcePath, targetPath) | |||
| } | |||
|
|
|||
| // Check if target path ends with a slash, indicating it should be treated as a directory | |||
| if strings.HasSuffix(fullTargetPath, "/") || strings.HasSuffix(targetPath, "/") { | |||
There was a problem hiding this comment.
| if strings.HasSuffix(fullTargetPath, "/") || strings.HasSuffix(targetPath, "/") { | |
| if strings.HasSuffix(fullTargetPath, "/") { |
Checking fullTargetPath for a trailing slash should be enough. However, this also needs to work on Windows. In that case I'm not sure whether having this change means a breaking change for customers.
There was a problem hiding this comment.
I suspect using filepath.ToSlash would probably do the trick.
If the destination directory does't exists cp command throws and error similar to linux cp command.
Fixes
#2835