Integrate morph into the plugin #619
Conversation
mgdelacroix
force-pushed
the
integrate-morph
branch
4 times, most recently
from
4176f52
to
7ad31c9
Compare
mgdelacroix
force-pushed
the
integrate-morph
branch
from
7ad31c9
to
fab6d4c
Compare
mgdelacroix
force-pushed
the
integrate-morph
branch
from
aae8b02
to
49aa79c
Compare
There was a problem hiding this comment.
Thanks, @mgdelacroix! Besides the few comments below, wondering if we "need" 15 separate migrations or if we could just have a single monolithic migration to get us "up to date", and start adding new ones discretely thereafter?
| } | ||
| currentVersion := len(applied) | ||
|
|
||
| s.api.LogDebug("== Ensuring migrations applied up to version ====================", "version", version, "current_version", currentVersion) |
There was a problem hiding this comment.
This is unrelated to this PR, but I've always found it odd that the morph logs are formatted (i.e. with ====... as it they were being consumed by a user at a console, when instead we mostly see them as messages inside server logs. 0/5
There was a problem hiding this comment.
Agree, the idea was to make all migration logs (inside and outside of morph) similar, but we can change the ones that come from the store to avoid the padding and the =s if we want, 0/5 really on my side
| return mErr | ||
| } | ||
|
|
||
| if remoteID != "" { |
There was a problem hiding this comment.
As I recall, these are data migrations that require running code vs. just SQL. Is the idea to continue to append to this method as such migrations are required, or in the long run would we split apart the two concepts (schema vs data)? I realize that would require some finagling given the need to run some data migrations "between" others.
@jespino, is this some shared channels complexity would could eliminate if we required users adopting two way sync to "reset" somehow?
There was a problem hiding this comment.
I think we will always need some sort of sequential process to be able to run data migrations in between the plain SQL ones. The idea with the ensureMigrationsAppliedUpToVersion + the constants that indicate the version needed for a data migration is to model that sequence and make it simple to follow and expand, but if we can find a way to get rid of it, I'd love to give it a try.
At some point we discussed making morph aware of data migrations, so the sequence was enforced by the migration system directly, but that's just an idea at this point.
| @@ -0,0 +1 @@ | |||
| SELECT 1; | |||
There was a problem hiding this comment.
nit: could morph just treat an empty file as a no-op?
There was a problem hiding this comment.
Heh, actually having an empty file is only a problem for MySQL, Postgres will process those perfectly, so I'm making that change
|
@lieut-data we could do one big "init" migration only, but there are two downsides that I can think of:
|
They had a simple `SELECT 1` before because that's what we've done in other instances as we had to support MySQL. Postgres is able to process empty migrations, so as the plugin only supports running on Postgres, the files are now empty.
|
/update-branch |
mgdelacroix
added
4: Reviews Complete
Summary
This PR integrates
morphinto the plugin store code, moving all migrations to SQL files, splitting them for improved clarity and structure. It creates a newmsteamssync_system_settingstable that stores one row per data migration, allowing them to indicate when they've run and removing the need for checking the database status every time the plugin starts to see if each data migration should run.The
Migratemethod implements a sequence of migrations that ensures that at every step all the requirements of the next migration are met or checked (either by morph or the data migration through the previously mentioned table), and every step logs when the migration starts, stops and the elapsed time.