Add data migration foundation - #239
Conversation
|
Great PR @shomykohai! Could we perhaps also add a unit test for migration itself? i.e. a test migration? |
| @tool | ||
| extends Node | ||
|
|
||
| const PANDORA_DATA_VERSION: int = 1 |
There was a problem hiding this comment.
This is not ideal because it is very difficult to control for unit tests... but perhaps when creating unit tests for migrations we could do some magic of pretending previous version is PANDORA_DATA_VERSION - 1 or something like that.
There was a problem hiding this comment.
Why would it be an issue? That should represent the latest data format (we assume that current pandora codebase holds the truth about latest format).
For unit tests, I exposed the "data_migrated" signal, and made new data start at index 0 if not migrated yet, so that it should simplify unit testing because we will be sure we're running through all migrations!
Maybe I just didn't get your point, so feel free to correct me!
90324ea to
e650134
Compare
| @@ -0,0 +1,3 @@ | |||
|
|
|||
| static func migrate(data: Dictionary, old_ver: int, new_ver: int) -> Dictionary: | |||
There was a problem hiding this comment.
not every migration may apply to every version... we probably need another method aka isSupported(version: int) -> bool where we can do a version/range check to avoid scenarios where the migration would be incompatible with the current data structure.
However, I don't think this is needed now and it can be added in future when the need arises.
There was a problem hiding this comment.
Isn't the concept of "indexed" migrations made so that, based on the data format version, we just start from old_version + 1 to mutate the incoming dict?
I thought of it in the same way we apply database migrations for context.
Unless you have in mind to make major breaking changes to the data format, in which case I get ur point.
There was a problem hiding this comment.
Let's say a new migration is needed, how would we make sure old/previous migrations are skipped? Because a migration should never be applied twice ideally. This is the part I am kinda missing here right now.
There was a problem hiding this comment.
old_ver in this case is the one in data.pandora
Let's say data.pandora is from first pandora version, and latest one is version 6.
old_ver will be 0, and migrations will run from 0 to 6.
Now, let's assume we have a data.pandora from version 4. old_ver will be 4, and we will run migrations 5 and 6.
The logic itself to run migrations is not there yet, but basically this will be checked inside this migrate function, where we will do something like:
static func migrate(data: Dictionary, old_ver: int, new_ver: int) -> Dictionary:
var start_migration_idx = old_ver + 1
# Safety check
if start_migration_idx > new_ver:
return data
for n in range(start_migration_idx, new_ver):
match n:
1: data = run_migration_v0_to_v1(data)
2: data = run_migration_v1_to_v2(data)
...
or even better if we make the PandoraMigration class, so we can simplify the loop to avoid match statements, but rather check the migration file name directly to build the script of said file and run (maybe a execute(data) function?) the migration.
There was a problem hiding this comment.
Okay. We can do this in a separate PR then!
Introduces the foundation for allowing future data migrations.
e650134 to
b243a66
Compare
Description
Introduces a versioning system for
data.pandora.For now, the implementation is "mock", so the migration yields back the same dictionary and just appends the a new field to
data.pandorafor future usage:_version.The idea is that, following the addressed issue vision, the Migration service will loop through a list of
.gdfiles that will manipulate the data.Files without an explicit version will be marked as version
0before the migration starts.Addressed issues