Switch to Knex migrations #1154

schuyler1d · 2019-06-22T21:23:29Z

This builds on @harpojaeger's excellent work in #692 and should get us out of the hell of race conditions when creating/destroying the db -- especially during testing runs.

Using knex's native migrations support also should improve usability and predictability (esp. for anyone that is already familiar with knex).

One thing that we'll need to be careful of:
Before merging into main, we probably want to cut a new release (3.0?) with some documentation on how to migrate from an older version that still has migrations between current main and when this PR is merged. In that case, they should update their code to the latest release BEFORE this one, run migrations, and only then, checkout this version (or any future versions). I can add documentation for that, but since it will be release/version-specific, I thought I'd hold off for guidance on placement. If we want, we could skip deletion of the old migrations file, and then have some yarn old-migrations command that you would run. However, that might create confusion going forward if new devs find a src/migrations/index.js and incorrectly update that instead.

From Harpo's changes, this does the following:

updates the schema changes since the PR -- send_before tweaks and campaign.creator_id
removes Harpo's test/server/db/knex.test.js -- this was a useful test while experimenting with the upgrade, but once we switch, the db will be out of date and it makes no sense to maintain the schema both in tests and in migrations (or maybe it does?)
creates the package.json local command yarn knex .... for easy migration
fixes some bugs introduced: (restoring datawarehouse connection and knex.destroy() was not working to remove the existing tables) -- also removes dotenv dependency loading in server/index.js to preserve independence from .env contexts vs. e.g. other testing.
upgrades knex to 0.17.3

cc: @ibrand and @lperson (since he is most likely to create schema change PRs)

…gration"

…ment

transaction still fails because knex can't find the knex_migrations table, which is quite odd this seems to be related to the sql batch query, because it works fine with a normal knex function (like createTableIfNotExists)

…n, so the test doesn't have to be run multiple times

…t not be related

…reserve ordering for complex foreign key dependencies

See travis-ci/travis-ci#5737.

…/travis-ci#5737

…th switching from previous src/migrations/index.js setup

…te DB connection from knex+migrations etc

ibrand

I want us to outline a step by step gameplan for the order of operations with rolling this out re:

Before merging into main, we probably want to cut a new release (3.0?) with some documentation on how to migrate from an older version that still has migrations between current main and when this PR is merged.

Here are a few guiding questions:

This is not backwards compatible, so what are the exact steps that folks would need to take after they git pull to get fully onto this new system
Will this influence anyone's current dev envs if they pull assuming they aren't doing a migration right now?
How does migrations/20190207220000_init_db.js fit into this transition?

Awesome work @schuyler1d and @harpojaeger I'm really excited about this, and want to make sure we merge it as seamlessly as we can!

ibrand · 2019-07-10T16:04:09Z

__test__/server/db/utils.js

+  'message',
+  'user_cell',
+  'job_request',
+  // 'migrations' // Now automatically managed by Knex


Wondering if we should just delete this line? Cause a few months after this PR this becomes a reference to a past state of the code that's no longer relevant? Could be useful though

ibrand · 2019-07-10T16:06:49Z

knexfile.env.js

@@ -0,0 +1,10 @@
+// Update with your config settings.


Is this a leftover comment from knex's docs? Or is the implication here that anyone who pulls this change needs to make some personal change to this file?

ibrand · 2019-07-10T16:09:18Z

migrations/20190207220000_init_db.js

@@ -0,0 +1,355 @@
+const initialize = async (knex, Promise) => {


Is this file an initial migration that folks need to run to get everything set up on the new system? It seems like the knex CLI should be covering us for future migrations. If it is an initial migration that needs to be run, can we label it with a comment block and document it?

it is an initial migration, but does not need to be run manually, because of this:
https://github.com/MoveOnOrg/Spoke/pull/1154/files#diff-6e0d62f54b853b53af874f5965d7adf2R36
i.e. on startup, by default migrations or initial db creation happen automatically.
So knex CLI is mostly useful for creating new migrations or manipulating them for db admin or doing it manually, e.g. in a production setting, where you want to run manually instead of let it 'autorun'

ibrand · 2019-07-10T16:15:06Z

src/server/models/thinky.js


 bluebird.promisifyAll(redis.RedisClient.prototype)
 bluebird.promisifyAll(redis.Multi.prototype)

-// // This was how to connect to rethinkdb:


Has all of this been lifted verbatim into knex.js as a refactor? Or is there a bigger difference that I'm missing?

Should be a verbatim copy

ibrand · 2019-07-10T16:15:52Z

src/server/knex.js

@@ -0,0 +1,73 @@
+// Define a Knex connection. Currently, this is used only to instantiate the


Wondering if we should should rename this to knex-config.js for clarity. From the name I wouldn't have guessed that the whole file is a config

maybe knex-connect.js? the config is in environment variables, but this establishes the connection

schuyler1d · 2019-07-10T17:52:44Z

Here are a few guiding questions:

1. This is not backwards compatible, so what are the exact steps that folks would need to take after they git pull to get fully onto this new system

For anyone without a database yet (or one they need to keep), then answer is nothing -- it will get auto-created when they start for the first time
For anyone that started a database with a version of main prior to June 3rd (when the last migrations/index.js change was made) and have NOT updated since then:

git checkout 7799081deb #current main
# deploy or run instance which triggers migrations (without SUPPRESS_MIGRATIONS or SUPPRESS_DATABASE_AUTOCREATE variables set)
git checkout main #post knexmigrations branch presumably
# deploy or run instance # this will do nothing if no additional migrations have been made, and run newer migrations if there have been.

2. Will this influence anyone's current dev envs if they pull assuming they aren't doing a migration right now?

If they had a database created before March (the time the last dev-affecting schema change was made), then they should clear/delete the database and let it autocreate again -- or do the instructions in (1)

3. How does `migrations/20190207220000_init_db.js` fit into this transition?

It seems knexmigrations ignores the init migration if it is already connected to a database. So the important thing here is that it reflects the current state of prod (which it does at the moment) -- first installs/runs will auto create it with the init_db file.

ibrand · 2019-07-15T17:43:11Z

yarn.lock

@@ -2793,13 +2793,6 @@ bl@~1.1.2:
  dependencies:
    readable-stream "~2.0.5"

-block-stream@*:


Is this file accidentally included? Or included on purpose?

AIUI, yarn.lock is updated occasionally based on upstream package dependencies.
I can remove the yarn.lock changes in this PR, if desired, but we should definitely update yarn.lock regularly -- and if we don't merge the changes here, then every time people do a yarn install the same changes will enter through a different PR.

harpojaeger added 30 commits July 8, 2018 14:37

specify --trim 500 to prevent default trimming behavior

525667e

knexfile and test stub

444b78d

include config for test environment

fbdddec

knexfile working for some db access; partially functional initial "mi…

de934df

…gration"

update knex; getting closer to migrations working in the test environ…

87c9631

…ment

refining migrations

5a45ad3

transaction still fails because knex can't find the knex_migrations table, which is quite odd this seems to be related to the sql batch query, because it works fine with a normal knex function (like createTableIfNotExists)

note to self

b1df69a

better formatting

9c25b46

sky is right: using a pg_dump file is not db-agnostic!

c593b38

create log table

5e03182

simplify knexfile so NODE_ENV is not required to load it

b6e7956

script to export the schema of every table for testing

553bfe6

test every table!

cca7af2

extend jest with a deep schema equality function

0514a1e

make changes to the log table schema - based on test!

0498e24

abstract table creation logic to a function and start adding zip_code

62c411b

try to solve some test inconsistencies

729c9b9

explicitly tear down knex connection so the test exits properly

ef22b3a

still getting scattered test errors, but it's faster overall

f2eacc0

partial message table schema; test failing

38cd979

once columns match, group all errors on columns together in one retur…

e5e7354

…n, so the test doesn't have to be run multiple times

message table passes

a2ff4a1

user & user_cell tables; scattered transaction errors, but those migh…

b85b007

…t not be related

switch the migration to an array-based schema creation iterator, to p…

0521429

…reserve ordering for complex foreign key dependencies

switch testing order to match

1961d1c

organization table passes tests

dfc2599

campaign table

2f746fb

assignment and campaign contact tables

e68646e

add campaign_contact index creation back in; still needs testing

8da6c95

interaction_step table

d6064e5

harpojaeger and others added 13 commits February 23, 2019 19:41

Try with pg 10.0, since Travis doesn't seem to like 11.1.

70530a6

See travis-ci/travis-ci#5737.

Go back to default Travis PG version, pending resolution of travis-ci…

dfcb5d8

…/travis-ci#5737

merge main and harpo/knex-migrations

169f21f

merge origin/main into knex-migrations

2ae157c

Merge branch 'update-rethinkknex' into knex-migrations-sky

5a479d1

get tests working with knex.migrate

d9a34e2

enable yarn knex command

8aed8b0

update yarn.lock file based on package.json

8d3858f

update knex => 0.17.3

c456a0a

move creator_id migration into initial migration to avoid problems wi…

1792898

…th switching from previous src/migrations/index.js setup

update docs with cli command

32d4748

delete schemas as they are out of date now

26ae751

revert changes to datawarehouse connection -- that should be a separa…

e0fde1a

…te DB connection from knex+migrations etc

schuyler1d requested a review from shakalee14 June 22, 2019 21:23

schuyler1d added needs qa round S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc labels Jun 22, 2019

merge main into knex-migrations-sky

e8f1ad4

ibrand reviewed Jul 10, 2019

View reviewed changes

rename knex=>knex-connect and other review feedback tweaks

d1b28f8

ibrand reviewed Jul 15, 2019

View reviewed changes

schuyler1d mentioned this pull request Jul 16, 2019

OSDI action handler and server for import and people, questions, answers, messages #1166

Closed

7 tasks

Merge remote-tracking branch 'origin/main' into knex-migrations-sky

3e7165f

schuyler1d mentioned this pull request Jul 16, 2019

paginated user error on empty people #1172

Open

stop node_env from overriding specific db config

6be1aae

ibrand mentioned this pull request Jul 18, 2019

Unique campaign contact cell and campaign_id migration #1020

Closed

schuyler1d mentioned this pull request Jul 18, 2019

948 always capitalize first letter #1074

Merged

ibrand mentioned this pull request Jul 19, 2019

Stage main #1176

Merged

shakalee14 merged commit 6be1aae into main Jul 31, 2019

schuyler1d mentioned this pull request Aug 1, 2019

Implement a knexfile #692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to Knex migrations #1154

Switch to Knex migrations #1154

schuyler1d commented Jun 22, 2019 •

edited

Loading

ibrand left a comment

ibrand Jul 10, 2019

ibrand Jul 10, 2019

ibrand Jul 10, 2019

schuyler1d Jul 10, 2019

ibrand Jul 10, 2019

schuyler1d Jul 10, 2019

ibrand Jul 10, 2019

schuyler1d Jul 10, 2019

schuyler1d commented Jul 10, 2019

ibrand Jul 15, 2019

schuyler1d Jul 15, 2019

		@@ -0,0 +1,355 @@
		const initialize = async (knex, Promise) => {

		@@ -0,0 +1,73 @@
		// Define a Knex connection. Currently, this is used only to instantiate the

Switch to Knex migrations #1154

Switch to Knex migrations #1154

Conversation

schuyler1d commented Jun 22, 2019 • edited Loading

ibrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuyler1d commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuyler1d commented Jun 22, 2019 •

edited

Loading