Skip to content
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

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

marikomedlock
Copy link
Collaborator

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.

@chenchals
Copy link
Collaborator

@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.

Copy link
Collaborator

@chenchals chenchals left a 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

@chenchals
Copy link
Collaborator

@marikomedlock for now can you but dbms:postgresql tag on both these files?

Copy link
Collaborator

@chenchals chenchals left a 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?

@marikomedlock
Copy link
Collaborator Author

Could you add dbms: postgresql tag to both the schema files?

Let me test it out in our GKE deployment.

@marikomedlock
Copy link
Collaborator Author

Tested out deploying this to the Verily GKE test environment with the dbms: postgressql tag in both changeset files. Everything looks OK, so keeping the change in the PR too.

Copy link
Collaborator

@chenchals chenchals left a 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:

  1. avoid identical constraint names in the 2-schema files
  2. rename the column key to property_key and criteria_key as appropriate
  3. Obviously need JAva code change for DAOs (I already have these in my PR#425, but I also changed value to property_value and criteria_value in the PR#425

constraints:
nullable: true
- column:
name: cohort_revision_id
type: ${id.type}
type: text
constraints:
references: cohort_revision(id)
foreignKeyName: ck_crit_cr
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@chenchals chenchals self-requested a review June 6, 2023 18:38
@marikomedlock
Copy link
Collaborator Author

Resolved offline, pasting here for reference in case we need to come back to it.

Can I suggest a few more changes? Specifically:

  1. avoid identical constraint names in the 2-schema files
  2. rename the column key to property_key and criteria_key as appropriate
  3. Obviously need JAva code change for DAOs (I already have these in my PR#425, but I also changed value to property_value and criteria_value in the PR#425

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.

@marikomedlock marikomedlock merged commit 5ba14b3 into main Jun 6, 2023
@marikomedlock marikomedlock deleted the mm-revert-changeset-modifications branch June 6, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants