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

Fix migration issue #81

Merged
merged 8 commits into from
Nov 3, 2016
Merged

Fix migration issue #81

merged 8 commits into from
Nov 3, 2016

Conversation

harshkothari410
Copy link
Contributor

Here I removed the checkDB functionality into postinstall and added to prestart. So that there is no issue of private space heroku migration / resetdb issue.

There will be little bit overhead on npm start but it results in lots of simplicity in automatic migration and deployment of new refocus on any platform.

models.sequelize.query(`select count(*) from
information_schema.tables where table_schema = 'public'`)
.then((data) => {
console.log('found', data)
console.log('found', data); // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

I added this when troubleshooting migration issues a while back. We don't need to keep this console.log anymore either, unless it's useful to you for migration troubleshooting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this as well.

const dbConfig = utils.dbConfigObjectFromDbURL();
console.log('create db now', dbConfig)
console.log('create db now', dbConfig); // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to keep this console.log anymore either, unless it's useful to you for migration troubleshooting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okies sure I am removing this.

@iamigo
Copy link
Contributor

iamigo commented Nov 2, 2016

@harshkothari410 does this fix the migration issues @meynet hit in heroku?

@harshkothari410
Copy link
Contributor Author

Yeah I tested on Heroku so I am assuming that it is fixed.

@pallavi2209
Copy link
Contributor

@harshkothari410 Resolve conflicts please.
Not related to this PR, but just checking, can we run sequelize.sync({ force: false }) now without problems (related to: when Ian ran into problems when he wanted to run db sync for new table creation) @iamigo

@harshkothari410
Copy link
Contributor Author

Is there any specific script we have for sequelize.sync({ force: false }) ? @pallavi2209

@iamigo
Copy link
Contributor

iamigo commented Nov 3, 2016

No. We need to add a call to sequelize sync from the doImport function in db/index.js.

@pallavi2209
Copy link
Contributor

Nevermind, I noticed a related story on Ian's dashboard for this sprint. Can you resolve conflicts in package.json for this PR? @harshkothari410

@iamigo
Copy link
Contributor

iamigo commented Nov 3, 2016

@harshkothari410 please let me know if you need help resolving the package.json conflicts so we can get this merged.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 83.538% when pulling 3524662 on W-3422404 into 0bbd8ff on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 83.545% when pulling aaf519e on W-3422404 into 0c5678a on master.

@pallavi2209 pallavi2209 merged commit d3dded2 into master Nov 3, 2016
@pallavi2209 pallavi2209 deleted the W-3422404 branch November 3, 2016 23:55
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