Migrations

[ajyang818]

This moves boxplot over to migrations.

If I'm understanding the documentation correctly, there are two disadvantages with db.sync({force: true/false}):

• If force is false, it doesn't reflect changes to existing tables (unless the table didn't exist before)
• If force is true, it wipes all data

The initial migration (the first real initial migration, not the blank one) creates a db based off of my current local db using SQL commands and some code I found online. I believe Sequelize then picks up the relationships in db.js.

@danielsuo @yjkogan @michaelsuo thoughts?

@danielsuo we will also need to add in production credentials in config/config.json in order to get Sequelize to apply the migrations on the live app.

[danielsuo]

Thanks for taking this on. We definitely want to use migrations. Once we finalize this, we should send out a note so that everyone reads your note on what migrations are / how to use.

[ajyang818]

Ok, great that this is needed!

Assigning to you ;). When you look it over, could you also try setting up the db and running migrations from scratch? Hopefully we can make the docs as painless as possible for others

[danielsuo]

I'll try to get to this tomorrow and will likely also reassign to yoni for further review.

[ajyang818]

I think this is a quasi-bottleneck for other features, so @yjkogan could you review in parallel?

[yjkogan]

@ajyang818 Can you explain what your real migration does / is meant to do? How could i test/try it? This migration looks very odd to me because it looks like the up is to create all tables and the down is to drop them. Are we basically making the initialization into a migration?

[ajyang818]

Yes, exactly. This was created by getting the dump from my existing postgres db state, and isolating the SQL commands that create the tables and their appropriate columns.

To test/try it, I would rename your existing boxplot db to something else (so that you have the existing data as backup if you want it), and then start from createdb boxplot, and then run the migration command (I think).

[danielsuo]

Question: we can do migration on a db that wasn't set up via migration right? I'm almost positive this is the case, but just want to check.

Second question: does the migration assuming a new database work for an existing database as well, ie, changes that don't need to be made are ignored and only incremental changes are made? (would be easier to have one migration file rather than a series of diffs)

On Tue, May 27, 2014 at 12:35 PM, Allen Yang notifications@github.comwrote:

Yes, exactly. This was created by getting the dump from my existing postgres db state, and isolating the SQL commands that create the tables and their appropriate columns.

To test/try it, I would rename your existing boxplot db to something else (so that you have the existing data as backup if you want it), and then start from createdb boxplot, and then run the migration command (I think).

— Reply to this email directly or view it on GitHubhttps://github.com/thelac/boxplot/pull/62#issuecomment-44301069 .

[ajyang818]

Yes, we can run a migration on the existing DB, but it's a good question because we'll have to 'trick' sequelize into thinking the migrations in this PR were already run, or otherwise sequelize -m will try to run all migrations the first time. I will look into how to do this, but it should be built in...

I think the migration attempts to run the entire migration file (if it doesn't have a record that that migration was run before). That means it'll try to create tables that already exist, and columns that already exist. I'm not sure what happens in sql when that happens (I imagine the naming conflict will block it, but it might do something weird)

[danielsuo]

Valentina Studio seems great for exploring the databases via GUI. Recommend others install. Thanks to Allen for the find!

(I was actually installing something similar to phpmyadmin and then actually read Allen's documentation...)

[danielsuo]

@ajyang818 migrations do work on existing DBs (I suppose this is why they exist).

Running migrations that create tables that already exist fails. I'm going to look into the sequelize documentation if there's a way to skip lines that create errors (a la fuckit.js), but really, we should be specifying the incremental migrations we want to do rather than running all of them.

When are you back today????

[ajyang818]

I'm back already! At work right now.

I don't think we should implement the skip method; that sounds dangerous going forward. I think the incremental vs. all problem is only a problem the first time; Sequelize ought to keep track of which migrations have been applied to the db, so in the future it'll just pick up new migrations to apply. We just have to figure out how Sequelize stores this information, and then let it know we've already taken care of that first big migration.

[danielsuo]

Sequelize -m is kind a 'dumb' function in the sense it just runs whatever js files are in the top level of the migrations folder.

I'm not sure what the right workflow is, but I archived the initialization migrations to migrations/migrated. Really, we shouldn't have to manually move migration files after a migration -- any suggestions?

Otherwise, this looks good to me.

[ajyang818]

No, I don't think that's right. Sequelize has code in the background to keep track of when the last time migrations were run, and upon calling sequelize -m it seems to look for any migration files created within a certain time window. See https://github.com/sequelize/sequelize/blob/master/lib/migrator.js#L88 and https://github.com/sequelize/sequelize/blob/master/lib/migrator.js#L225

I think this information is stored in the SequelizeMeta table (which is automatically created upon initializing migrations, I believe). I haven't had a chance to look further into it, but I'm pretty sure it is absolutely not right to manually migrate old files to a different folder (and in fact, this will cause lots of problems if/when we need to migrate down)

[ajyang818]

Another reference: https://github.com/sequelize/sequelize/issues/684

[danielsuo]

Ok, great. I'm glad you know something about this because it felt really wrong.

If this is the case, Allen, could you help improve the instructions so that we can run the migration on an existing database without issues?

My guess is we'd have to populate the SequelizeMeta table (and at this point, I'd have to drop my SequelizeMeta table because it's probably so eff'd) or something, but let us know!

On Wed, May 28, 2014 at 3:38 PM, Allen Yang notifications@github.comwrote:

No, I don't think that's right. Sequelize has code in the background to keep track of when the last time migrations were run, and upon calling sequelize -m it seems to look for any migration files created within a certain time window. See https://github.com/sequelize/sequelize/blob/master/lib/migrator.js#L88and https://github.com/sequelize/sequelize/blob/master/lib/migrator.js#L225

I think this information is stored in the SequelizeMeta table (which is automatically created upon initializing migrations, I believe). I haven't had a chance to look further into it, but I'm pretty sure it is absolutely not right to manually migrate old files to a different folder (and in fact, this will cause lots of problems if/when we need to migrate down)

— Reply to this email directly or view it on GitHubhttps://github.com/thelac/boxplot/pull/62#issuecomment-44454360 .

[ajyang818]

Yep, will look into it. Reading all this makes me suspect that the solution is just as easy as copying a correct SequelizeMeta table over, but will poke around more to make sure.

[danielsuo]

The solution for us locally seems pretty clear: drop all the tables and run the migration from scratch. Just not sure how to migrate a db that already has stuff, but no sequelizemeta table.

I can't think of anything except dumping the sequelizemeta table and creating it on the production server (much like the boxplot_session stuff).

On Wed, May 28, 2014 at 3:52 PM, Allen Yang notifications@github.comwrote:

Yep, will look into it. Reading all this makes me suspect that the solution is just as easy as copying a correct SequelizeMeta table over, but will poke around more to make sure.

— Reply to this email directly or view it on GitHubhttps://github.com/thelac/boxplot/pull/62#issuecomment-44455904 .

[ajyang818]

@danielsuo and @yjkogan, that should do it. See the instructions in the new docs file for how to test this locally!

[yjkogan]

Will try to look at this tomorrow or Friday. Sorry for the delay!

[ajyang818]

Wait, did you merge this while we were all drunk??

Also, did the commands run on production ok?

On Fri, Jun 13, 2014 at 11:04 PM, Daniel Suo notifications@github.com wrote:

Merged #62 https://github.com/thelac/boxplot/pull/62.

— Reply to this email directly or view it on GitHub https://github.com/thelac/boxplot/pull/62#event-131388118.

[kevinakwok]

Is this our 'oh god what did I merge last night' moment?

On Sat, Jun 14, 2014 at 2:18 PM, Allen Yang notifications@github.com wrote:

Wait, did you merge this while we were all drunk?? Also, did the commands run on production ok? On Fri, Jun 13, 2014 at 11:04 PM, Daniel Suo notifications@github.com wrote:

Merged #62 https://github.com/thelac/boxplot/pull/62.

— Reply to this email directly or view it on GitHub https://github.com/thelac/boxplot/pull/62#event-131388118.

---

Reply to this email directly or view it on GitHub: https://github.com/thelac/boxplot/pull/62#issuecomment-46095504

[danielsuo]

Amazing github conversation. We haven't run on production yet, but I was able to get it on my dev environment. We didn't merge in before for no good reason.

On Sat, Jun 14, 2014 at 2:20 PM, kevinakwok notifications@github.com wrote:

Is this our 'oh god what did I merge last night' moment?

On Sat, Jun 14, 2014 at 2:18 PM, Allen Yang notifications@github.com wrote:

Wait, did you merge this while we were all drunk?? Also, did the commands run on production ok? On Fri, Jun 13, 2014 at 11:04 PM, Daniel Suo notifications@github.com wrote:

Merged #62 https://github.com/thelac/boxplot/pull/62.

— Reply to this email directly or view it on GitHub https://github.com/thelac/boxplot/pull/62#event-131388118.

---

Reply to this email directly or view it on GitHub: https://github.com/thelac/boxplot/pull/62#issuecomment-46095504

— Reply to this email directly or view it on GitHub https://github.com/thelac/boxplot/pull/62#issuecomment-46095556.

[yjkogan]

+1 to dan