Conversation
|
@marikomedlock Thank you for this update and adding me to the PR. I will create a new branch from main and work on as we discussed earlier. |
|
@marikomedlock for now can you but dbms:postgresql tag on both these files? |
chenchals
left a comment
chenchals
left a comment
There was a problem hiding this comment.
Could you add dbms: postgresql tag to both the schema files?
Let me test it out in our GKE deployment. |
|
Tested out deploying this to the Verily GKE |
chenchals
left a comment
chenchals
left a comment
There was a problem hiding this comment.
Can I suggest a few more changes? Specifically:
| type: text | ||
| constraints: | ||
| references: cohort_revision(id) | ||
| foreignKeyName: ck_crit_cr |
There was a problem hiding this comment.
Foreign key name: ck_crit_cr 20230410_schema_reset.yaml#430 clashes with same name in 20230530_avoid_postgres_specific_features.yaml#65 Please consider renaming one of these to a different name maybe ck_crit_cr_2 in the avoid_postgres file
| type: text | ||
| constraints: | ||
| references: concept_set(id) | ||
| foreignKeyName: fk_crit_cs |
There was a problem hiding this comment.
Foreign key name: fk_crit_cs in 20230410_schema_reset.yaml#439 clashes with same name in 20230530_avoid_postgres_specific_features.yaml#74 Please consider renaming one of these to a different name maybe fk_crit_cs_2 in the avoid_postgres file
| type: text | ||
| constraints: | ||
| references: cohort_revision(id) | ||
| foreignKeyName: ck_crit_cr |
There was a problem hiding this comment.
Foreign key name: ck_crit_cr in 20230410_schema_reset.yaml#430 clashes with same name in 20230530_avoid_postgres_specific_features.yaml#65 Please consider renaming one of these to a different name maybe ck_crit_cr_2 in the avoid_postgres file
| type: text | ||
| constraints: | ||
| references: concept_set(id) | ||
| foreignKeyName: fk_crit_cs |
There was a problem hiding this comment.
Foreign key name: fk_crit_cs in 20230410_schema_reset.yaml#439 clashes with same name in 20230530_avoid_postgres_specific_features.yaml#74 Please consider renaming one of these to a different name maybe fk_crit_cs_2 in the avoid_postgres file
| @@ -76,12 +77,12 @@ databaseChangeLog: | |||
| remarks: Deleting a concept set will cascade to delete the criteria contained in it | |||
| - column: | |||
| name: key | |||
There was a problem hiding this comment.
Is it possible to change the name of this column from key to criteria_key?
| @@ -19,12 +20,12 @@ databaseChangeLog: | |||
| remarks: Deleting a study will cascade to delete its properties | |||
| - column: | |||
| name: key | |||
There was a problem hiding this comment.
Is it possible to change the name of this column from key to property_key? Obviously need java changes for the DAOs.
|
Resolved offline, pasting here for reference in case we need to come back to it.
Unfortunately no. I can't make changes to these files because they've already been deployed in our environments. This is the reason for this PR -- I need to put them back to what was actually deployed. Any new changes (e.g. to column names) need to be in separate changeset files. |
3 participants
Reverted the changeset modifications from #412. When I tested this locally, PostGres seemed fine with the updated changeset files, since the rendered version was the same. But it failed in our GKE deployment.
This doesn't change the overall approach to getting Tanagra's DB schema working with MariaDB/MySQL. The end goal is still to have a version of the schema that works with both PostGres and MariaDB/MySQL. Since that will require wiping the DB in our existing deployments that use PostGres, we need to wait until user testing is over to combine the schemas. In the meantime, we will maintain 2 separate schemas, and plan to drop the postgres-only schema files once user testing is over.