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

Remove Foreign Key Constraints for Service Account #1172

Merged
merged 6 commits into from
Aug 9, 2024
Merged

Conversation

BinamB
Copy link
Contributor

@BinamB BinamB commented Aug 7, 2024

Link to JIRA ticket if there is one:
Pre-alembic era created a foreign key clent_id(from the client table) on the google_service_account table. This foreign key was then removed from the schema but any commons created before the constraint was removed still held the foreign key. The previous alembic migration tuncates the client table but this fails if the foreign key constraint still persists therefore failing the migration. This migration checks for the existence of the foreign key constraint and removes it if it exists. There is no downgrade path for this since not having the foreign key constraint is the correct schema throughout all versions. This migration is specifically for commons that were created before the foreign key constraint was removed.

New Features

Breaking Changes

Bug Fixes

  • Create new migration to remove foreign key constraint if it exists on google_service_account table

Improvements

Dependency updates

Deployment changes

  • Requires database migration

@coveralls
Copy link

coveralls commented Aug 7, 2024

Pull Request Test Coverage Report for Build 10322529192

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 75.278%

Totals Coverage Status
Change from base Build 10290555822: 0.0%
Covered Lines: 7792
Relevant Lines: 10351

💛 - Coveralls

@BinamB BinamB marked this pull request as ready for review August 8, 2024 16:36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a way to test that the full alembic migration succeeds? IIRC the migration fails here:
https://github.com/uc-cdis/fence/blob/master/migrations/versions/9b3a5a7145d7_authlib_update_1_2_1.py

Because of this FK constraint. But this only tests if the migration to remove the FK constraint succeeds, not that the collection of migration scripts succeeds.

I think a full migration check is beyond the scope of what you're doing here, but it might be best to move the removal of the FK to the previous migration https://github.com/uc-cdis/fence/blob/master/migrations/versions/9b3a5a7145d7_authlib_update_1_2_1.py here instead of adding a new migration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we update this migration
https://github.com/uc-cdis/fence/blob/master/migrations/versions/9b3a5a7145d7_authlib_update_1_2_1.py

To do this instead of making a new migration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this won't work. We can't have a new migration b/c the previous one fails. We must update the previous one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this won't work. We can't have a new migration b/c the previous one fails. We must update the previous one

Avantol13
Avantol13 previously approved these changes Aug 9, 2024
migrations/versions/9b3a5a7145d7_authlib_update_1_2_1.py Outdated Show resolved Hide resolved
Co-authored-by: Alexander VanTol <Avantol13@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants