Migrations

[christiansmith]

Upgrading to new versions of code often requires changes to an existing database. Currently, we've made the code to automate migration in the boot/database.js file. The size of this file is growing and will become unmaintainable as we add migration code for future versions. This commit expresses a proposed way of formalizing migrations. With this strategy, we'll be able to add a new module that handles migration for each new version. In addition, separating the migration code in this way makes it testable in isolation.

[christiansmith]

This code is not ready to merge. It's here in draft form for review and discussion.

[adalinesimonian]

This is different behaviour than what we currently have. With this check, inserting roles, scopes, and assigning permissions only happens if there is no existing data. Currently, this is all run regardless of the version of the data in the database, to alleviate concerns of users moving up from earlier versions where this logic was run solely by nv migrate.

It looks like this should either

1. Always run, which also helps in the case an admin accidentally deletes any of these entries from the database, or
2. Be a migration defined for the latest version of Connect that did not yet run these scripts automatically at boot-time.

I'm vouching for 1.

[adalinesimonian]

Secondly, if this is to always run, I'm not sure it belongs in a migration script. This is less of a migration script and more of a case where we're ensuring something is in the database. A related migration would be converting the data if the existing data in the database is old and in a different format.

boot/database.js may still have some use for those kinds of scenarios. It would run after (or before?) the migration scripts are finished.

[adalinesimonian]

So we have the migrations:

• 0.0.0 - runs if version <= 0.0.0
• 0.1.55 - runs if version <= 0.1.54
• 0.1.56 - runs if version <= 0.1.55

This is inconsistent and confusing. The migration version numbers should match up.

Either the names become consistent with the semver checks, or the semver checks become consistent with the names.

i.e., either 0.1.55 and 0.1.56 get named 0.1.54 and 0.1.55, or the semver checks for them become <0.1.55 and <0.1.56 respectively.

[adalinesimonian]

I'm not completely sold on boot/database.js being renamed boot/migrate.js for reasons described in my notes in 0.0.0.js and the terminology in the file still overall reflects a more general database initialization pattern rather than being strictly migrations. It looks like boot/database.js should stay and either it runs the migration scripts in addition to other DB logic, or migration logic is run entirely on its own, separate from the rest of the logic.

[christiansmith]

I've renamed migrations/0.0.0.js to migrations/baseline.js for clarity and changed the logic to ensure it is always run. Beyond that, I think we're bikeshedding.

@vsimonian Please review and merge (assuming everything works correctly). Thanks.

[adalinesimonian]

context:

christiansmith [2:25 PM] I believe the code now behaves the same as it does without these commits. The purpose was to give us a way to organize incremental, version-specific changes to the database, and this PR does that. I believe boot/database is too generic and doesn't effectively describe what's happening. It could be interpreted as loading database drivers or creating a connection. In fact, that is not what's happening.
vartan [2:25 PM] That's a fair reason, I can get behind that.
vartan [2:25 PM] Alright, what about the version numbers, though?
christiansmith [2:28 PM] ok, that's a trivial change. I'll do it now.
christiansmith [2:30 PM] pushed

[adalinesimonian]

LGTM!