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

Infer migrations_paths from database name #36886

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

seejohnrun
Copy link
Member

@seejohnrun seejohnrun commented Aug 7, 2019

Summary

Similar to what we do for schema files, we now infer the default
migrations_paths for a database configuration using the database
specification name.

We still allow overriding these defaults with a migrations_paths key
in the database configuration file.

Additionally, we've updated some documentation accordingly in the guides.

We've also removed Migrator.migrations_paths since we should be taking
the migrations_paths off of the config instead.

cc / @eileencodes

@seejohnrun seejohnrun force-pushed the infer-migrations-paths branch 2 times, most recently from 3d1860f to 64a54a4 Compare August 7, 2019 22:19
@eileencodes eileencodes self-assigned this Aug 8, 2019
@eileencodes eileencodes added this to the 6.1.0 milestone Aug 8, 2019
@seejohnrun seejohnrun force-pushed the infer-migrations-paths branch 2 times, most recently from f83df6e to 053a3f2 Compare August 8, 2019 17:45
@seejohnrun seejohnrun force-pushed the infer-migrations-paths branch 4 times, most recently from 766f6a0 to 5c07af4 Compare September 16, 2019 19:57
@seejohnrun seejohnrun force-pushed the infer-migrations-paths branch 4 times, most recently from 99ad51f to 019d07f Compare December 4, 2019 18:25
@rails-bot
Copy link

rails-bot bot commented Mar 3, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

seejohnrun and others added 4 commits June 4, 2020 16:03
Similar to what we do for schema files, we now infer the default
`migrations_paths` for a database configuration using the database
specification name.

We still allow overriding these defaults with a `migrations_paths` key
in the database configuration file.

Additionally, update some documentation accordingly in the guides.

We've also removed `Migrator.migrations_paths` since we should be taking
the `migrations_paths` off of the config instead.

Co-authored-by: eileencodes <eileencodes@gmail.com>
@eileencodes
Copy link
Member

So we don't forget again in the future:

The problem we hit with this PR is that we don't know which entry is the default and therefore should get db/migrate as its migrations paths and which entries should be inferred as their own name. We originally considered making primary required but eventually decided against that. I think there are a few ways to solve this but they all have caveats:

  1. Require that apps using inferred paths add db/migrate to at least one of the configurations so we know which one the default is. This is probably quite error prone, and confusing for users.
  2. Require that apps mark a configuration as default. This would help us in other places but getting this message out to apps may be difficult.
  3. Assume the first configuration is the default. We do this in other places, it could make a lot of sense, but it's a bit implicit and not really explicit. Potentially we could combine 2 and 3 - if there's a default defined, we use that for db/migrate, if not we assume it's the first one.

@rafaelfranca rafaelfranca removed this from the 6.1.0 milestone Nov 24, 2020
Base automatically changed from master to main January 14, 2021 17:01
@jasonkarns
Copy link
Contributor

This (or the lack of this) behavior just bit us. The docs indicate (as of 7.0.3.1) that option 3 above is followed

If a primary configuration is provided, it will be used as the "default" configuration. If there is no configuration named "primary", Rails will use the first configuration as default for each environment. The default configurations will use the default Rails filenames. For example, primary configurations will use schema.rb for the schema file, whereas all the other entries will use [CONFIGURATION_NAMESPACE]_schema.rb for the filename.

This phrasing (specifically: "will use the default Rails filenames") makes it seemingly reasonable to assume that the migration path would also be inferred as db/[CONFIGURATION_NAMESPACE]_migrate.

Weirdly enough some additional impact of migrations_paths defaulting that surprised us:

If a non-primary database does not need migrations (and therefore theoretically need not have migrations_path config), then a bunch of db:* rake tasks start failing. To whit:

  1. Rails defaults the secondary db to also use db/migrate by default
  2. So then rails sees the primary's migrations as pending for the secondary db
  3. Which triggers the "abort-if-pending-migrations" pre-req for many rake tasks.

This is especially odd when running rake tasks scoped to the primary db. (Which seems to indicate that the "abort-if-pending-migrations" check is not scoped to the db whose tasks are being requested.)

Example:

defaults:
  shared: &shared
    adapter: mysql2
    encoding: utf8mb4
    username: root
    password: pass
    host: 127.0.0.1
    port: 3306

  the_main: &the_main_defaults
    <<: *shared

  the_second: &the_second_defaults
    <<: *shared
    use_metadata_table: false

development:
  the_main:
    <<: *the_main_defaults
    database: main
  the_second:
    <<: *the_second_defaults
    database: second

rake db:reset:the_main some_other_task

drops the_main development db and then promptly fails/exits with: "You have N pending migrations:" (the_main's test db is not reset, nor is some_other_task run)

While it seems clear there are a handful of interrelated quirks with these db tasks; at least one prevention mechanism is for migrations_paths to be inferred just as the _schema.rb path is. (which would ensure pending migrations are not erroneously reported)

Some other strange behavior:
setting migrations_paths: nil works around the issue for us; however, migrations_paths: false does not! Of course, the "clean" solution is to set path to a legitimate value, even if the directory doesn't exist or is empty.

@eileencodes
Copy link
Member

If a non-primary database does not need migrations (and therefore theoretically need not have migrations_path config)

Will database_tasks: false not work for this? That will ignore tasks for the db you don't want migrations for.

It doesn't sound like the bugs you're discussing are directly related to this PR and unfortunately there are too many problems we hit to move forward at this time. If there are areas for improvement a new issue should be opened with a reproduction.

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.

None yet

4 participants