-
-
Notifications
You must be signed in to change notification settings - Fork 525
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
Allow the Postgres schema for the migration table to be specified through migrationStorageTableSchema (#265) #635
Conversation
Unsure what's causing the schema to not be created here, both on my local PostgreSQL instance and a Docker instance, all of the tests pass. |
Hi, pinging to try to get some more information. Still unsure why it's failing on TravisCI, since I have tests passing locally off a fresh Postgres instance bootstrapped by the Docker Compose configuration. Any ideas? |
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.
Some changes that I think will help fix CI but overall new API looks good
src/core/migrator.js
Outdated
if (helpers.version.getDialectName() === 'pg') { | ||
const customSchemaName = helpers.umzug.getSchema('migration'); | ||
if (customSchemaName && customSchemaName !== 'public') { | ||
return sequelize.showAllSchemas().then(schemas => { |
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.
Use createSchema
directly which handles existing schema with IF NOT EXISTS
https://github.com/sequelize/sequelize/blob/aa92764f953eeefbf194a6e2b765552adfa73bda/lib/query-interface.js#L35
test/db/migrate.test.js
Outdated
|
||
describe('custom meta schema', () => { | ||
it('correctly uses the defined schema', function (done) { | ||
const self = this; |
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.
self = this
not required with arrow functions
test/support/index.js
Outdated
|
||
// If Postgres, loop through each of the non-public schemas and DROP/re-CREATE them. | ||
if (this.dialectIsPostgres()) { | ||
sequelize |
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 think tests are failing because of missing return
test/support/index.js
Outdated
let searchPathSchemas = schemas.join(', '); | ||
let promise = Bluebird.resolve(); | ||
|
||
_.forEach(schemas, function (schema) { |
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.
Use Promise.map
+ Promise.all
combination rather than forEach
for better readability
test/support/index.js
Outdated
}); | ||
|
||
// Drop the public schema tables. | ||
promise.then(dropAllTables); |
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.
return the promise
@sushantdhiman, thanks for your comments, updated the PR with fixes. Ran the tests locally and they pass under the same node and npm versions, the main difference is that I'm running PostgreSQL 10.1. I can't think of why that would cause the failure. The other possibility is a PG permission/role issue with the created schema? |
@kpeng Yeah I ran your PR locally and tests pass properly with Postgres 9.5 as well. Looks like permission issue with schema creation |
@kpeng Please rebase this branch with |
Thanks, @sushantdhiman, that seemed to have worked! |
Thanks @kpeng , Released as |
This change drops support for Go1.8 and sets the minimum to Go1.9. By making this change we are able to use the same testing steps on *nix and Windows. Travis and Appveyor build scripts have been updated to reflect the said testing step.
Thanks to @jonnolen's original PR for getting me started on this. Added a test that injects
migrationStorageTableSchema
to a test config and ensures that the non-public schema is created correctly.