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

Use http.FileSystem for all the things #144

Closed
wants to merge 4 commits into from

Conversation

@coolaj86
Copy link
Contributor

@coolaj86 coolaj86 commented Dec 21, 2018

This shows how it's possible to use http.FileSystem for:

  • file
  • packr
  • fileb0x
  • wedav
  • almost every other thing that works at all like a file system
AJ ONeal added 2 commits Dec 20, 2018
AJ ONeal
AJ ONeal and others added 2 commits Dec 21, 2018
AJ ONeal
@coolaj86
Copy link
Contributor Author

@coolaj86 coolaj86 commented Dec 21, 2018

It looks like the build is failing due to Cassandra, not necessarily this change.

I’ve been able to use it locally in a project and it works as expected.

How would I be able to tell?

@dhui
Copy link
Member

@dhui dhui commented Dec 21, 2018

How would I be able to tell?

Add tests

@dhui
Copy link
Member

@dhui dhui commented Dec 21, 2018

Please address the issues I've raised here. It's fine to address those issues as a comment in this PR. e.g. doesn't need to be a comment in that github issue.

I don't see WebDAV source driver implementing the source.Driver interface, nor do I see any of the WebDAV sub-drivers being registered.

@coolaj86
Copy link
Contributor Author

@coolaj86 coolaj86 commented Dec 21, 2018

@dhui

Add tests

I ran my own tests locally. I also ran your tests locally. The tests that are failing are something about a Cassandra timeout, which seems unrelated. Adding my own tests will not solve that problem as I don't use Cassandra and I wouldn't know how to test this specific functionality more than you and I already have.

Please address the issues I've raised here.

This PR is a direct response to those issues. Please take a look at the code. The purpose of the code is to illustrate that by using Go's http.FileSystem` interface you support almost every file module that exists - including Packr.

WebDAV, the network transfer protocol, is not useful for this. However, the webdav.FileSystem, from the official Go package, much like http.FileSystem is an interface that is used by many packages. Therefore, by supporting both interfaces, you eliminate the need for custom plugins (such as packr, fileb0x, vfsgen, and any others that support them).

@dhui
Copy link
Member

@dhui dhui commented Dec 21, 2018

We need tests for each source driver. Relying on existing tests to catch issues is not sufficient since we need to know if the driver we're depending on works or breaks with an update.

The purpose of the code is to illustrate that by using Go's http.FileSystem` interface you support almost every file module that exists - including Packr.

Ahh, I see now!
I like how using interfaces reduces the need for custom source driver code, but I'm not a fan of the new split logic in the file source driver. It would be cleaner to implement a source driver(s) that only consumes certain interfaces (e.g. named source.HttpFS and source.webdavFS) and re-implement source.File to use source.HttpFS.
Source drivers would still need to be registered but can be registered at the top level. e.g. we won't need to use "sub-drivers"

Are you familiar with other FS interfaces? I find it odd to use an FS interface from the http package since some of the FS won't the associated with HTTP. I did a cursory search for other FS interfaces but didn't find another built-in one. I also found this, but it's not a built-in interface so fewer packages are likely to implement it.

@mikijov
Copy link

@mikijov mikijov commented Jan 29, 2019

I would like to encourage you two somehow resolve this. http.Filesystem is the de-facto standard. You should not really look at it as http. It really relies on os.File interface really, but it limits it down to only few functions. It was first used in web world, so that's why http, but since it has been used in many other projects. The real reason for me adding a vote here is that I too want to use esc since go-binddata is deprecated. So just like @smacker in his lookout project, I would very much like to see support for esc or better, support for any provider of http.Filesystem interface.

@dhui
Copy link
Member

@dhui dhui commented Jan 29, 2019

To be clear, implementing the http.FileSystem interface is not a blocker for the PR. My previous comment was purely exploratory in nature.

The blockers for this PR are:

  1. Merge conflicts
  2. Code cleanup/refactor - The file source driver should rely on the shared/base http.FileSystem source driver "implementation" which should also be used by other source drivers.
  3. New source drivers should have tests for basic functionality

I don't have the bandwidth to make these changes myself, so feel free to open a PR to resolve the blockers mentioned above.

@dhui
Copy link
Member

@dhui dhui commented Jan 29, 2019

@mikijov Thanks for providing background on how http.FileSystem came to be

@coolaj86
Copy link
Contributor Author

@coolaj86 coolaj86 commented Jan 30, 2019

I want to do another PR with the requested changes, but I haven't had the time and so I've just been using my own fork.

However, I don't intend to maintain a fork long-term.

@maxekman
Copy link

@maxekman maxekman commented Jul 26, 2019

Any update on this? Would love to see it merged to enable the use all the different http.FileSystem-compatible libs!

@wayneashleyberry
Copy link

@wayneashleyberry wayneashleyberry commented Nov 25, 2019

Anything I can help with here? I'd really love to get this in as well 🤞

@dhui
Copy link
Member

@dhui dhui commented Dec 24, 2019

Closed in favor of #293

@dhui dhui closed this Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants