-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add pkger source driver support #377
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
663520e
Add pkger source driver support
hnnsgstfssn 655a795
pkger: rename Instance to Pkger
hnnsgstfssn c177e4a
pkger: make WithInstance accept *Pkger
hnnsgstfssn 9afbbbf
pkger: refactor and add access to global pkging.Pkger instance
hnnsgstfssn 8ae7cd4
pkger: fix typo and cleanup debug logging
hnnsgstfssn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package pkger | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net/http" | ||
|
|
||
| "github.com/golang-migrate/migrate/v4/source" | ||
| "github.com/golang-migrate/migrate/v4/source/httpfs" | ||
| "github.com/markbates/pkger/pkging" | ||
| ) | ||
|
|
||
| func init() { | ||
| source.Register("pkger", &driver{}) | ||
| } | ||
|
|
||
| // Pkger is an implementation of http.FileSystem backed by an instance of | ||
| // pkging.Pkger. | ||
| type Pkger struct { | ||
| pkging.Pkger | ||
|
|
||
| // Path is the relative path location of the migrations. It is passed to | ||
| // httpfs.PartialDriver.Init. If unset "/" is used as all paths are | ||
| // absolute. | ||
| Path string | ||
| } | ||
|
|
||
| // Open implements http.FileSystem. | ||
| func (p *Pkger) Open(name string) (http.File, error) { | ||
| f, err := p.Pkger.Open(name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return f.(http.File), nil | ||
| } | ||
|
|
||
| type driver struct { | ||
| httpfs.PartialDriver | ||
| } | ||
|
|
||
| // Open implements source.Driver. NOT IMPLEMENTED. | ||
| func (d *driver) Open(url string) (source.Driver, error) { | ||
| return nil, fmt.Errorf("not yet implemented") | ||
| } | ||
|
|
||
| // WithInstance returns a source.Driver that is backed by an instance of Pkger. | ||
| func WithInstance(instance *Pkger) (source.Driver, error) { | ||
| if instance.Path == "" { | ||
| instance.Path = "/" | ||
| } | ||
|
|
||
| var fs http.FileSystem | ||
| var ds driver | ||
|
|
||
| fs = instance | ||
|
|
||
| if err := ds.Init(fs, instance.Path); err != nil { | ||
| return nil, fmt.Errorf("failed to init: %w", err) | ||
| } | ||
|
|
||
| return &ds, nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| package pkger | ||
|
|
||
| import ( | ||
| "log" | ||
| "testing" | ||
|
|
||
| "github.com/gobuffalo/here" | ||
| st "github.com/golang-migrate/migrate/v4/source/testing" | ||
| "github.com/markbates/pkger/pkging" | ||
| "github.com/markbates/pkger/pkging/mem" | ||
| ) | ||
|
|
||
| func Test(t *testing.T) { | ||
| i := testInstance(t) | ||
|
|
||
| d, err := WithInstance(i) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| st.Test(t, d) | ||
| } | ||
|
|
||
| func TestWithInstance(t *testing.T) { | ||
| i := testInstance(t) | ||
|
|
||
| _, err := WithInstance(i) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| func TestOpen(t *testing.T) { | ||
| d := &driver{} | ||
| _, err := d.Open("") | ||
| if err == nil { | ||
| t.Fatal("expected err, because it's not implemented yet") | ||
| } | ||
| } | ||
|
|
||
| func testInstance(t *testing.T) *Pkger { | ||
| info, err := here.New().Current() | ||
| if err != nil { | ||
| t.Fatalf("failed to get the current here.Info: %v\n", err) | ||
| } | ||
|
|
||
| pkg, err := mem.New(info) | ||
| if err != nil { | ||
| log.Fatalf("failed to create an in-memory pkging.Pkger: %v\n", err) | ||
| } | ||
|
|
||
| createMigrationFile(t, pkg, "/1_foobar.up.sql") | ||
| createMigrationFile(t, pkg, "/1_foobar.down.sql") | ||
| createMigrationFile(t, pkg, "/3_foobar.up.sql") | ||
| createMigrationFile(t, pkg, "/4_foobar.up.sql") | ||
| createMigrationFile(t, pkg, "/4_foobar.down.sql") | ||
| createMigrationFile(t, pkg, "/5_foobar.down.sql") | ||
| createMigrationFile(t, pkg, "/7_foobar.up.sql") | ||
| createMigrationFile(t, pkg, "/7_foobar.down.sql") | ||
|
|
||
| return &Pkger{ | ||
| Pkger: pkg, | ||
| } | ||
| } | ||
|
|
||
| func createMigrationFile(t *testing.T, pkg pkging.Pkger, m string) { | ||
| _, err := pkg.Create(m) | ||
| if err != nil { | ||
| t.Fatalf("failed to package migration file %q: %v\n", m, err) | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a separate
driverstruct? You can embedhttpfs.PartialDriverintoPkgerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the driver and the instance requires
Openmethods of different signatures.source.DriverrequiresOpen(url string) (source.Driver, error).http.FileSystemrequiresOpen(name string) (http.File, error).It would've been nice if
pkging.Pkgerimplementedhttp.FileSystembut it turns out in fact it does not. I might be missing something but I need to implementhttp.FileSystemto be able to pass it to the partial driverInit(fs http.FileSystem, path string) errormethod.Pkger(previouslyInstance) wrapspkging.Pkgerand implementshttp.FileSystemso that it can be passed to the partial driver.driverimplements thesource.Driverinterface by providing itsOpenmethod.If this is all unclear I'm happy to rework it somehow but you will have to provide some guidance on what you think makes more sense.
I'm also curious to know if I'm just missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use one struct by not embedding
pkging.Pkger.e.g.
Looks like it does
You can pass
Pkger.pkgertoInit()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signatures are different. It does't return
http.Filebutpkging.File. It's subtle but meansWhat am I missing? I still don't see how it would work by simply not embedding
pkging.Pkger.If we want
Pkgerto be the driver and look likeI'm not against that but I think we need to wrap
Pkger.pkgerto implementhttp.FileSystem. Another approach I can think of is to haveand then wrap
pkging.Pkgerjust before callingInitlike soWhat about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this a bit more I'm honestly wondering if it's worth adding this driver at all since it essentially only wraps
pkging.Pkgerinhttp.FileSystem. It might make sense to just put it on the user to wrap it themselves and instead usehttpfs.Newdirectly. What do you think about that?For
packr.Box, the other packager, it should be even easier since it alreay implementshttp.FileSystem, so the user can pass it straight in.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, didn't notice that! That's annoying... Not sure why they went that route. At a quick glance, the interfaces look the same to me...
Looks like we'll need to wrap
pkging.Pkger... I'm not picky about whattype Pkger structlooks like as long as the fields aren't exported.If you're only going to use
migrateas a Go library, thenhttpfs.New()is probably the easiest and quickest route to get started. However, if you want to use DB URIs and themigrateCLI, you'll need to create a driver using thehttpfs.PartialDriverand register it. You can't usehttpfs.New()for this since doing so will hardcode the driver to thehttp.FileSystemspecified viahttpfs.New().I'd recommend having CLI support to debug and fix any issues you have with your migrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what you mean by hardcoding the driver to the specified file sytem. Could you elaborate?
How would the CLI support a source that is explicitly for embedded data? What would it mean to tell the CLI to read migrations from
pkger:///migrations?Looking at
go_bindatait doesn't implement CLI support and as you can see I just did the same by leavingsource.Driver.Openunimplemented. Personally I only usemigrateas a library.That said, consumers of
pkgermostly register resources in a global instance ofpkging.Pkgerusingpkger.Applyand then access files using package scoped functionspkger.Open. This global instance it not, as far as I can tell, accessible. It seems like a good idea to usesource.Driver.Opento return a driver that reads from the globalpkging.Pkgerinstance. It would allow library users to opt for eitherWithInstanceand their own instance ofpkging.Pkgeror forOpenafter registering migrations on the global instance withpkger.Apply. ThepkgerCLI currently only generates code that registers embedded resources in the global instance so it would be nice to allowing access to it.I've actually already gone ahead ahead and implemented this along with increased test coverage. I'm happy to address any new feedback on that.
On the back of the above to partly answer my own question,
pkger:///migrationswill read migrations from the relative directory/migrationsusing the package scopedpkger.Openi.e. using a global instance ofpkging.Pkger. However in the context of the CLI I'm not sure that helps anyone since they can't register migrations on the global instance that exists only in memory during the execution of the CLI, right?