Allow passing a handle instead of url connection string

[danilobuerger]

As discussed in https://github.com/gemnasium/migrate/issues/24 we should have the ability to pass a handle instead of an url connection string. However, we can not just assume *sql.DB as we have other nosql drivers like cassandra and bash.

[danilobuerger]

We probably have two viable options:

1. interface{}

We could make methods like func UpSync(url, migrationsPath string) (err []error, ok bool) be func UpSync(handle interface{}, migrationsPath string) (err []error, ok bool) instead and pass that to the driver who need to then do a type assertion.

2. go via driver

Most likely any one project using migrate are using just one type of driver. I don't think many projects exists where you would have e.g. migrations for postgres and cassandra in the same project. They would probably be separate. So we might get away with not going through migrate as a registry to reach the driver but to just talk to the driver directly which in turn uses common functionality from the migrate library. So we would expose the driver to the developer and have migrate as a dependency to the driver instead of the other way around.

[gravis]

Thanks for clarifying this!

I agree 100% on point 2.

So to sum up:

• it's the responsibility of the cli to fetch and instantiate the right driver.
• Functions will pass a driver instead of a URL, so instead of having func UpSync(url, migrationsPath string) (err []error, ok bool), we would have func UpSync(d driver, migrationsPath string) (err []error, ok bool)
• if more than one driver is used, it's the responsibility of the cli to call migrate func for each of them. Running migrations with several drivers at once seems pretty awkward anyway, and as you said unlikely :)
• This will help with Remove drivers from tests #1 as well, since we can create a fake driver of our tests.

Are we on the same page?

[danilobuerger]

Well, that also seems like a good solution but api wise I meant something different. What I meant was:

Instead of having migrate.UpSync("postgres://..", "./migrations") I was proposing postgres.UpSync(db, "./migrations") which then uses internally common functionality from the migrate package.

For the cli either way works fine as this is not exposed to the user. However, if they are using the library, a sample open & migrate snippet might look like so:

migrate: func UpSync(d driver, migrationsPath string) (err []error, ok bool)

func OpenAndMigrate(dsn, migrationsPath string) (*sql.DB, error) {
	db, err := sql.Open("postgres", dsn)
	if err != nil {
		return nil, err
	}

	if err = db.Ping(); err != nil {
		return nil, err
	}

	driver := &postgres.Driver{}
	driver.SetDB(db)

	errors, _ := migrate.UpSync(driver, migrationsPath)
	if len(errors) > 0 {
		return nil, errors[0]
	}

	return db, nil
}

driver: func UpSync(db *sql.DB, migrationsPath string) (err []error, ok bool)

func OpenAndMigrate(dsn, migrationsPath string) (*sql.DB, error) {
	db, err := sql.Open("postgres", dsn)
	if err != nil {
		return nil, err
	}

	if err = db.Ping(); err != nil {
		return nil, err
	}

	errors, _ := postgres.UpSync(db, migrationsPath)
	if len(errors) > 0 {
		return nil, errors[0]
	}

	return db, nil
}

[danilobuerger]

Obviously with your api, we could also introduce a func New(...) driver.Driver on each driver and use it like so:

func OpenAndMigrate(dsn, migrationsPath string) (*sql.DB, error) {
	db, err := sql.Open("postgres", dsn)
	if err != nil {
		return nil, err
	}

	if err = db.Ping(); err != nil {
		return nil, err
	}

	errors, _ := migrate.UpSync(postgres.New(db), migrationsPath)
	if len(errors) > 0 {
		return nil, errors[0]
	}

	return db, nil
}

[danilobuerger]

In any case, since we are reworking the API, I have some more issues with it that we could consider:

• Migrate (or driver) funcs should just return an error instead of (err []error, ok bool). Nobody needs ok, as (len(err) == 0) == ok. Also, I want to get rid of []error, that's not idiomatic. Instead, we could add an exported error type that users can type assert to to get all errors. If they don't, they just get the first one.

• Swap sync and async. UpSync would become Up and Up would become UpWithChan or UpAsync or whatever. I would bet that far more users use the sync funcs instead of the channel ones.

• Integrate a logger. We should really allow passing along a logger to all those migrate methods so these can actually log the migration outcome of each file if a logger is supplied.

[mattes]

All the points here are very valid. I addressed some in the ticket that was referenced above.

Integrate a logger. We should really allow passing along a logger to all those migrate methods so these can actually log the migration outcome of each file if a logger is supplied.

v3 passes in an Logger interface. @danilobuerger
https://github.com/mattes/migrate/blob/v3.0-prev/log.go#L5

[gravis]

That's a lot of subjects in the same issue :)

I'm fine with the changes you're proposing.
Also I agree on this:

• Migrate should return an error instead of a list of errors. The first failure gets returned upwards until handled, like any other go program. This will also simplify a lot the code.
• UpSync would become Up, and Up should become UpAsync`.
• Logger: What do you have in mind? Let's open a different for that specific need, ok? We're using logrus, whereas others are using glog. The two don't share a common interface, so it's beyond the scope of this issue :)

Thanks for your help!

[danilobuerger]

Alright, I hope I can get some work done on the weekend.

[josephbuchma]

Hi guys! Any progress on this (on issue subject, and anything discussed above) ?

[josephbuchma]

Also, maybe I've missed something, but I don't see any rationale behind "async" functions and "piping".
Unlike mattes/migrate, this library provides pretty much stateless API, so it seems like async stuff here only adds extra complexity without providing much value.

[gravis]

Hi, the async and piping things were coming from the original mattes/migrate code.
This project is basically a fork of this code. We could get rid of these parts, feel free to propose some pull requests too.

[josephbuchma]

Forking, updating and opening PRs for each driver repo separately sucks (when you edit all of them at the same time 😄 )
Other problem is CI - all buids are failing as it pulls masters of dependencies