Mange / roadie-rails

Making HTML emails comfortable for the Rails rockstars
MIT License
363 stars 65 forks source link

Add support for rails 5 #50

Closed sgringwe closed 8 years ago

sgringwe commented 8 years ago

Fixes https://github.com/Mange/roadie-rails/issues/49.

The specs are failing on travis but I did get them to pass locally. There is an issue installing gems on travis. I am wondering if this is due to rails 5 now requiring ruby 2.2.2?

EDIT: I did try running setup locally on 2.2 and I got a similar error, but with rack.

sgringwe commented 8 years ago

@Mange friendly ping to see if this can be merged

Mange commented 8 years ago

Sorry for not getting back earlier. I've been busy over the holidays.

There are a few files and let-over configs that could be stripped away, for example the database config and the tmp/cache-development.txt stuff for memcaching locally.

I'm also a bit confused about the build failures. It cannot install a dependency, so that should probably hint about some sort of dependency problem. I could take a look if you don't figure it out.

sgringwe commented 8 years ago

No problem @Mange . I'll work on removing the extra files and see if I can fix dependency

sgringwe commented 8 years ago

@Mange i've gone ahead and removed the unnecessary caching bit. I attempted to remove database.yml, but that resulted in an error in active_record's railtie. Not sure how you'd like to proceed there.

After more searching, I'm feeling fairly confident that the error in CI is due to a ruby dependency issue. activesupport 5.0.0.beta1 (and rails 5.0.0.beta1) require ruby 2.2.2 (https://rubygems.org/gems/activesupport/versions/5.0.0.beta1) and so on the old ruby versions, it cannot install. How would you like to handle that? Can we only test rails 5 on 2.3?

This may now be dependent on https://github.com/Mange/roadie-rails/pull/51.

sgringwe commented 8 years ago

I've added a check in the integration spec to only run rails 5 specs if ruby <= 2.2.2. Let me know what you think.

EDIT: Looks like I need to also edit setup.sh to not attempt installing gems for rails 5 on ruby < 2.2.2, as well. Looking around there seems there may be more elegant ways on travis to do this (https://github.com/collectiveidea/audited/pull/252/files). I don't know much about travis, and don't want to go too far into the wrong direction so I'm going to wait for direction for now.

eileencodes commented 8 years ago

Other than fixing the travis build, @sgringwe is there anything left on this PR that you need help with? Rails RC1 is coming out soon. We also use the old version of this gem at Basecamp and I'd love to help get this in so I don't have to use our forked version anymore. Let me know how I can help. :smile:

Mange commented 8 years ago

master should be green again. Please merge it to get better build information.

I have no suggestion right now on how to handle the conditional Ruby versions[1], but I think most working solutions would be okay by me right now. I could always adjust later if I find a better way.

I'm probably not going to the Appraisals path again. I want a full-stack integration test, and when I was using Appraisal before I had a lot of problems to get the integration tests at a good level – I ended up stubbing a lot of Rails things. Maybe that's easier now when roadie-rails is extracted from roadie, but in any case it's not something for this specific PR. :smile:

[1]: Well, just to get the ball running... It's fine to pass parameters to the setup.sh script (for example the RUBY_VERSION) from the Rakefile. The script could then skip Rails 5 unless Ruby version is high enough. integration_spec could then also skip Rails 5 on those versions. This is a very hacky solution, but maybe this plants enough ideas for someone to take a stab at it. I no one does, I might try it out myself actually. [EDIT: The setup.sh script is my own in this repo. It's fine to be "less generic" in it. if [[ $foo == "rails_50" ]] is not forbidden right now. I'd rather extract abstract concepts when I get more than 3 instances of them, following the adage that "The wrong abstraction is worse than duplication".]

sgringwe commented 8 years ago

Thanks all, I'll take a look later

sgringwe commented 8 years ago

@Mange i believe this is good to go now

codecov-io commented 8 years ago

Current coverage is 97.29%

Merging #50 into master will not affect coverage as of e61d968

@@            master     #50   diff @@
======================================
  Files           21      21       
  Stmts          592     592       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            576     576       
  Partial          0       0       
  Missed          16      16       

Review entire Coverage Diff as of e61d968

Powered by Codecov. Updated on successful CI builds.

cpence commented 8 years ago

Following as well, this is AFAICT the last gem in my app's stack that I need to get a bump for Rails 5 testing. Thanks!

mparramont commented 8 years ago

Following here as well. Keep up the good work people! :)

Mange commented 8 years ago

Great work, people. I'll merge it now.

Mange commented 8 years ago

I just pushed version 1.1.1 with this included. Thank you for your contribution @sgringwe. :heart:

bmulholland commented 8 years ago

Thank you @sgringwe and @Mange !

sgringwe commented 8 years ago

Thanks for merging!