Upstream and allow engines #2
Conversation
…a-migration Foxondo fix eager schema migration
foxondo - Add delegation to exists? for use by third parties
…obal-functions-for-rake-tasks Berniechiu fix/avoid global functions for rake tasks
…class Fix wrong module class was called
This is a re-creation of ilyakatz#202, which fixes a bug where the AR migration schema version is incorrectly referenced instead of the data migration version. This causes data migrations be to be incorrectly re-run on an otherwise migrated database whenever the most recent schema version is lower than the most recent data version.
Reidab patch 1
This will help developers debug issues related to an incorrect file path as early as possible.
Bazay custom template
Obsolete post install message removed
There was a problem hiding this comment.
I think in the future it might be worth making two separate PRs: one to pull changes from upstream, and one for the changes we want to make. It just highlights the Trusted-specific changes a little better and makes them easier to talk about.
I'm skipping review for the upstream changes; presumably those have been checked by the Data Migrate team. I'm just looking at this commit with changes allowing our use of engines.
The code itself looks good. Two comments, but neither is really a blocker for deploying at Trusted. Before merging though, could we add a spec to spec/data_migrate/data_spec.rb or somewhere similar to test the functionality of the new code and prevent regressions?
| match_data = DataMigrate::DataMigrator.match(file) | ||
| versions << match_data[1].to_i if match_data | ||
| versions = Set.new | ||
| DataMigrate::DataMigrator.migrations_paths.each do |path| |
There was a problem hiding this comment.
It looks like this might be changing the list from an expanded path to a relative path. It might be an issue if running the data_migrate command from somewhere other than Rails.root ? Pretty edge case, would not consider this a blocker for deployment, but maybe something the upstream folks will be concerned about 😸
| Dir.foreach(path) do |file| | ||
| match_data = DataMigrate::DataMigrator.match(file) | ||
| versions << match_data[1].to_i if match_data | ||
| end | ||
| end | ||
| versions |
There was a problem hiding this comment.
Set and Array have pretty different methods available. If it doesn't break anything, I think changing to Set here is an improvement since it removes duplicates. But if we wanted to minimize changes to method signatures, we could put a .to_a here so the method returns the same type as it did before.
This is an update which fixes the failing tests in ilyakatz#225 as well as ensures that the data rake tasks run successfully when there are multiple migration paths due to engines also using data migrations.
another alternative here is to install the migrations into the host application, which is how i believe schema migrations work. open to thoughts on strategy here!