-
Notifications
You must be signed in to change notification settings - Fork 1
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
Revert changeset modifications. #429
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. |
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.
Back to 1^2 :-) LGTM
@marikomedlock for now can you but dbms:postgresql tag on both these files? |
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.
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 |
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.
Can I suggest a few more changes? Specifically:
constraints: | ||
nullable: true | ||
- column: | ||
name: cohort_revision_id | ||
type: ${id.type} | ||
type: text | ||
constraints: | ||
references: cohort_revision(id) | ||
foreignKeyName: ck_crit_cr |
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.
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
@@ -432,7 +433,7 @@ databaseChangeLog: | |||
remarks: Deleting a cohort revision will cascade to delete the criteria contained in it | |||
- column: | |||
name: concept_set_id | |||
type: ${id.type} | |||
type: text | |||
constraints: | |||
references: concept_set(id) | |||
foreignKeyName: fk_crit_cs |
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.
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
constraints: | ||
nullable: true | ||
- column: | ||
name: cohort_revision_id | ||
type: ${id.type} | ||
type: text | ||
constraints: | ||
references: cohort_revision(id) | ||
foreignKeyName: ck_crit_cr |
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.
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
@@ -67,7 +68,7 @@ databaseChangeLog: | |||
remarks: Deleting a cohort revision will cascade to delete the criteria contained in it | |||
- column: | |||
name: concept_set_id | |||
type: ${id.type} | |||
type: text | |||
constraints: | |||
references: concept_set(id) | |||
foreignKeyName: fk_crit_cs |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.