Skip to content

Fix creating local filer at the root of a windows file system#487

Closed
shreyas-goenka wants to merge 15 commits intomainfrom
fix-windows-root-filer
Closed

Fix creating local filer at the root of a windows file system#487
shreyas-goenka wants to merge 15 commits intomainfrom
fix-windows-root-filer

Conversation

@shreyas-goenka
Copy link
Copy Markdown
Contributor

Changes

Allows filer to be created at root of a windows file system

Tests

Manually

@shreyas-goenka shreyas-goenka changed the title add windows fix Fix creating local filer at the root of a windows file system Jun 15, 2023
Copy link
Copy Markdown
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a tiny unit test for this.

Can be:

  • Skip unless Windows
  • Get a TempDir
  • Create filer on /
  • Assert stat on TempDir succeeds

@shreyas-goenka
Copy link
Copy Markdown
Contributor Author

integration tests pass

@shreyas-goenka
Copy link
Copy Markdown
Contributor Author

Not a complete solution yet

}
return &LocalClient{
root: NewRootPath(root),
root: NewUnixRootPath(root),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still a windows path if the specified path is not equal to /, the name is confusing.

Since you have a UnixRootPath, please add a WindowsRootPath that uses filepath.Join instead of path.Join and includes the root check in Join itself. Then there is no need for a NopRootPath and we always end up picking the Windows one when running on Windows, regardless of the specified root.

@shreyas-goenka
Copy link
Copy Markdown
Contributor Author

solved by #506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants