Add pkger source driver support #377

hnnsgstfssn · 2020-04-18T00:18:16Z

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

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

coveralls · 2020-04-18T02:25:49Z

Pull Request Test Coverage Report for Build 755

37 of 37 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 53.525%

Totals
Change from base Build 751:	0.4%
Covered Lines:	2612
Relevant Lines:	4880

💛 - Coveralls

dhui

Thanks for the PR; it looks like a great start!

dhui · 2020-04-18T08:55:57Z

source/pkger/pkger.go

+
+// Instance is an implementation of http.FileSystem backed by an instance of
+// pkging.Pkger.
+type Instance struct {


Rename Instance to Pkger to improve readability

Sure I don't mind. FWIW I named it Instance to avoid stuttering with pkger.Pkger.

dhui · 2020-04-18T08:56:34Z

source/pkger/pkger.go

+}
+
+// WithInstance returns a source.Driver that is backed by an Instance.
+func WithInstance(instance interface{}) (source.Driver, error) {


Use Pkger instead of interface{}

Will do. I just followed the precedence of existing source driver go_bindata.

dhui · 2020-04-18T09:00:19Z

source/pkger/pkger.go

+	return f.(http.File), nil
+}
+
+type driver struct {


Why use a separate driver struct? You can embed httpfs.PartialDriver into Pkger

Because the driver and the instance requires Open methods of different signatures.

source.Driver requires Open(url string) (source.Driver, error).

http.FileSystem requires Open(name string) (http.File, error).

It would've been nice if pkging.Pkger implemented http.FileSystem but it turns out in fact it does not. I might be missing something but I need to implement http.FileSystem to be able to pass it to the partial driver Init(fs http.FileSystem, path string) error method.

Pkger (previously Instance) wraps pkging.Pkger and implements http.FileSystem so that it can be passed to the partial driver.

driver implements the source.Driver interface by providing its Open method.

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.

You can use one struct by not embedding pkging.Pkger.

e.g.

type Pkger struct { httpfs.PartialDriver pkger pkging.Pkger }

It would've been nice if pkging.Pkger implemented http.FileSystem but it turns out in fact it does not.

Looks like it does

Pkger (previously Instance) wraps pkging.Pkger and implements http.FileSystem so that it can be passed to the partial driver.

You can pass Pkger.pkger to Init()

Looks like it does

The signatures are different. It does't return http.File but pkging.File. It's subtle but means

./pkger.go:56:28: cannot use instance.Pkger (type pkging.Pkger) as type http.FileSystem in argument to ds.PartialDriver.Init: pkging.Pkger does not implement http.FileSystem (wrong type for Open method) have Open(string) (pkging.File, error) want Open(string) (http.File, error)

What am I missing? I still don't see how it would work by simply not embedding pkging.Pkger.

If we want Pkger to be the driver and look like

type Pkger struct { httpfs.PartialDriver pkger pkging.Pkger }

I'm not against that but I think we need to wrap Pkger.pkger to implement http.FileSystem. Another approach I can think of is to have

type fsFunc func(name string) (http.File, error) func (f fsFunc) Open(name string) (http.File, error) { return f(name) }

and then wrap pkging.Pkger just before calling Init like so

fs := fsFunc(func(name string) (http.File, error) { f, err := instance.pkger.Open(name) if err != nil { return nil, err } return f.(http.File), nil }) if err := instace.Init(fs, instance.Path); err != nil {

What about that?

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.Pkger in http.FileSystem. It might make sense to just put it on the user to wrap it themselves and instead use httpfs.New directly. What do you think about that?

For packr.Box, the other packager, it should be even easier since it alreay implements http.FileSystem, so the user can pass it straight in.

The signatures are different. It does't return http.File but pkging.File It's subtle but means ...

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 what type Pkger struct looks like as long as the fields aren't exported.

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.Pkger in http.FileSystem. It might make sense to just put it on the user to wrap it themselves and instead use httpfs.New directly. What do you think about that?

If you're only going to use migrate as a Go library, then httpfs.New() is probably the easiest and quickest route to get started. However, if you want to use DB URIs and the migrate CLI, you'll need to create a driver using the httpfs.PartialDriver and register it. You can't use httpfs.New() for this since doing so will hardcode the driver to the http.FileSystem specified via httpfs.New().

I'd recommend having CLI support to debug and fix any issues you have with your migrations

However, if you want to use DB URIs and the migrate CLI, you'll need to create a driver using the httpfs.PartialDriver and register it. You can't use httpfs.New() for this since doing so will hardcode the driver to the http.FileSystem specified via httpfs.New().

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_bindata it doesn't implement CLI support and as you can see I just did the same by leaving source.Driver.Open unimplemented. Personally I only use migrate as a library.

That said, consumers of pkger mostly register resources in a global instance of pkging.Pkger using pkger.Apply and then access files using package scoped functions pkger.Open. This global instance it not, as far as I can tell, accessible. It seems like a good idea to use source.Driver.Open to return a driver that reads from the global pkging.Pkger instance. It would allow library users to opt for either WithInstance and their own instance of pkging.Pkger or for Open after registering migrations on the global instance with pkger.Apply. The pkger CLI 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:///migrations will read migrations from the relative directory /migrations using the package scoped pkger.Open i.e. using a global instance of pkging.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?

dhui · 2020-04-18T09:02:38Z

source/pkger/pkger.go

+}
+
+// Open implements http.FileSystem.
+func (p *Instance) Open(name string) (http.File, error) {


Open() should call Init()

Does this still apply after reading the explanation of why there are two structs?

dhui · 2020-04-18T09:09:18Z

source/pkger/pkger.go

+
+	fs = p
+
+	if err := ds.Init(fs, p.Path); err != nil {


Init() should be called with an instance of pkging.Pkger

Does this still apply after reading the explanation of why there are two structs?

pkging.Pkger does not implement http.FileSystem. Do you mean to call Init with *Pkger (previously *Instance)? It is in fact already doing that. If it's unclear, perhaps I could remove

var fs http.FileSystem fs = instance

and pass instance directly to Init. Would that make it clearer?

dhui · 2020-04-18T12:42:09Z

source/pkger/pkger.go

+	return f.(http.File), nil
+}
+
+type driver struct {


You can use one struct by not embedding pkging.Pkger.

e.g.

type Pkger struct { httpfs.PartialDriver pkger pkging.Pkger }

It would've been nice if pkging.Pkger implemented http.FileSystem but it turns out in fact it does not.

Looks like it does

Pkger (previously Instance) wraps pkging.Pkger and implements http.FileSystem so that it can be passed to the partial driver.

You can pass Pkger.pkger to Init()

dhui · 2020-04-20T00:44:12Z

source/pkger/pkger.go

+	}
+
+	// wrap pkger to implement http.FileSystem.
+	fs := fsFunc(func(name string) (http.File, error) {


Should be able to refactor this to return WithInstance(pkger, u.Path). Based on the pkgr.Open(), we'd probably have to use here.Current().

This is not a blocker for this PR since the difference in complexity is arugable...
e.g. the setup required to use WithInstance() may not be worth the duplicated code

Lemme know if you don't think this refactor is worth it and I'll go ahead and merge the PR as is.

I say go ahead and merge it as is. I'm not 100% sure, but if I change my mind I'll go ahead and contribute a refactor :)

Thanks!

A1Liu · 2020-05-22T20:59:03Z

How do I use this PR? I read through pkger_test.go and still can't figure out how to add a directory of migrations to pkger and use it as a source for migrate.

hnnsgstfssn · 2020-05-23T10:23:16Z

@A1Liu it depends on how you're using Pkger.

Here is one example

package main

import (
	"errors"
	"log"

	"github.com/golang-migrate/migrate/v4"
	"github.com/markbates/pkger"

	_ "github.com/golang-migrate/migrate/v4/database/postgres"
	_ "github.com/golang-migrate/migrate/v4/source/pkger"
	_ "github.com/lib/pq"
)

func main() {
	pkger.Include("/module/path/to/migrations")
	m, err := migrate.New("pkger:///module/path/to/migrations", "postgres://postgres@localhost/postgres?sslmode=disable")
	if err != nil {
		log.Fatalln(err)
	}
	if err := m.Up(); errors.Is(err, migrate.ErrNoChange) {
		log.Println(err)
	} else if err != nil {
		log.Fatalln(err)
	}
}

Does that help? Let me know if there is anything that needs explaining.

A1Liu · 2020-05-23T17:28:48Z

Thank you! That's exactly what I needed.

hinupurthakur · 2021-10-28T13:21:37Z

@hnnsgstfssn

@A1Liu it depends on how you're using Pkger.

Here is one example

package main

import (
	"errors"
	"log"

	"github.com/golang-migrate/migrate/v4"
	"github.com/markbates/pkger"

	_ "github.com/golang-migrate/migrate/v4/database/postgres"
	_ "github.com/golang-migrate/migrate/v4/source/pkger"
	_ "github.com/lib/pq"
)

func main() {
	pkger.Include("/module/path/to/migrations")
	m, err := migrate.New("pkger:///module/path/to/migrations", "postgres://postgres@localhost/postgres?sslmode=disable")
	if err != nil {
		log.Fatalln(err)
	}
	if err := m.Up(); errors.Is(err, migrate.ErrNoChange) {
		log.Println(err)
	} else if err != nil {
		log.Fatalln(err)
	}
}

Does that help? Let me know if there is anything that needs explaining.

I am using sqlite db, therefore my code structure looks something like this:

func migrationUp(db *sqlx.DB) error {
	pkger.Include("/pkg/db/migrations/")

	driver, err := sqlite3.WithInstance(db.DB, &sqlite3.Config{})
	if err != nil {
		return fmt.Errorf("creating sqlite3 db driver failed %s", err)
	}

	m, err := migrate.NewWithDatabaseInstance(
		"pkger:///pkg/db/migrations/",
		"sqlite3", driver)

	if err != nil {
		return fmt.Errorf("initializing db migration failed %s", err)
	}
	if err := m.Up(); err != nil && err != migrate.ErrNoChange {
		return fmt.Errorf("migrating database failed %s", err)
	}
	return nil
}

But I am getting this error,

initializing db migration failed failed to init driver with relative path "/pkg/db/migrations/": here.Package: command-line-arguments: here.cache: command-line-arguments: [go list -json -find command-line-arguments] exit status 1: go: go.mod file not found in current directory or any parent directory; see 'go help modules'

Can you please help here? Thanks in advance.

hnnsgstfssn · 2021-10-28T14:17:12Z

@hinupurthakur it looks like you are not using modules or have structured your code such that Pkger is unable to locate the module root, as indicated by the error message: go.mod file not found in current directory or any parent directory. Pkger requires the use of modules.

This is the structure of the above minimal example with your path applied

tree
.
├── go.mod
├── go.sum
├── main.go
└── pkg
    └── db
        └── migrations
            ├── 000001_init.down.sql
            └── 000001_init.up.sql

created with

main='package main

import (
        "errors"
        "log"

        "github.com/golang-migrate/migrate/v4"
        "github.com/markbates/pkger"

        _ "github.com/golang-migrate/migrate/v4/database/postgres"
        _ "github.com/golang-migrate/migrate/v4/source/pkger"
        _ "github.com/lib/pq"
)

func main() {
        pkger.Include("/pkg/db/migrations")
        m, err := migrate.New("pkger:///pkg/db/migrations", "postgres://postgres@localhost/postgres?sslmode=disable")
        if err != nil {
                log.Fatalln(err)
        }
        if err := m.Up(); errors.Is(err, migrate.ErrNoChange) {
                log.Println(err)
        } else if err != nil {
                log.Fatalln(err)
        }
}';
cd /tmp;
mkdir x;
cd x;
mkdir -p pkg/db/migrations;
go mod init x;
go run github.com/golang-migrate/migrate/v4/cmd/migrate@master create -ext sql -dir /tmp/x/pkg/db/migrations -seq init;
echo $main >main.go;
go mod tidy;
docker run --rm -it --net host -d postgres:12.1-alpine;
sleep 3; : # allow db to start
go run .;
tree;

Hope that is helpful, but let me know if it still does not make sense.

hnnsgstfssn · 2021-10-28T14:31:16Z

I would recommend to use package embed from the standard library instead of pkger if possible.

package main

import (
	"embed"
	"errors"
	"log"
	"net/http"

	"github.com/golang-migrate/migrate/v4"
	_ "github.com/golang-migrate/migrate/v4/database/postgres"
	"github.com/golang-migrate/migrate/v4/source/httpfs"
	_ "github.com/lib/pq"
)

//go:embed pkg/db/migrations
var migrations embed.FS

func main() {
	sourceInstance, err := httpfs.New(http.FS(migrations), "pkg/db/migrations")
	if err != nil {
		log.Fatalln(err)
	}
	m, err := migrate.NewWithSourceInstance("httpfs", sourceInstance, "postgres://postgres@localhost/postgres?sslmode=disable")
	if err != nil {
		log.Fatalln(err)
	}
	if err := m.Up(); errors.Is(err, migrate.ErrNoChange) {
		log.Println(err)
	} else if err != nil {
		log.Fatalln(err)
	}
}

hinupurthakur · 2021-10-29T06:08:29Z

main >main.go;
go mod tidy;

My requirement is to have a single build file for the project without any dependency. The binary works fine if it is in the project but as soon as I move it somewhere else it shows it's dependency on go.mod

hinupurthakur · 2021-10-29T06:08:42Z

I would recommend to use package embed from the standard library instead of pkger if possible.

package main

import (
	"embed"
	"errors"
	"log"
	"net/http"

	"github.com/golang-migrate/migrate/v4"
	_ "github.com/golang-migrate/migrate/v4/database/postgres"
	"github.com/golang-migrate/migrate/v4/source/httpfs"
	_ "github.com/lib/pq"
)

//go:embed pkg/db/migrations
var migrations embed.FS

func main() {
	sourceInstance, err := httpfs.New(http.FS(migrations), "pkg/db/migrations")
	if err != nil {
		log.Fatalln(err)
	}
	m, err := migrate.NewWithSourceInstance("httpfs", sourceInstance, "postgres://postgres@localhost/postgres?sslmode=disable")
	if err != nil {
		log.Fatalln(err)
	}
	if err := m.Up(); errors.Is(err, migrate.ErrNoChange) {
		log.Println(err)
	} else if err != nil {
		log.Fatalln(err)
	}
}

Okay I will try with embed then

dhui requested changes Apr 18, 2020

View reviewed changes

hnnsgstfssn added 2 commits April 18, 2020 12:21

pkger: rename Instance to Pkger

655a795

pkger: make WithInstance accept *Pkger

c177e4a

hnnsgstfssn requested a review from dhui April 18, 2020 12:05

dhui requested changes Apr 18, 2020

View reviewed changes

hnnsgstfssn requested a review from dhui April 18, 2020 14:45

hnnsgstfssn added 2 commits April 19, 2020 23:21

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

9afbbbf

pkger: fix typo and cleanup debug logging

8ae7cd4

dhui approved these changes Apr 20, 2020

View reviewed changes

dhui merged commit f5a22be into golang-migrate:master Apr 21, 2020

hnnsgstfssn mentioned this pull request May 24, 2020

source/pkger: add readme #397

Merged

renovate bot mentioned this pull request May 22, 2021

fix(deps): update module github.com/golang-migrate/migrate to v4 - autoclosed advenjourney/advenjourney#20

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pkger source driver support #377

Add pkger source driver support #377

hnnsgstfssn commented Apr 18, 2020

coveralls commented Apr 18, 2020 •

edited

Loading

dhui left a comment

dhui Apr 18, 2020

hnnsgstfssn Apr 18, 2020

dhui Apr 18, 2020

hnnsgstfssn Apr 18, 2020

dhui Apr 18, 2020

hnnsgstfssn Apr 18, 2020

dhui Apr 18, 2020

hnnsgstfssn Apr 18, 2020

hnnsgstfssn Apr 18, 2020

dhui Apr 18, 2020

hnnsgstfssn Apr 19, 2020 •

edited

Loading

dhui Apr 18, 2020

hnnsgstfssn Apr 18, 2020

dhui Apr 18, 2020

hnnsgstfssn Apr 18, 2020

dhui Apr 18, 2020

dhui Apr 20, 2020

hnnsgstfssn Apr 21, 2020

A1Liu commented May 22, 2020

hnnsgstfssn commented May 23, 2020

A1Liu commented May 23, 2020

hinupurthakur commented Oct 28, 2021

hnnsgstfssn commented Oct 28, 2021

hnnsgstfssn commented Oct 28, 2021

hinupurthakur commented Oct 29, 2021

hinupurthakur commented Oct 29, 2021

Add pkger source driver support #377

Add pkger source driver support #377

Conversation

hnnsgstfssn commented Apr 18, 2020

coveralls commented Apr 18, 2020 • edited Loading

Pull Request Test Coverage Report for Build 755

💛 - Coveralls

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hnnsgstfssn Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A1Liu commented May 22, 2020

hnnsgstfssn commented May 23, 2020

A1Liu commented May 23, 2020

hinupurthakur commented Oct 28, 2021

hnnsgstfssn commented Oct 28, 2021

hnnsgstfssn commented Oct 28, 2021

hinupurthakur commented Oct 29, 2021

hinupurthakur commented Oct 29, 2021

coveralls commented Apr 18, 2020 •

edited

Loading

hnnsgstfssn Apr 19, 2020 •

edited

Loading