[Notifications] making DB notification objects more generic #3678
Conversation
theSaarco
requested review from
quaark,
Tankilevitch and
liranbg
and removed request for
quaark and
Tankilevitch
There was a problem hiding this comment.
my main concern is that this table will be code-dup and prone to errors on data migrations when you add another x_notifactions. worth some comments around the table that its properties must be exactly the same as any other x_notifications because its interface is reused
| @@ -49,17 +49,17 @@ def upgrade(): | |||
| "condition", sa.String(length=255, collation="utf8_bin"), nullable=False | |||
| ), | |||
| sa.Column("params", sa.JSON(), nullable=True), | |||
| sa.Column("run", sa.Integer(), nullable=True), | |||
| sa.Column("parent_id", sa.Integer(), nullable=True), | |||
There was a problem hiding this comment.
worth leaving a comment why you decided to use parent id and not run id as this is a bit counter-intuitive, without understanding the reason (which is, afaik - due to implementation abstraction)
There was a problem hiding this comment.
Well, the idea is that this table is generated through a make_notification function that can be called for any DB object, so the table will be the same across objects (with a different name). This is a pattern we use in several other cases (tags, labels). I will add some comments here, though this is just the migration code, so usually won't be used for real reference - the actual DB schema code is clearer in that sense.
This PR modifies the way notifications are stored in the DB, with the forward thinking of extending notifications to more than just Runs. The changes keep a notification table per object-type, and modifies functions that handle this table to accept the "parent" class, so it can easily be extended to other objects.
Note to developers:
This PR breaks compatibility with previous releases of the development branch. It will cause issues when upgrading from previous rc version to a newer version containing this change.