Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

rake railties:install:migrations respects the order of railties #8930

Merged
merged 1 commit into from Jan 15, 2013

Conversation

Projects
None yet
4 participants
Contributor

cordawyn commented Jan 14, 2013

I have a couple of projects that make use of Rails' engines. There are two engines with migrations, where one engine depends on having a database table created in a migration in another engine. Unfortunately, "railties:install:migrations" task does not respect the order of engines that one can configure via "Rails.application.config.railties_order". This simple commit fixes that by using "ordered_railties" instead of "railties" (and thus the migration files are copied to the main application in the correct order).

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Jan 14, 2013

activerecord/CHANGELOG.md
@@ -1226,4 +1226,8 @@
*Aaron Patterson*
+* Rake task "railties:install:migrations" respects the order of railties.
+
+ *Slava Kravchenko*
@carlosantoniodasilva

carlosantoniodasilva Jan 14, 2013

Owner

Please move this to the top, and change the quotes to backticks so it will probably show as an inline code block when viewing as markdown. Make sure you amend your commit and push force. Thanks!

@cordawyn

cordawyn Jan 14, 2013

Contributor

Done! (Oops, made it another commit, sorry)

Member

steveklabnik commented Jan 14, 2013

(Oops, made it another commit, sorry)

That's fine, just rebase, squash, and force push.

Contributor

cordawyn commented Jan 14, 2013

All done. Thanks for the hint, @steveklabnik

Member

steveklabnik commented Jan 14, 2013

Any time! :)

I'll let @carlosantoniodasilva merge, though.

@carlosantoniodasilva carlosantoniodasilva added a commit that referenced this pull request Jan 15, 2013

@carlosantoniodasilva carlosantoniodasilva Merge pull request #8930 from cordawyn/ordered_railties
rake railties:install:migrations respects the order of railties
8348f9e

@carlosantoniodasilva carlosantoniodasilva merged commit 8348f9e into rails:master Jan 15, 2013

Thanks ❤️

@cordawyn this has failed some railties tests. The solution would be to move the ordered_railties method to public, but leaving it as nodoc since it's not public for user consumption, just for the framework.

I tried doing that, but then I got a new failure:

  1) Failure:
test_copying_migrations(RailtiesTest::EngineTest) [test/railties/engine_test.rb:102]:
Expected /2_create_users/ to not match "NOTE: Migration 3_create_sessions.rb from bukkits has been skipped. Migration with the same name already exists.\nNOTE: Migration 1_create_sessions.rb from app_template_application has been skipped. Migration with the same name already exists.\nNOTE: Migration 2_create_users.bukkits.rb from app_template_application has been skipped. Migration with the same name already exists.\nNOTE: Migration 3_add_last_name_to_users.bukkits.rb from app_template_application has been skipped. Migration with the same name already exists.".

I don't have time to look at it now, can you please take a look and send a pull request fixing it if you find the issue? Otherwise I'll have to revert, unfortunately. Thanks!

Contributor

cordawyn commented Jan 15, 2013

I'll take a look at that, sure.

Contributor

cordawyn commented Jan 15, 2013

I think you can revert the commit for now, because this is going to take me more than a day, I'm afraid. I'll get back with another version of the fix later.

@carlosantoniodasilva carlosantoniodasilva added a commit that referenced this pull request Jan 15, 2013

@carlosantoniodasilva carlosantoniodasilva Revert "Merge pull request #8930 from cordawyn/ordered_railties"
This reverts commit 8348f9e, reversing
changes made to 9dfe2d6.

Reason: this broke railties tests as explained in the issue, and the
author is going to review and report back.

#8930 (comment)
c372eec

@tenderlove tenderlove added a commit to tenderlove/rails that referenced this pull request Jan 15, 2013

@tenderlove tenderlove Merge branch 'master' into keithgabryelski-partitioned4
* master: (2054 commits)
  Change the behavior of route defaults
  Add support for other types of routing constraints
  Ensure port is set when passed via the process method
  Raise correct exception now Journey is integrated.
  Revert "Merge pull request #8930 from cordawyn/ordered_railties"
  Revert "log at debug level what line caused the redirect_to"
  Improve mysql database tasks handling to ensure we always rescue from an exception
  Revert "Merge pull request #8942 from yahonda/tested_only_with_mysql"
  ActiveRecord <-> Active Record [ci skip]
  Use Rails 4 find_by in README [ci skip]
  Account for ignored cookie set by turbolinks
  test for ActiveModel::Conversion#to_partial_path and namespaced models
  Address test_create_when_database_exists_outputs_info_to_stderr failures
  Rename :value option to :selected, in line with other select helpers Add tests for time & datetime. Add documentation.
  Revert benchmark helper regression. Use a #capture within a #benchmark block. Breaks benchmark calls that return non-String values otherwise.
  rake railties:install:migrations respects the order of railties
  Test to allow Range including DateTime and DateTime::Infinity
  Add regression test to #8907
  Allow value to be set on date_select
  Remove WIP from Working With JavaScript Guide.
  ...

Conflicts:
	activerecord/lib/active_record/model_schema.rb
	activerecord/lib/active_record/persistence.rb
3357ae9

@tenderlove tenderlove added a commit that referenced this pull request Jan 17, 2013

@tenderlove tenderlove Merge branch 'master' into jobs
* master: (39 commits)
  Deprecate direct calls to AC::RecordIdentifier.dom_id and dom_class
  clear specific logs when using rake log:clear
  Fix date_select :selected option so you can pass it nil
  document Intercepters in ActionMailer guide
  Don't rely on Hash key's ordering
  Remove warnings: "(...) interpreted as grouped expression"
  adding regression test in master for #8631
  strong parameters exception handling
  Remove header bloat introduced by BestStandardsSupport middleware
  allow :dirs option for .enumerate
  use case statement
  Change the behavior of route defaults
  Add support for other types of routing constraints
  Ensure port is set when passed via the process method
  Raise correct exception now Journey is integrated.
  Update guides/source/active_record_validations.md
  Revert "Merge pull request #8930 from cordawyn/ordered_railties"
  Cleaning up ActiveModel::Dirty tests
  Revert "log at debug level what line caused the redirect_to"
  Improve mysql database tasks handling to ensure we always rescue from an exception
  ...

Conflicts:
	guides/source/action_mailer_basics.md
04c23f2
Contributor

parndt commented Jan 18, 2013

Hey I opened #8985 to hopefully address all issues.

@arunagw arunagw added a commit to arunagw/rails that referenced this pull request May 23, 2014

@arunagw arunagw rake railties:install:migrations respects the order of railties
This PR fixes #8930 and some stuff from #8985
e262c12

@arunagw arunagw added a commit to arunagw/rails that referenced this pull request May 23, 2014

@arunagw arunagw rake railties:install:migrations respects the order of railties
This PR fixes #8930 and some stuff from #8985
203059b

@arunagw arunagw added a commit to arunagw/rails that referenced this pull request May 27, 2014

@arunagw arunagw rake railties:install:migrations respects the order of railties
This PR fixes #8930 and some stuff from #8985
50de394

@seuros seuros added a commit to seuros/django that referenced this pull request May 29, 2014

@arunagw @seuros arunagw + seuros rake railties:install:migrations respects the order of railties
This PR fixes #8930 and some stuff from #8985
eed2f9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment