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

Usage with Rails Engines #165

Open
tleish opened this issue Dec 20, 2020 · 12 comments
Open

Usage with Rails Engines #165

tleish opened this issue Dec 20, 2020 · 12 comments

Comments

@tleish
Copy link

tleish commented Dec 20, 2020

Can some point me to best practice of using this gem with Rails engines, where each Rails Engine stores their own data migrations as they do with schema migrations? (see: https://blog.pivotal.io/labs/labs/leave-your-migrations-in-your-rails-engines)

@ilyakatz
Copy link
Owner

Hm, good question. I never really thought about it in terms of engines. The gem should really be used in whatever place has it's own database. So theoretically, it should work in an engine. Practically, I haven't tried it. Does it actually work?

@tleish
Copy link
Author

tleish commented Dec 22, 2020

I added the following into each engine.rb

      # Add seeds.rb to migrations
      data_migrations_path = Array(::DataMigrate.config.data_migrations_path)
      data_migrations_path << File.expand_path('db/data', config.root)
      ::DataMigrate.config.data_migrations_path = data_migrations_path

rake data:migrate picks up tall the migrations, runs the data migrations and records them to the data_migrations table. However, other commands do not work. For example, I get an error when running rake data:schema:load, which is why I wondered if there was a better approach to configuring data migrations with engines.

Was there initial work that never finished to support multiple paths? For example, you can find the following method

# data_migrate/data_migrator_five.rb
def self.migrations_paths
    [DataMigrate.config.data_migrations_path]
end

@kyrofa
Copy link

kyrofa commented Sep 2, 2021

I'm trying this as well, using the same technique as OP regarding migrations. I agree with @tleish that support for multiple paths will be needed, which will allow for a flow similar to what we do for migrations. The main app will need to load data_migrate, though, to get the rake tasks in the right place. Then if individual engines can add their data migrations to a path much like we do for schema migrations, I think everything should work nicely.

@ilyakatz I'd be willing to do this work if it's interests you and you're willing to review it.

@alexevanczuk
Copy link

Hi @kyrofa @ilyakatz and @tleish !

We're also looking to allow for many values of DataMigrate.config.data_migrations_path. Ideally, this could accept a glob, but it would be fine if it just accepted an array of file paths (and we can expand out our own glob). Is something you'd consider accepting a PR for?

@ilyakatz
Copy link
Owner

Oh sorry, looks like I missed the previous comment. And yes, I'm happy to take in a PR, I haven't been actively maintaining this (or working in Rails for that matter), so happy to merge incoming PRs

@alexevanczuk
Copy link

Thanks @ilyakatz ... ya know, as it turns out, we're already able to pass multiple paths to the configuration! The value ends up being massed through to the MigrationContext which calls Array(...) with the value and then globs out that array, so it already works the way we need! Thanks for your response.

@ilyakatz
Copy link
Owner

Oh great, maybe one of the Rails upgrades made it possible. Do you mind updating Readme to indicate this, it may help other folks!

@alexevanczuk
Copy link

Can do!

We had some odd failures in CI related to this change so I'm just going to dig in and confirm it's related to our setup! Then I'll add to the README once I'm more confident other clients will be able to use the same approach 🙏

@gustavodiel
Copy link
Contributor

@alexevanczuk any updates on that? We are really looking forward to that 😄

@alexevanczuk
Copy link

@gustavodiel So we were able to get migrations to run locally with this:

DataMigrate.configure do |config|
   default_path = DataMigrate::Config.new.data_migrations_path
   package_packs = Stimpack::Packs.all.map { |pack| pack.path.relative_path_from(Dir.pwd).join('db/data/') }.map(&:to_s)
   config.data_migrations_path = [default_path, *package_packs]
 end

However, on CI we had this error:

Errno::ENAMETOOLONG: File name too long @ dir_initialize - 
packs/a/db/datapacksb/db/data/packsc/db/data[and so on]
--
  | /var/www/vendor/bundle/ruby/2.7.0/gems/data_migrate-8.0.0/lib/data_migrate/data_schema.rb:34:in `open'

I think for our purposes, we are actually going to keep migrations in /db/data and stop this investigation for now since we don't get a ton of benefit from distributing migrations in subfolders.

@gustavodiel
Copy link
Contributor

@alexevanczuk That seems to be an issue with Stimpack.
What we did was for each engines, in the lib/engine.rb file we added:

    initializer :engine do |app|
      ::DataMigrate.configure do |data_migrate|
        default_path = ::DataMigrate::Config.new.data_migrations_path
        data_migrate.data_migrations_path = [default_path, root.join('db', 'data')]
      end
    end

and worked flawlessly 😄

@Spence1115
Copy link

Spence1115 commented Mar 1, 2023

@gustavodiel so we've tried doing exactly what you did, and we get two problems

  1. The file ends up going to db/data/Users/spence1115/git/.../engines/some_engine/db/data/20230301021917_populate_thing.rb
  2. It seems to always only pick up the file name of the last engine added

It also only read from one engine. We changed to

::DataMigrate.configure do |data_migrate|
        new_path = root.join("db", "data")
        current_path = ::DataMigrate.config.data_migrations_path
        data_migrate.data_migrations_path = if current_path.instance_of?(Array)
          current_path.concat([new_path])
        else
          [current_path, new_path]
        end
      end

in each engine, which meant it picked up and ran migrations for each engine, but still doesn't work for creating the migration in the first place :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants