Add Rails master appraisals in preparation for Rails 5 #1976

maclover7 · 2015-08-28T21:54:40Z

#1973

Also reorganized Appraisals to make it easier to understand

tute · 2015-09-09T02:14:06Z

Appraisals

-appraise "4.2.awsv1" do
-  gem "rails", "~> 4.2.0"
+appraise "master.awsv1" do
+  gem 'rails', github: "rails/rails"
  gem "aws-sdk", "~> 1.5"
 end


Do we need to take this combination into account? When is AWS v1 end of life?

Not sure about v1 support (this would be another big breaking change deprecation), but I can look into this.

tute · 2015-09-09T02:15:43Z

Thanks so much @maclover7! Seems like the red CI comes from rails#master? Should I restart the build (or you trigger it rebasing your commit on top of latest master)?

Thanks again!

maclover7 · 2015-09-09T21:16:59Z

Pushed up change that should fix dependency resolution problems - hopefully we can see whether the test suite passes on master now.

tute · 2015-09-09T21:20:03Z

Not sure about v1 support (this would be another big breaking change deprecation), but I can look into this.

I don't know how AWS works. But if people are moving on to v2, as most Rails apps moved to 4, then it could be plausible to say "paperclip 5 only with AWS v2". But to be fair the code is already written, and I wouldn't expect it to change much, so it might not be worth it if it will bring unhappy users.

maclover7 · 2015-09-09T22:12:51Z

If we are going to deprecate AWS v1 support, I'd say we should include deprecation warnings in the next release out the door, to give time to upgrade.

tute · 2015-09-09T22:17:37Z

If we are going to deprecate AWS v1 support, I'd say we should include deprecation warnings in the next release out the door, to give time to upgrade.

👍 👏

maclover7 · 2015-09-09T22:22:05Z

Should I open up a ticket on rails/rails for that "syntax error" that's breaking the rails master travis build? https://travis-ci.org/thoughtbot/paperclip/jobs/79552598#L237 @tute

tute · 2015-09-10T05:18:06Z

Should I open up a ticket on rails/rails for that "syntax error" that's breaking the rails master travis build? https://travis-ci.org/thoughtbot/paperclip/jobs/79552598#L237 @tute

It looks like that's Ruby >2 syntax. We should add an exception for Rails master/5 and Ruby 2 CI run (see for example https://github.com/doorkeeper-gem/doorkeeper/blob/master/.travis.yml#L16-L18).

maclover7 · 2015-09-16T21:02:12Z

@tute Can you check my .travis.yml syntax - updated it in last commit to try and stop 2.0 and rails master from running

tute · 2015-09-17T19:06:41Z

.travis.yml


 matrix:
  fast_finish: true
  allow_failures:
    - rvm: jruby-19mode
    - rvm: rbx-2
+  exclude:
+    - gemfile: master.awsv1.gemfile


Do we need to add the gemfiles/ path?

yep, i think that's what the problem is. will squash down all of my commits before merging in :P

yep, i think that's what the problem is. will squash down all of my commits before merging in :P

Excellent, and rebase on top of latest master too. Thank you, @maclover7! 👏

maclover7 · 2015-09-18T21:01:36Z

Rebased and consolidated --> just for maintenance of the code, is there any way to streamline our gemfiles and appraisals? Getting very cluttered now by testing 4 versions of Rails and 3 versions of aws-sdk. cc @jyurek

tute · 2015-09-25T15:48:15Z

I've been restarting CI but we always get the same timeout. Makes me think there's actually something wrong with either rails or our code?

tute · 2015-11-12T03:27:30Z

Rebased your branch on top of latest master. Right now, master is red for some cucumber steps, so if we get those it's not related with changes in this branch.

That said, latest CI run had the following failure that we'll need to address:

/home/travis/build/thoughtbot/paperclip/spec/spec_helper.rb:11:in `require': cannot load such file -- activerecord-import (LoadError)

Thanks!

tute · 2015-11-12T03:28:34Z

.travis.yml

  - gemfiles/4.2.awsv1.gemfile
+  - gemfiles/4.2.awsv2.0.gemfile
+  - gemfiles/4.2.awsv2.1.gemfile
+  - gemfiles/master.awsv1.gemfile


I so want to get rid of AWS V1. :|

tute · 2015-11-18T05:26:56Z

This PR is back to "normal" failures, to be worked on.

tute · 2015-12-22T22:01:58Z

@maclover7 will rebase your branch, and make it point to 5 beta1, à la doorkeeper-gem/doorkeeper#753, unless you'd rather do it yourself, or do it differently. Thanks!

tute · 2015-12-22T22:08:29Z

Will start with smaller PRs removing from master unsupported versions of Rails (3.2 and 4.1), and AWS v1.

maclover7 · 2015-12-22T22:46:30Z

Rebased! Going to have a ton of time to work on this after tomorrow (yay school vacation!). Key stuff is to get out a 4.x release with deprecations, and then to move forward with 5.0.0, after Rails 5 is released.

tute · 2015-12-22T22:48:08Z

That is great! You might want to rebase again after #2074, with smaller CI matrix.

I will now work on removing from master AWS v1 support.

tute · 2015-12-23T04:44:00Z

.travis.yml

-  - gemfiles/4.2.awsv1.gemfile
+  - gemfiles/4.2.awsv2.1.gemfile
+  - gemfiles/master.awsv2.0.gemfile
+  - gemfiles/master.awsv2.1.gemfile


Should we do master or 5.0.0.beta1?

tute · 2016-01-04T19:05:39Z

Did Rails 5 change Validators API? It looks like AttachmentContentTypeValidator's not option is failing.

Fixing spec/paperclip/validators_spec.rb will fix the others for Rails 5, and we can merge.

tute · 2016-01-05T12:27:28Z

@sgrif: is the behavior described in #1976 (comment) an expected ActiveRecord change? Thank you.

tute · 2016-03-12T21:42:38Z

The reason specs fail in Rails 5 seem to be that this validation block never runs: https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/validators.rb#L67-L69

send(:"before_#{attribute}_post_process", :if => if_clause, :unless => unless_clause) do |*args|
  validator_class.new(options.dup).validate(self)
end

I can't spot why that is.

tute · 2016-03-12T21:50:54Z

@rafaelfranca can you spot if a change in Rails 5 around callbacks (aside from the new option ActiveSupport.halt_callback_chains_on_return_false) might be messing something up here? Thank you!

rafaelfranca · 2016-03-15T02:03:56Z

Not that I'm aware. The only change I remember is the way we internally execute a callback but that also changes in Rails 4.2 so you would be able to see the same problem in 4.2 if it was caused by that.

houndci-bot · 2016-03-23T03:24:15Z

spec/support/reporting.rb

+module Reporting
+  def silence_stream(stream)
+    old_stream = stream.dup
+    stream.reopen(RbConfig::CONFIG['host_os'] =~ /mswin|mingw/ ? 'NUL:' : '/dev/null')


Line is too long. [86/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

tute · 2016-03-23T03:35:35Z

@kitop thank you very much for your help on this! I'd merge it as is, let me know if you'd like to change anything on the code or the commits before doing so.

- Also bump Travis Ruby 2.2.x version to 2.2.4

`bundle exec rails -v` was taking seconds every time, making the test suite run very slow. This changes the code to compare the version against `ActiveRecord::VERSION`, which is going to be the same one as `rails -v`, making it much, much faster. Simplifies condition: we no longer support Rails < 4.2, so we test for `>= "5.0"` or fallback to `4.2` branch.

Ref: rails/rails@2386daa Specifically: > Chains of callbacks defined **with** a `:terminator` option will > maintain their existing behavior of halting as soon as a `before_` > callback matches the terminator's expectation. For instance, > ActiveModel's callbacks will still halt the chain when a `before_` > callback returns `false`. In terminator callbacks, the `result` value changed to be a lambda now. This change reflects that, updating the terminator to compare the result of the block to `false`. Also: * Removes Rails 4.1 branch (we don't support it anymore) * Rewrite for legibility: change `unless` for `if`

Ref: rails/rails@481e49c `silence_stream` was deprecated in Rails 4.2 and removed in Rails 5 because it was not thread safe. Paperclip is not running the tests in threaded mode, it's safe to add the method back in a test support module.

@kitop

While working on this branch, Kito found that this test fails due to: rails/rails@6ec8ba1#diff-fdcf8b65b5fb954372c6fe1ddf284c78R76 We are not yet sure if it's a bug in paperclip or in Rails itself. With current `ActiveModel::Errors` implementation the following happens: ``` record.errors # => @messages = {} record.errors.include?(:foo) # => false record.errors # => @messages = { :foo => [] } ``` Which bit us in: https://github.com/thoughtbot/paperclip/blob/69f18375333234b6f395300266e2612936bd242e/lib/paperclip/validators/attachment_file_name_validator.rb#L23 Another related Rails commit: rails/rails@b97035d I worked around the issue by changing what we assert in this spec. I am still not sure that this is a bug in current Rails master. cc @kitop for review

tute reviewed Sep 9, 2015
View reviewed changes

tute mentioned this pull request Sep 9, 2015

Rails 5 Support #1973

Closed

tute reviewed Sep 17, 2015
View reviewed changes

maclover7 force-pushed the add-rails-master-appraisals branch from 7ab68cf to 57637b0 Compare September 18, 2015 20:59

tute force-pushed the add-rails-master-appraisals branch from ba25c10 to 41a6c8e Compare November 12, 2015 03:25

tute reviewed Nov 12, 2015
View reviewed changes

tute added this to the v5.0.0 milestone Nov 12, 2015

maclover7 mentioned this pull request Nov 12, 2015

V5.0 Release Plan #2049

Closed

9 tasks

tute force-pushed the add-rails-master-appraisals branch from 41a6c8e to 8e4095f Compare November 17, 2015 04:56

maclover7 force-pushed the add-rails-master-appraisals branch from 8e4095f to e080faf Compare December 22, 2015 22:44

maclover7 force-pushed the add-rails-master-appraisals branch from e080faf to 2e01f84 Compare December 22, 2015 23:00

tute reviewed Dec 23, 2015
View reviewed changes

tute force-pushed the add-rails-master-appraisals branch 2 times, most recently from a80fc61 to 8016fcb Compare January 4, 2016 17:59

tute force-pushed the master branch from 017fead to e60f000 Compare March 12, 2016 18:57

tute force-pushed the add-rails-master-appraisals branch 5 times, most recently from 1c0d262 to 22c522f Compare March 12, 2016 21:41

tute force-pushed the add-rails-master-appraisals branch 4 times, most recently from 1579de1 to 1c07805 Compare March 23, 2016 03:24

houndci-bot reviewed Mar 23, 2016
View reviewed changes

maclover7 and others added 5 commits March 23, 2016 08:53

Add Rails master appraisals in preparation for Rails 5

caa7c22

- Also bump Travis Ruby 2.2.x version to 2.2.4

Add silence_stream method removed in Rails 5

93c34e7

Ref: rails/rails@481e49c `silence_stream` was deprecated in Rails 4.2 and removed in Rails 5 because it was not thread safe. Paperclip is not running the tests in threaded mode, it's safe to add the method back in a test support module.

tute force-pushed the add-rails-master-appraisals branch from 1c07805 to 799845e Compare March 23, 2016 12:53

tute merged commit d97a6c6 into master Mar 23, 2016

tute deleted the add-rails-master-appraisals branch March 23, 2016 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rails master appraisals in preparation for Rails 5 #1976

Add Rails master appraisals in preparation for Rails 5 #1976

maclover7 commented Aug 28, 2015

tute Sep 9, 2015

maclover7 Sep 9, 2015

tute commented Sep 9, 2015

maclover7 commented Sep 9, 2015

tute commented Sep 9, 2015

maclover7 commented Sep 9, 2015

tute commented Sep 9, 2015

maclover7 commented Sep 9, 2015

tute commented Sep 10, 2015

maclover7 commented Sep 16, 2015

tute Sep 17, 2015

maclover7 Sep 18, 2015

tute Sep 18, 2015

maclover7 commented Sep 18, 2015

tute commented Sep 25, 2015

tute commented Nov 12, 2015

tute Nov 12, 2015

tute commented Nov 18, 2015

tute commented Dec 22, 2015

tute commented Dec 22, 2015

maclover7 commented Dec 22, 2015

tute commented Dec 22, 2015

tute Dec 23, 2015

tute commented Jan 4, 2016

tute commented Jan 5, 2016

tute commented Mar 12, 2016

tute commented Mar 12, 2016

rafaelfranca commented Mar 15, 2016

houndci-bot Mar 23, 2016

tute commented Mar 23, 2016

Add Rails master appraisals in preparation for Rails 5 #1976

Add Rails master appraisals in preparation for Rails 5 #1976

Conversation

maclover7 commented Aug 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented Sep 9, 2015

maclover7 commented Sep 9, 2015

tute commented Sep 9, 2015

maclover7 commented Sep 9, 2015

tute commented Sep 9, 2015

maclover7 commented Sep 9, 2015

tute commented Sep 10, 2015

maclover7 commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maclover7 commented Sep 18, 2015

tute commented Sep 25, 2015

tute commented Nov 12, 2015

Choose a reason for hiding this comment

tute commented Nov 18, 2015

tute commented Dec 22, 2015

tute commented Dec 22, 2015

maclover7 commented Dec 22, 2015

tute commented Dec 22, 2015

Choose a reason for hiding this comment

tute commented Jan 4, 2016

tute commented Jan 5, 2016

tute commented Mar 12, 2016

tute commented Mar 12, 2016

rafaelfranca commented Mar 15, 2016

Choose a reason for hiding this comment

tute commented Mar 23, 2016