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 migations for non-ee installs #3817

Merged
merged 5 commits into from
May 7, 2024
Merged

Fix migations for non-ee installs #3817

merged 5 commits into from
May 7, 2024

Conversation

knolleary
Copy link
Member

Fixes #3811

PR #3645 introduced a migration on an EE-only model without a check the DB table exists.

For clean installs of 2.1 or later, this wouldn't be a problem because of the baseline migration we added in 2.1.0 that initialises all expected tables for an empty database. The idea was to remove the need to have lots of guards in future migrations. However that baseline wasn't intended to also help upgrade an existing database. And that's what this PR addresses.

For someone who installed pre 2.1.0, without an EE license, they won't have some of the EE tables. This PR does two things:

  1. adds a table guard in the existing migration
  2. adds a new migration that checks the existence of each EE table and creates it if needed

The net result will be the db structure should be identical for OSS and EE installs so future migrations don't have to worry about that.

@knolleary knolleary requested a review from Steve-Mcl May 3, 2024 14:55
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 53.84615% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 79.10%. Comparing base (3cc710c) to head (7a5b46e).

Files Patch % Lines
...grations/20240503-01-ensure-all-ee-tables-exist.js 50.00% 11 Missing ⚠️
...tions/20231207-01-EE-add-to-flow-template-table.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3817      +/-   ##
==========================================
- Coverage   79.15%   79.10%   -0.05%     
==========================================
  Files         278      279       +1     
  Lines       12500    12524      +24     
  Branches     2773     2782       +9     
==========================================
+ Hits         9894     9907      +13     
- Misses       2606     2617      +11     
Flag Coverage Δ
backend 79.10% <53.84%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented May 3, 2024

Hmmm. Odd, pretty sure i tested that scenario. I was aware of the rollup and (in my head) it was designed to align the tables and get everyone on an even footing. I intentionally did not check for the existence of the table (due to prior statement)

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

I have checked the field names, references, allow Nulls & cascades for the tables added.

Code wise, review gets approval.

Let me know if you want an interactive test by pulling and firing up a particular version of FF to check the operations explicitly.

Otherwise feel free to merge.

@knolleary
Copy link
Member Author

I did test this for the upgrade path specific mentioned in the original issue. And I'm satisfied this is a no-op for existing EE installs. Happy to merge.

@knolleary knolleary merged commit 2173621 into main May 7, 2024
10 of 11 checks passed
@knolleary knolleary deleted the fix-migrations branch May 7, 2024 12:50
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.

DB Migration error 2.0.1 -> 2.3.0
2 participants