-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Database Migrations for CoreAccount after migration to Generic #3977
Conversation
e792e56
to
32d3585
Compare
… into query.py file
32d3585
to
6b8188c
Compare
I still need to make the queries compatible with memgraph but appart from that the PR is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR, learnt a lot while reviewing it 👍🏻
name="Account", | ||
namespace="Core", | ||
branch_support=BranchSupportType.AGNOSTIC.value, | ||
labels=["CoreAccount", InfrahubKind.GENERICACCOUNT, InfrahubKind.LINEAGEOWNER, InfrahubKind.LINEAGESOURCE], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace by constant InfrahubKind.ACCOUNT
instead of "CoreAccount"
Fixes #3915
This PR implements the migrations to update the schema and the data after the conversion of
CoreAccount
to use the newCoreGenericAccount
(#3807)It was an interesting exercice because it was probably our most complicated migration to date and it gave me the opportunity to do some cleanup in the code and to implement some helpers that will make it easier next time.
These migrations needs to be applied both to the schema and the data (existing nodes) and they are applied with 2 commands
infrahub db migrate
infrahub db update-core-schema
The first command is very low level and can run any queries to modify either the schema or the data. the main constraint for this command is that we don't have access to the registry or the existing schema so the query we execute must have very few dependencies.
The second command
infrahub db update-core-schema
is doing a standard schema update by calculating the diff between the current core schema and the one currently defined in the code.A significant part of the schema updates can automatically managed by this command but some preparation works can be required if the change is not entirely supported.
For this migration, it was required to :
CoreAccount
that have been moved to the new generic from the schema before applying the new schemaCoreAccount
that are referencing the attributes and relationships that have been migrated.For the migration of the existing nodes, it was required to
type
toaccount_type
CoreAccount
CoreGenericAccount
toCoreAccount
In order to avoid duplicating some complicated queries, I've converted some existing SchemaMigration into more generic queries that can be used within the graph migrations and outside of the context of schema migrations.
And I've introduced a few new ones
The fix for #4004 is also required in order to successfully complete the upgrade because some relationships on
CoreThread
andCoreComment
will be updated during the migration.Disabling some schema validations when running
infrahub db update-core-schema
One of the main issue to address was all the validations enforced when we are loading the existing schema at the start beginning of
infrahub db update-core-schema
and during the update process.For example today it's not supported to update the value of
inherit_from
(to add the new generic). it was not possible to update the value manually without also creating the entire generic in the schema so instead I implemented a flag to disable the error related toupdate_support
when we are loading the latest schema duringinfrahub db update-core-schema
At some point I also explore the option to disable more validators, especially when we are processing the new schema after loading it from the database. Ultimately I decided to take another route but I left the flag in place because it feels like we'll need it at some point.