-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduce a configuration value for migration_path
#4190
Introduce a configuration value for migration_path
#4190
Conversation
1cbc7dd
to
74b276c
Compare
Awesome! This is exactly what I was thinking! Thank you @forkata! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@forkata thank you for adding this new preference 🎉
I left a suggestion for a small improvement, let me know if it works for you
Since Rails 6 multiple database support is a native feature, which allows users to configure separate read/write databases. This change allows users to configure the path to the migrations folder from the default "db/migrate" if they are using the multiple database support and have specified a different database for the Solidus engine. This will ensure the check that all migrations are installed can look at a user configurable folder. One change required to make the folder path configurable is to run the migration check after 'load_config_initializers' instead of before. This will allow the value of `migration_path` configuration to be set in an initializer. Co-authored-by: Adam Mueller <adam@super.gd> Co-authored-by: Alex Blackie <alex@super.gd> Co-authored-by: Benjamin Willems <benjamin@super.gd> Co-authored-by: Jared Norman <jared@super.gd>
74b276c
to
9eaacc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @forkata 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks SuperGood! 👍
Interesting. @kennyadsl pointed out that the change to when the hook runs was added by Clarke in a commit that was trying to get the configuration stuff set up before initializers ran. My guess is that the migration check got lumped into that change not realizing it didn't really matter (unlike the rest of the hooks in that commit, which you definitely do want to run before your initializers.) |
There is a small downside to not checking migrations before running initializers and that is in the case where someone is accessing models from their initializers. I am not sure how common that use case is, so I am not too concerned about degrading the user experience with this change. |
@forkata We were discussing that in the Core meeting. What exact situation could someone get into where this would be a problem? |
I don't think there is a case where this would be a problem, it's more that the messaging would not be as clear as it would be prior to this change. The situation I am thinking of is you have not run a particular migration to create a table, but you are trying to reference the corresponding model from an initializer. |
Description
Since Rails 6 multiple database support is a native feature, which
allows users to configure separate read/write databases. This change
allows users to configure the path to the migrations folder from the
default "db/migrate" if they are using the multiple database support and
have specified a different database for the Solidus engine. This will
ensure the check that all migrations are installed can look at a user
configurable folder.
One change required to make the folder path configurable is to run the
migration check after 'load_config_initializers' instead of before. This
will allow the value of
migration_path
configuration to be set in aninitializer.
Ref #4182
Checklist:
I have attached screenshots to this PR for visual changes (if needed)