Add Spree::Migration #5000
Add Spree::Migration #5000
Conversation
Uses the current Rails version the app is running to run migrations. This makes sure, that a) extensions that are running an older Rails version (we still support down to Rails 5.2) are able to run migrations that are introduced in later Rails versions. Also it makes sure that the Solidus tables are created the same way as the rest of the host apps tables that most likely exist. Most of our migrations or very old (some of them still run with Rails 4.2 version). This changes tables and timestamps to very old Rails defaults. Leading to a misconfigured and inconsistent database schema for new Rails apps. Since this does not effect existing apps, it should be safe to do that.
github-actions
bot
added
the
changelog:solidus_core
Need to use add_reference so that the column types match. MySQL will rightfully complain otherwise.
tvdeyen
force-pushed
the
solidus-migration
branch
from
b9177b1
to
1d5dbf9
Compare
|
Ha, this already revealed a bug with mismatching column types for foreign keys. A thing that MySQL will rightfully complain about and fail to install a fresh Solidus app. |
Codecov Report
@@ Coverage Diff @@
## master #5000 +/- ##
==========================================
- Coverage 86.69% 86.66% -0.03%
==========================================
Files 579 580 +1
Lines 14721 14725 +4
==========================================
Hits 12762 12762
- Misses 1959 1963 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@tvdeyen Thanks! I'm trying to figure out an example of the difference between a migration on Rails 4.2 and 7 (any other versions are ok for the example). Can you help me? |
|
I really don't like this because it doesn't provide guarantees. I think this could cause issues with the schema falling out of state as new features are coded in Solidus. Perhaps we should create a migration to get current existing tables up to the standards of future rails gem versions. I already have some code for that for timestamps. https://gist.github.com/BenMorganIO/4874b7a1d37c465999535a08ef2c2927 @kennyadsl Differences would be:
Some of these we can easily just go through as a laundry list and create a migration to do a mass changeover. |
|
@BenMorganIO I didn't think about this approach, but I think it might make a lot of sense. Still, I'm afraid we need to add this migration in a major release, at least for some of the changes mentioned. For example, adding indexes might require updating some inconsistent data to avoid the migration to fail. |
|
I agree, it should be in a major release. This will take about a day out of a devs time to work through on a large, older project, but not likely more than that. |
There was a problem hiding this comment.
@kennyadsl You can find all of the differences between migration versions here: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/migration/compatibility.rb
Not the easiest thing to read, but that's where they're stored. A notable example, prior to Rails 5.1 primary keys were stored as integer columns. Afterwards, they defaulted to bigint. (You can see the logic swap back to integer in the V5_0 class.)
| @@ -0,0 +1,4 @@ | |||
| module Spree | |||
| class Migration < ActiveRecord::Migration[ActiveRecord::Migration.current_version] | |||
There was a problem hiding this comment.
I'm worried leaving this dynamic is going to cause some headaches in the future. While true that we don't need to worry about existing stores, since they've already copied in the migrations, any future changes to ActiveRecord::Migration will result in non-deterministic migrations for stores.
And possibly even more overhead for existing users. If we find ourselves needing to update old migrations to work with more recent versions of Rails, any stores that have already copied those migrations will likely need to also copy over our changes themselves. (Or just never run migrations again, but I don't think that's something we can unilaterally enforce.)
I think @BenMorganIO's approach is the more sustainable one. We should provide migrations that can help update existing tables to match the new defaults we'd get from running the migration on more modern versions of Rails. That will help ensure both existing and new Solidus stores have a consistent schema to work from.
|
Just for reference, I opened #5030, which is related to this. |
Summary
Uses the current Rails version the app is running to run migrations.
This makes sure, that
a) extensions that are running an older Rails version (we still support down to Rails 5.2) are able to run migrations that are introduced in later Rails versions.
b) the Solidus tables are created the same way as the rest of the host apps tables that most likely exist.
Most of our migrations or very old (some of them still run with Rails 4.2 version). This changes tables and timestamps to very old Rails defaults. Leading to a misconfigured and inconsistent database schema for new Rails apps.
Since this does not effect existing apps, it should be safe to do that.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: