Fix DisableCreateTable for MigrationSet instances.

[peterldowns]

Previously the DisableCreateTable setting was always being read from the global migSet instance. This meant that setting ms.DisableCreateTable on a given var ms MigrationSet instance had no effect.

This commit fixes that bug, and adds a test to confirm the fix. Regardless of the global migSet.DisableCreateTable value, ms.getMigrationDbMap() will always read from ms.DisableCreateTable.

[peterldowns]

This PR adds a test verify that DisableCreateTable = true results in the migrations being applied successfully without a migrations table being created. Unfortunately this test fails:

❯ go test ./...

----------------------------------------------------------------------
FAIL: migrate_test.go:745: SqliteMigrateSuite.TestRunMigrationObjDisableCreateTable

migrate_test.go:755:
    c.Assert(err, IsNil)
... value sqlite3.Error = sqlite3.Error{Code:1, ExtendedCode:1, SystemErrno:0x0, err:"no such table: gorp_migrations"} ("no such table: gorp_migrations")

OOPS: 40 passed, 1 FAILED
--- FAIL: Test (0.01s)
FAIL
FAIL	github.com/rubenv/sql-migrate	0.136s
ok  	github.com/rubenv/sql-migrate/sql-migrate	0.140s [no tests to run]
ok  	github.com/rubenv/sql-migrate/sqlparse	0.193s
FAIL

I believe this has uncovered another bug, which is that DisableCreateTable simply doesn't work. I've added another test which I think shows the problem. When DisableCreateTable = true, the migrations table will (correctly) not be created, but the ms.GetMigrationRecords method always queries this table to try to find which migrations exist:

func (ms MigrationSet) GetMigrationRecords(db *sql.DB, dialect string) ([]*MigrationRecord, error) {
	dbMap, err := ms.getMigrationDbMap(db, dialect)
	if err != nil {
		return nil, err
	}

	var records []*MigrationRecord
	query := fmt.Sprintf("SELECT * FROM %s ORDER BY %s ASC", dbMap.Dialect.QuotedTableForQuery(ms.SchemaName, ms.getTableName()), dbMap.Dialect.QuoteField("id"))
	_, err = dbMap.Select(&records, query)
	if err != nil {
		return nil, err
	}

	return records, nil
}

I can picture three potential ways to fix this problem:

• Remove DisableCreateTable as an option entirely, since it does not work, and it's unlikely that anyone is relying on it working.
• Update GetMigrationRecords to always return nil, nil when DisableCreateTable == true, as if no migrations had ever been applied.
• Update GetMigrationRecords to use a different query that returns nil, nil iff the migrations table doesn't exist, but if it does exist it returns any records from the table.

I don't think I can make this choice for you, but please feel free to build on top of this PR if you'd like to use it to help solve the problem!

[rubenv]

I think the point of disabling the creation of the table is for those cases where you want to manage that table yourself.

However, you do need it to track state (it's not just reading it: after executing a migration it also inserts there).

I wonder what your use case is to run completely without a migration table?

Update GetMigrationRecords to use a different query that returns nil, nil iff the migrations table doesn't exist, but if it does exist it returns any records from the table.

As a general advice: never do things like this, as it will silently hide configuration errors, then do the thing you probably do not want in such a scenario.

[peterldowns]

I think the point of disabling the creation of the table is for those cases where you want to manage that table yourself.

However, you do need it to track state (it's not just reading it: after executing a migration it also inserts there).

OK, I'll update the tests to just confirm that sql-migrate will not attempt to create the table. Thank you for clarifying. I wasn't sure what was the intended behavior with DisableCreateTable = true because I could not find any documentation or discussion of its use.

I wonder what your use case is to run completely without a migration table?

I am working on a library for quickly creating postgres databases for testing. As part of creating a test database, I assume users would like to run their migrations. I have therefore implemented adapters for all of the most popoular golang migration libraries, including an adaptor for sql-migrate.

The sqlmigrator adapter uses a MigrationSet instance to run migrations, which is how I discovered the problem with DisableCreateTable not working.

I personally do not use sql-migrate, and I don't know why someone would want to use DisableCreateTable, I just want to support the use case in which another developer does.

[peterldowns]

@rubenv this PR is now ready for review, it contains the bugfix and tests to confirm it 🥳

[rubenv]

Merged, thanks a lot!

[peterldowns]

@rubenv thank you for reviewing and merging! You may want to close #236 since it also attempted to fix this issue, and is no longer necessary.