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

Compatibility with Rails 5.0 #249

Closed
mvastola opened this issue Dec 21, 2015 · 15 comments
Closed

Compatibility with Rails 5.0 #249

mvastola opened this issue Dec 21, 2015 · 15 comments

Comments

@mvastola
Copy link
Contributor

With the release of Rails 5.0.0.beta1 just days ago, many users (myself included) are seeking to upgrade their current Rails 4.2 apps to ensure forward-compatibility with this new version of Rails and to make use of its new features and capabilities.

Currently, the audited.gemspec includes the line:

gem.add_dependency 'activerecord', '~> 4.0'

which prevents usage of this gem (or even experimentation with it) in this new version of Rails.

I would like to request that audited be made compatible with Rails 5.0.

@danielmorrison
Copy link
Member

I'll try to get this today or tomorrow. Pull requests welcome in the meantime! ;)

@mvastola mvastola changed the title Compatability with Rails 5.0 Compatibility with Rails 5.0 Dec 22, 2015
@mvastola
Copy link
Contributor Author

@danielmorrison
I just began work on a pull request (https://github.com/Partyista/audited/tree/rails5-pr-work) assuming you hadn't gotten a chance yet, but I'm running into an issue: specifically, rails-observers is also version-locked.

So I'm going to see if I can get that working and then I will submit my commit as a PR (assuming all tests pass).

@mvastola
Copy link
Contributor Author

Also, I wasn't sure what to do about your .travis.yml. I didn't change anything regarding this, but Rails 5 requires Ruby >= 2.2.2, so CI tests for Rails 5 on other versions of Ruby will definitely fail (which will be confusing in that it will presumably mark future valid PRs as having failed the CI testing).

I'm not too fluent in Travis' config.. is there a way to specify that the Rails 5 appraisal should only be tested on that one Ruby version?

@mvastola
Copy link
Contributor Author

@danielmorrison FYI, I have submitted rails/rails-observers#39, a PR to rails-observers which fixes various issues with its test suite (and very minor issues with the library itself) to verify full Rails 5.0 compatibility. I have further asked the maintainer(s) to release a new version following the adoption of my PR there.

Once that happens, I have a PR nearly ready to go (it just needs the new version number of rails-observers) for audited which I will submit to you to enable Rails 5.0 interoperability.

I have verified that audited's test suite passes against my rails-observers PR for now by locally adding the following to Appraisals in audited:

appraise 'rails50' do
  gem 'rails', '>= 5.0.0.beta1', '< 5.1'
  gem 'rails-observers', :github => 'Partyista/rails-observers', :branch => 'pr-fix-tests'
end

And running the following in terminal:

mvastola@vastdesk:~/src/audited$ appraisal rails50 rake test
>> BUNDLE_GEMFILE=/media/DatastoreCrypt/home/mvastola/src/audited/gemfiles/rails50.gemfile bundle exec rake test
/usr/local/rvm/rubies/ruby-2.2-head/bin/ruby -I"lib:test" -I"/usr/local/rvm/rubies/ruby-2.2-head/lib/ruby/2.2.0" "/usr/local/rvm/rubies/ruby-2.2-head/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/install_generator_test.rb" "test/upgrade_generator_test.rb" 
DEPRECATION WARNING: `serve_static_files` is deprecated and will be removed in Rails 5.1.
Please use `public_file_server.enabled = true` instead.
 (called from block in <top (required)> at /media/DatastoreCrypt/home/mvastola/src/audited/spec/rails_app/config/environments/test.rb:16)
DEPRECATION WARNING: `static_cache_control` is deprecated and will be removed in Rails 5.1.
Please use
`config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }`
instead.
 (called from block in <top (required)> at /media/DatastoreCrypt/home/mvastola/src/audited/spec/rails_app/config/environments/test.rb:17)
DEPRECATION WARNING: around_filter is deprecated and will be removed in Rails 5.1. Use around_action instead. (called from block in <top (required)> at /media/DatastoreCrypt/home/mvastola/src/audited/lib/audited/sweeper.rb:59)
Run options: --seed 13105

# Running:

.......

Finished in 0.127554s, 54.8787 runs/s, 290.0733 assertions/s.

7 runs, 37 assertions, 0 failures, 0 errors, 0 skips

If you could, your endorsement in favor of merging my pull request to rails-observers (or simply your statement that Rails 5.0 compatibility would help this gem), though not crucial, might help accelerate this process.

@kelhusseiny
Copy link

@mvastola I really need a compatible version with rails5, when do you think you will be ready to merge this PR?

@mvastola
Copy link
Contributor Author

mvastola commented Jan 1, 2016

@Azzurrio rails-observer only literally just merged my PR less than an hour ago (audited is dependent on rails-observer). Now I just need them to push a new version of their gem so that audited can require that version at minimum.

I will, however, put together an audited PR now if you'd like to point your Gemfile to my fork in the interim.

@mvastola
Copy link
Contributor Author

mvastola commented Jan 1, 2016

@Azzurrio See https://github.com/Partyista/audited/tree/rails5-pr-work
You will need to also put a reference to rails-observer's Github repo (master branch) in your Gemfile.

@danielmorrison If you could check out #252 and let me know if everything is to your satisfaction (other than the reference to the rails-observer Github) I'd appreciate it.

@mvastola
Copy link
Contributor Author

mvastola commented Jan 2, 2016

#252 is now working except for one test that isn't passing with MySQL and PostgreSQL when using Rails 5 with multithreading. I'm going to look more closely into this, but if anyone has any ideas, I'd very much appreciate them.

Update: This was due to Rails 5's new support for nested transactions coinciding with the fact that use_transactional_fixtures defaulted to true. It prevented the newly created record from appearing to the other threads because the transaction hadn't committed. In any case, it is now fixed.

@mvastola
Copy link
Contributor Author

mvastola commented Jan 2, 2016

Sorry for the delay. I had recalled this as being as simple as a version change, but I guess I had been thinking of another gem (I've been juggling a bunch of 5.0 upgrades as of late). Regardless, this PR is now passing all tests and ready to try out.

@kelhusseiny
Copy link

@mvastola Great Work 👍 I will you use your fork untill ur PR is merged

@mvastola
Copy link
Contributor Author

mvastola commented Jan 2, 2016

@Azzurrio Cool. Definitely let me know if you run into any issues with my fork!

@kelhusseiny
Copy link

@mvastola I've added this to Gemfile

gem "audited", github: "Partyista/audited", branch: "rails5-pr-work"

After running bundle install I ran into this error

Bundler could not find compatible versions for gem "activemodel":
  In snapshot (Gemfile.lock):
    activemodel (= 5.0.0.alpha)

  In Gemfile:
    rails was resolved to 5.0.0.alpha, which depends on
      activemodel (= 5.0.0.alpha)

    audited was resolved to 4.2.0, which depends on
      rails-observers (~> 0.1.2) was resolved to 0.1.2, which depends on
        activemodel (~> 4.0)

The problem here is that the rails-observers gem hasn't released yet. So I've added

gem "rails-observers", github: "rails/rails-observers"

and it works perfectly after then 💃
So I just wanna mentioned this as note for anyone who want to use audited with rails 5 and also rails-observers maintainers should release a new version ASAP concurrently with your PR here.

Summary:
Just add this to Gemfile and run bundle install

gem "rails-observers", github: "rails/rails-observers"
gem "audited", github: "Partyista/audited", branch: "rails5-pr-work"

@mvastola
Copy link
Contributor Author

mvastola commented Jan 2, 2016

Yup. :-) In this comment I already mentioned:

You will need to also put a reference to rails-observer's Github repo (master branch) in your Gemfile.

I've already asked rails-observers to push a new release with my newly-merged PR. If you'd be willing to throw your name into the ring as having an interest in rails-observers doing so, by all means make a comment 👍 ing my request.

@mvastola
Copy link
Contributor Author

mvastola commented Jan 2, 2016

@Azzurrio Also, rails 5.0.0.beta1 is out, so you may want to upgrade from the alpha (though this is unrelated to your issue).

@kelhusseiny
Copy link

@mvastola Yeah I know. But after upgrading to beta1 I ran into 2 errors. First one related to database config prepared_statement option, It's basically when setting that option to false it causes an error, After some debugging I figured that It's activerecord wrong if statement condition. I did a Pull Request for that in Rails but I think they are waiting for tests. May you check it and help me in that if you've some time.
The second error was related to Heroku in production, while trying accessing the console we got an error

NameError: undefined local variable or method `broadcast_messages' for #<RailsStdoutLogging::StdoutLogger:0x007f2842237500>

I haven't invested time on that but the first problem was enough to rollback into rails alpha again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants