Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 15 commits into from

Conversation

shreyas-goenka
Copy link
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
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
Contributor Author

integration tests pass

@shreyas-goenka
Copy link
Contributor Author

Not a complete solution yet

return &LocalClient{
root: NewRootPath(root),
root: NewUnixRootPath(root),
Copy link
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
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