braintree / pg_ha_migrations

Enforces DDL/migration safety in Ruby on Rails project with an emphasis on explicitly choosing trade-offs and avoiding unnecessary magic.
MIT License
220 stars 23 forks source link

Ruby 3: Resolve Argument Error when running migrations on Ruby 3.x #73

Closed brettjnorris closed 2 years ago

brettjnorris commented 2 years ago

Issue

Running migrations in Rails app using Ruby 3.x throws an Argument Error.

Example migration:

 class CreateTest < ActiveRecord::Migration[7.0]
   def up
     safe_create_table :tests do |t|

       t.timestamps
     end
   end

   def down
     unsafe_drop_table :tests
   end
 end

Output:

== 20220617143144 CreateTest: migrating =======================================
-- create_table(:tests, {})
rake aborted!
StandardError: An error has occurred, all later migrations canceled:

wrong number of arguments (given 2, expected 1)
/Users/b.norris/repos/testapp/db/migrate/20220617143144_create_test.rb:3:in `up'

Caused by:
ArgumentError: wrong number of arguments (given 2, expected 1)
/Users/b.norris/repos/testapp/db/migrate/20220617143144_create_test.rb:3:in `up'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)

This can be verified by checking out my testapp, which uses Ruby 3.1.0 and Rails 7: https://github.com/brettjnorris/testapp

Cause

This is caused by some changes to positional and keyword arguments in Ruby 3. See https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

Solution

According to the document listed above (https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/), some changes will need to be made to support these new argument changes.

This PR adds ruby2_keywords as recommended to support Ruby 2.6 and earlier and updates a handful of method calls to pass options hashes with the double splat operator.

This was tested locally against Ruby 2.7 and 3.1. This can be verified by checking out this branch on my test app: https://github.com/brettjnorris/testapp/tree/with_updated_pg_ha_migrations

Additional Feedback

I'm open to feedback on this approach.

jcoleman commented 2 years ago

@brettjnorris Thanks for this; I can look at it myself at some point, but any chance you'd be able to see about updating the GitHub workflow config to add Ruby 3? That'd allow us to show everything is fine on CI.

brettjnorris commented 2 years ago

@brettjnorris Thanks for this; I can look at it myself at some point, but any chance you'd be able to see about updating the GitHub workflow config to add Ruby 3? That'd allow us to show everything is fine on CI.

Yes, I will look into this. I would feel more comfortable with this change with CI support as well.

brettjnorris commented 2 years ago

I have added update workflow configuration to use the Ruby Matrix as documented in https://github.com/ruby/setup-ruby. Github is requiring maintainer approval to run the workflow. Once that is done, I can verify the results and addresses issues if needed.

jcoleman commented 2 years ago

I just approved running the workflow; will be interesting to see the results.

jcoleman commented 2 years ago

@brettjnorris This failed https://github.com/braintree/pg_ha_migrations/runs/7031578014?check_suite_focus=true

What do you think about narrowing the Ruby versions to 2.7 and 3.0 for now to make it easier to troubleshoot?

jcoleman commented 2 years ago

@brettjnorris Looks like some of these errors may be using Rails versions on Ruby versions they don't support? https://github.com/braintree/pg_ha_migrations/runs/7074879823?check_suite_focus=true

brettjnorris commented 2 years ago

@jcoleman I double checked the Rails versions that are supported by Ruby 3.x and it looks like 6.1 and newer are the only Rails versions that officially support Ruby 3. I updated the matrix configuration to (hopefully) only include ruby 3 for the support rails versions.

jcoleman commented 2 years ago

@brettjnorris Looks like there's somehow now a config without a PG version: https://github.com/braintree/pg_ha_migrations/runs/7078663146?check_suite_focus=true

jcoleman commented 2 years ago

This looks like possibly a real failure? https://github.com/braintree/pg_ha_migrations/runs/7118872305?check_suite_focus=true

brettjnorris commented 2 years ago

This looks like possibly a real failure? https://github.com/braintree/pg_ha_migrations/runs/7118872305?check_suite_focus=true

It looks like there may have been an issue with Rails 7.0.0 and Ruby 3.1.0 according to this thread: https://github.com/rails/rails/issues/43998. Rails 7.0.1 included a fix for this issue.

I bumped the Rails version in the Rails 7 gemfile to Rails 7.0.1 as that resolved the failure in my local testing.

jcoleman commented 2 years ago

@brettjnorris I've release 1.6.0 with this fix. Thanks for contributing!

brettjnorris commented 2 years ago

@jcoleman thank you for putting the release together. And I appreciate your help working out the issues with the tests.