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

fix: Avoid a long table lock for the first_name migration #4816

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

khvn26
Copy link
Member

@khvn26 khvn26 commented Nov 6, 2024

Thanks for submitting a PR! Please check the boxes below:

  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

This replaces users.0039 migration with a manual one that aims to avoid long locking the users_ffadminusers table during migration execution.

A new users_ffadminusers.first_name_v2 column is added and is backing the FFAdminUser.first_name field.

Notes:

  • Should we replace the migraton or rather add one? Currently, to deploy this on top of the previous version, a migrations table mofification is in order.
  • Alternatively, we could probably do away with adding ALTER TABLE {table_name} RENAME COLUMN first_name_v2 TO first_name statement to the forward migration.
  • This is most likely not compatible with Oracle due to syntax differences.

How did you test this code?

Added two unit tests for reverse and forward migrations.

@khvn26 khvn26 requested a review from a team as a code owner November 6, 2024 18:09
@khvn26 khvn26 requested review from matthewelwell and removed request for a team November 6, 2024 18:09
Copy link

vercel bot commented Nov 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Nov 6, 2024 6:09pm
flagsmith-frontend-preview ⬜️ Ignored (Inspect) Nov 6, 2024 6:09pm
flagsmith-frontend-staging ⬜️ Ignored (Inspect) Nov 6, 2024 6:09pm

@github-actions github-actions bot added api Issue related to the REST API fix labels Nov 6, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-4816 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-e2e:pr-4816 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-frontend:pr-4816 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-api:pr-4816 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-4816 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-4816 Finished ✅ Results

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Uffizzi Ephemeral Environment deployment-58038

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/4816

📄 View Application Logs etc.

What is Uffizzi? Learn more!

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.33%. Comparing base (364c565) to head (78b666a).
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4816   +/-   ##
=======================================
  Coverage   97.33%   97.33%           
=======================================
  Files        1180     1181    +1     
  Lines       41213    41251   +38     
=======================================
+ Hits        40113    40151   +38     
  Misses       1100     1100           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matthewelwell
Copy link
Contributor

Alternate implementation here: #4817

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants