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

Packr Box as migration source. #116

Closed
chakrit opened this issue Oct 19, 2018 · 13 comments
Closed

Packr Box as migration source. #116

chakrit opened this issue Oct 19, 2018 · 13 comments

Comments

@chakrit
Copy link

chakrit commented Oct 19, 2018

Is your feature request related to a problem? Please describe.
go-bindata has been left unmaintained, unwillingly or otherwise. See
jteeuwen/go-bindata#5

This presents a problem as we're using go-bindata to embed our migration files into our Go binaries. The next best alternative is https://github.com/gobuffalo/packr

Describe the solution you'd like
Migration source support for packr boxes.

Describe alternatives you've considered
N/A

Additional context
N/A

@chakrit
Copy link
Author

chakrit commented Oct 19, 2018

Threw this together quickly: https://gist.github.com/chakrit/f0f685dc224013ea3b2c3e2e8ac77a34 in case anyone wants to make a proper PR out of it.

@dhui
Copy link
Member

dhui commented Oct 19, 2018

Feel free to open a PR. Don't forget to add tests.

@adelowo
Copy link

adelowo commented Oct 29, 2018

I can send a PR ( plus tests ) for this before Saturday

@maxekman
Copy link

Would be awesome to get Packr support merged.

@adelowo
Copy link

adelowo commented Dec 19, 2018

Damn, I forgot about this.... Will send in the PR on Sunday evening

@coolaj86
Copy link
Contributor

coolaj86 commented Dec 20, 2018

Hey hey! I was just about to work on something similar.

What are your thoughts on using the http.FileServer webdav.FileSystem http.FileSystem interface so that it'll work with gobindata, fileb0x, vfsgen, and all of the other bindata packers as well?

@dhui
Copy link
Member

dhui commented Dec 20, 2018

@coolaj86 What do we gain by using another interface?

I see the following issues with using another interface/struct (http.FileServer or webdav.FS):

  1. The interface needs to provide a way to "list" files which is necessary to read the next/previous migration to run. See the migrate.source.Driver interface.
    • It doesn't look like http.FileServer or webdav.FS provide a method for listing files.
  2. Using another interface won't save any work since you'll need to integrate the bin packer with the other interface, which should be roughly the same amount of work as integrating with migrate.source.Driver
  3. Specifying the source driver would stumble. e.g. the schema would look something like http+go-bindata:// or webdav+go-bindata:// instead of go-bindata://

@adelowo
Copy link

adelowo commented Dec 20, 2018

Hey @coolaj86, let me know if you are picking this one up

@coolaj86
Copy link
Contributor

coolaj86 commented Dec 21, 2018

@dhui @adelowo This is what I meant: #144

Obviously you'll want more docs and whatnot, but that shows how it could work.

http.FileSystem is the "canonical" FileSystem implementation that almost everyone who is making any sort of FS abstraction conforms to. It only differs from os.Open and os.File.Readdir in that it's an interface rather than a struct. Or at least I think I said that right.

@coolaj86
Copy link
Contributor

coolaj86 commented Dec 21, 2018

The interface needs to provide a way to "list" files which is necessary to read the next/previous migration to run. See the migrate.source.Driver interface. It doesn't look like http.FileServer or webdav.FS provide a method for listing files.

They both use http.File which must implement http.File.Readdir (same as os.Readdir).

ioutil.ReadDir is just a convenience wrapper over os.Readdir. Whereas the os package defines a singleton with several wrappers, http.File defines an interface. An os.File implements http.File.

Intuitively, you'd think that http.File would implement an interface from os, not the other way around... but that's just how the cookie crumbled as the language evolved.

Using another interface won't save any work since you'll need to integrate the bin packer with the other interface, which should be roughly the same amount of work as integrating with migrate.source.Driver

The point is that packr, fileb0x, vfsgen, and most other bindata generators already implement http.FileSystem and/or webdav.FileSystem, so there's no need to do any work.

The plugins for packr and fileb0x that I included in my PR are essentially no-op wrappers. The only reason to have such a plugin is for SEO benefit and for the benefit of people who didn't read or understand the documentation of those packages as implementing the http.FileSystem and webdav.FileSystem interfaces (which I didn't understand at first either).

Specifying the source driver would stumble. e.g. the schema would look something like http+go-bindata:// or webdav+go-bindata:// instead of go-bindata://

It's all the same interface (see the code in the PR). A single fs:// namespace would handle all cases. The only reason to have separate names would be for the benefit of documentation and SEO.

@dhui
Copy link
Member

dhui commented Dec 21, 2018

A single fs:// namespace would handle all cases. The only reason to have separate names would be for the benefit of documentation and SEO.

I believe we'll still need to register the each source driver for the following reasons:

  1. Need to import the wrapped source driver. e.g. packr, fileb0x, etc
  2. User visibility. e.g. users know what source drivers are available via the docs and CLI

@dhui
Copy link
Member

dhui commented Jan 13, 2020

This should be trivial to implement now that #293 has been merged.
Also, it looks like there's yet another packer/packager...
https://github.com/markbates/pkger

dhui pushed a commit that referenced this issue Apr 21, 2020
* Add pkger source driver support

As go-bindata has been abandoned [1] there are open requests, #116, for
alternative sources with similar functionality. The Buffalo project [2]
created packr and recently pkger [3] was announced [4] with the
intention to supersede packr.

This change adds support for using pkger as a source.

The implementation relies on httpfs.PartialDriver for pretty much all
functionality.

[1] jteeuwen/go-bindata#5
[2] https://gobuffalo.io/
[3] https://github.com/markbates/pkger
[4] https://blog.gobuffalo.io/introducing-pkger-static-file-embedding-in-go-1ce76dc79c65

* pkger: rename Instance to Pkger

* pkger: make WithInstance accept *Pkger

* pkger: refactor and add access to global pkging.Pkger instance

* pkger: fix typo and cleanup debug logging
@dhui
Copy link
Member

dhui commented Apr 29, 2020

Closing since pkgr is supported. If anyone wants packr support, they can still open a PR and add it.

@dhui dhui closed this as completed Apr 29, 2020
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

No branches or pull requests

5 participants