fix(models): add missing foreign key to workflow_uuid of Job (#214) #214
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #214 +/- ##
==========================================
+ Coverage 73.28% 73.31% +0.02%
==========================================
Files 7 7
Lines 917 918 +1
==========================================
+ Hits 672 673 +1
Misses 245 245
|
|
This PR does not handle the cleaning of the database to make sure that all the values in Should the cleanup be performed in the migration, by deleting the related records? Otherwise, we have to instruct the administrator to perform the cleanup manually UPDATE: the alembic migration now also deletes the jobs referring to non-existing workflows |
mdonadoni
changed the title
| # 1. delete jobs which refer to non-existing workflows | ||
| op.execute( | ||
| job_table.delete().where( | ||
| job_table.c.workflow_uuid.notin_(sa.select([workflow_table.c.id_])) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
|
|
|
This was tested on a clone of DEV |
| "ALTER CONSTRAINT fk_job_cache_job_id_job DEFERRABLE" | ||
| ) | ||
| ) | ||
| op.execute(sa.text("SET CONSTRAINTS __reana.fk_job_cache_job_id_job DEFERRED")) |
There was a problem hiding this comment.
Hmm, shall we do a check first whether there would be any rows deleted by this upgrade, and if not then we are good, and if yes shall we warn the user beforehand about the deletion in order to ask for the permission to do so?
(FWIW I think it is perfectly OK, practically, to also remove the rows in this hard-way, since if a job is not attached to a workflow, then chances are it is not exposed to users at all, and so it's ready for the deletion. I'm just wondering, theoretically, whether this isn't a good occasion to introduce an interactive please-confirm-action upgrade technique for DB recipes. Could serve as a nice example for the future?)
Closes #210
Depends on #213