rubyatscale / packs-rails

packs-rails establishes and implements a set of conventions for splitting up large monoliths.
MIT License
268 stars 26 forks source link

Question: should db/migrate be part of the default paths? #25

Closed sk- closed 2 years ago

sk- commented 2 years ago

When using engines we had to manually add db/migrate to the list of paths.

Stimpack.config.paths.concat(%w(db/migrate))

Should that path be part of the default paths? Alternatively, would it be possible to add that path for engines?

ngan commented 2 years ago

This on is tricky because the way that Rails looks for "other migration paths" in only in config/database.yml under the migrations_paths key (https://edgeguides.rubyonrails.org/active_record_multiple_databases.html).

Afaict adding extra db/migrate paths doesn't do anything. You can experiment and let me know! Thanks.

sk- commented 2 years ago

@ngan With the change I posted all our migrations are run. We even have a test that checks that the resulting schema has not changed.

oleg-vinted commented 2 years ago

Adding db/migrate to Stimpack.config.paths allows developers to put migrations into packs rather than storing them globally in db/migrate.

So unless you also use migrations_paths, the downside of this is that rails generate migration does not take these paths into account when creating new migrations, so you'd have to move the file by hand.

alexevanczuk commented 2 years ago

Currently our application uses multiple database, and certain packs have their databases' migrations located at packs/pack_name/db/migrate. So without having stimpack know which packs use their own database, this could produce unexpected results when using this feature of Rails.

For now, our recommendation if you don't use multiple databases would be to manually add these paths:

    # In `config/application.rb`
    Stimpack::Packs.all.map do |pack|
      config.paths['db/migrate'] << pack.path.join('db/migrate')
    end

Later if we can make stimpack aware of packs that use separate databases, we could bake this right into stimpack.