Skip to content
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

Merged
merged 9 commits into from
Oct 14, 2018

Conversation

kpeng
Copy link
Contributor

@kpeng kpeng commented Mar 26, 2018

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.

@kpeng kpeng changed the title Allow the Postgres schema for the migration table to be specified through migrationStorageTableSchemma (#265) Allow the Postgres schema for the migration table to be specified through migrationStorageTableSchema (#265) Mar 26, 2018
@kpeng
Copy link
Contributor Author

kpeng commented Mar 26, 2018

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.

@kpeng
Copy link
Contributor Author

kpeng commented Apr 6, 2018

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?

Copy link
Contributor

@sushantdhiman sushantdhiman left a 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

if (helpers.version.getDialectName() === 'pg') {
const customSchemaName = helpers.umzug.getSchema('migration');
if (customSchemaName && customSchemaName !== 'public') {
return sequelize.showAllSchemas().then(schemas => {
Copy link
Contributor

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


describe('custom meta schema', () => {
it('correctly uses the defined schema', function (done) {
const self = this;
Copy link
Contributor

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


// If Postgres, loop through each of the non-public schemas and DROP/re-CREATE them.
if (this.dialectIsPostgres()) {
sequelize
Copy link
Contributor

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

let searchPathSchemas = schemas.join(', ');
let promise = Bluebird.resolve();

_.forEach(schemas, function (schema) {
Copy link
Contributor

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

});

// Drop the public schema tables.
promise.then(dropAllTables);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return the promise

@kpeng
Copy link
Contributor Author

kpeng commented Sep 20, 2018

@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?

@sushantdhiman
Copy link
Contributor

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

@sushantdhiman
Copy link
Contributor

@kpeng Please rebase this branch with master, We are now using docker setup for CI as well, let's see if that works

@kpeng
Copy link
Contributor Author

kpeng commented Oct 13, 2018

Thanks, @sushantdhiman, that seemed to have worked!

@sushantdhiman sushantdhiman merged commit 2a17c78 into sequelize:master Oct 14, 2018
@sushantdhiman
Copy link
Contributor

Thanks @kpeng , Released as v5.1.0

codetriage-readme-bot pushed a commit to codetriage-readme-bot/cli that referenced this pull request Jun 5, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants