-
Notifications
You must be signed in to change notification settings - Fork 268
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
db: updates for image expiry notification (PROJQUAY-7075) #2894
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @Sunandadadi and the rest of your teammates on |
7e94780
to
6cd7f1a
Compare
ed63988
to
2582626
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2894 +/- ##
=======================================
Coverage 70.34% 70.35%
=======================================
Files 439 439
Lines 41269 41274 +5
Branches 5410 5410
=======================================
+ Hits 29032 29037 +5
Misses 10531 10531
Partials 1706 1706
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2582626
to
fcf8090
Compare
4cc971d
to
c2c0150
Compare
data/database.py
Outdated
@@ -1426,6 +1428,7 @@ class RepositoryNotification(BaseModel): | |||
config_json = TextField() | |||
event_config_json = TextField(default="{}") | |||
number_of_failures = IntegerField(default=0) | |||
last_ran_ms = BigIntegerField(null=True) |
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.
You'll want an index on this field if you're going to be looking up by last_ran_ms
class TagNotificationSuccess(BaseModel): | ||
notification = ForeignKeyField(RepositoryNotification, index=True, null=False) | ||
tag = ForeignKeyField(Tag, index=True, null=False) | ||
method = ForeignKeyField(ExternalNotificationMethod, null=False) |
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.
You'll have to test when ever the foreign notification or tag rows are deleted that this row get's deleted as well without causing any issues.
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.
yes, I added both in the second PR, tags clean up and notification clean up
1b0cb30
to
606612c
Compare
repositorynotification_indexes = inspector.get_indexes("repositorynotification") | ||
if not "last_ran_repositorynotification" in [i["name"] for i in repositorynotification_indexes]: | ||
op.create_index( | ||
"last_ran_repositorynotification", |
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.
nit: the conventional index name is <table_name>_<column_name>
|
||
def downgrade(op, tables, tester): | ||
op.drop_column("repositorynotification", "last_ran_ms") | ||
op.drop_index("last_ran_repositorynotification", table_name="repositorynotification") |
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.
I would imagine that the index needs to be dropped before the column, could be wrong though
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.
good point. I reversed the operations
789da20
to
5c9ce57
Compare
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.
LGTM
bbf518e
to
80dc8c9
Compare
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.
LGTM
database migrations for image expiry notification (PROJQUAY-7075)
seeding db changes for cypress