Add type MigrationSet to allow concurrent usage of sql-migrate #156
Conversation
migrate_test.go
Outdated
| Migrations: sqliteMigrations[:1], | ||
| } | ||
|
|
||
| ms := NewMigrationSet("", "") |
There was a problem hiding this comment.
Perhaps we should use a custom table name in this test?
| // Shouldn't apply migration again | ||
| n, err = ms.Exec(s.Db, "sqlite3", migrations, Up) | ||
| c.Assert(err, IsNil) | ||
| c.Assert(n, Equals, 0) |
There was a problem hiding this comment.
And then here do a direct query on the custom table name to verify that it actually wrote the migration record to that table?
This way we're validating that the supplied parameters actually work (and keep working in future releases).
There was a problem hiding this comment.
I will do the both to verify the default value
migrate.go
Outdated
| type MigrationSet struct { | ||
| // Name of the table used to store migration info. | ||
| tableName string | ||
| // Name of a schema that the migration table be referenced. | ||
| schemaName string | ||
| } | ||
|
|
||
| // NewMigrationSet returns a parametrized Migration object | ||
| func NewMigrationSet(schemaName string, tableName string) MigrationSet { | ||
| if tableName == "" { | ||
| tableName = "gorp_migrations" | ||
| } | ||
| return MigrationSet{ | ||
| tableName: tableName, | ||
| schemaName: schemaName, | ||
| } | ||
| } |
There was a problem hiding this comment.
My only concern here is that an empty MigrationSet is an invalid configuration.
Usually in go the empty value of a struct is a useful one. Perhaps it's better to remove the tableName == "" check here and move it to where the value is actually used.
Once you do that, the members of MigrationSet can become public, which will allow users to use a custom one where they only override the schema name:
MigrationSet{
SchemaName: "myschema",
}
And once we do that, perhaps we don't need NewMigrationSet() anymore? Just use the object initializer.
There was a problem hiding this comment.
To avoid multiple tableName == "", you could define a method MigrationSet to "retrieve the tableName if set, else default".
There was a problem hiding this comment.
Oh and this avoids an important risk in terms of backwards compatibility: suppose we ever want to add a third parameter to MigrationSet. As is currently, with NewMigrationSet(string, string), we'd need to add a third parameter to that function, which would break any code using it. That's not acceptable. Exposing the struct fields avoids that issue.
There was a problem hiding this comment.
Seems legit to me, I'll propose a commit in this direction
|
This makes total sense and doesn't break anything with regards to backwards compatibilty. Love it, thanks already. The CI failures aren't caused by you (see #155), so we can ignore them for now. Added some comments with regards to cleaning it up slightly in terms of usage and improving the test coverage a bit. |
|
Excellent, thanks a lot! Merging! 🍾 |
Usage of global variable in migrate package make difficult the concurrent usage of sql-migrate on two (or more) databases/schema.
This PR introduce a MigrationSet type that provide a way to instanciate sql-migrate with different parameters for databases.
To ensure backward compatibility the global variables (tableName and schemaName) are replaced by a MigrationSet instance and functions without receiver call their MigrationSet counterpart on this instance).