-
Notifications
You must be signed in to change notification settings - Fork 175
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
Upgrade to ruby 2.4 #1349
Upgrade to ruby 2.4 #1349
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I guess.
@@ -103,7 +103,7 @@ | |||
action :create_if_missing | |||
end | |||
|
|||
node.default['brightbox-ruby']['version'] = "2.3" | |||
node.default['brightbox-ruby']['version'] = "2.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to "DRY" this up like this?
node.default['brightbox-ruby']['version'] = RUBY_VERSION
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking if I understand: currently "2.4" isn't used anywhere else, but you're suggesting pulling "2.4" out into a variable to make the code a little more self documenting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's an unusual usage of "DRY", and I see how it's confusing.
To me DRYness is this: When you change something, do you have to make that change in more than one place?
The ruby version isn't mentioned elsewhere in the code, but one way or another we have chosen what Ruby version we're using, and I assume we want the same version here.
.travis.yml
Outdated
@@ -3,12 +3,13 @@ cache: | |||
directories: | |||
- WcaOnRails/vendor/bundle | |||
rvm: | |||
- 2.3.1 | |||
- 2.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just learned that you can have a .ruby_version
file in your project that both rbenv and rvm will obey.
I think we should have that in this project, since it would help people run the right ruby version.
I also suspect/hope that the rvm directive here could then be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting! After a little research, it looks like .ruby_version
is just a RVM thing, which would not help out people who do not use RVM. It looks like Gemfile allows you to specify a ruby version, however, and RVM supports that instead of a .ruby_version
as a way to specify the version of ruby your project uses (see https://rvm.io/workflow/projects#project-file-gemfile).
I'll try that out and see if travis picks it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, adding a ruby "2.4.0"
directive to the Gemfile didn't work for travis:
Warning: the running version of Bundler (1.13.6) is older than the version that created the lockfile (1.14.6). We suggest you upgrade to the latest version of Bundler by running `gem install bundler`.
Your Ruby version is 2.2.5, but your Gemfile specified 2.4.0
The command "eval bundle install --jobs=3 --retry=3 --deployment " failed. Retrying, 2 of 3.
I'll try out a .ruby-version
file now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis seems to also ignore the .ruby-version
file. See here.
For now, I've gone back to just specifying the ruby version in .travis.yml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know .ruby_version
also works in rbenv
, because I'm now using it. 'rbenv' and 'RVM' should cover most developers.
For the rest we could have a test asserting that RUBY_VERSION
is equal to the contents of .ruby_version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, clever idea to keep the 2 in sync!
c828e40
to
4a4ad0c
Compare
72d9311
to
51ad3d5
Compare
This requires upgrading to Rails 5.0.2 for full ruby 2.4 compatibility. This fixes thewca#1114.
This fixes #1114.
This is actually possible to do now that we're in Rails 5! However, I'm still seeing some warnings related to this upgrade, so I'm going to wait a bit for the next version of Rails 5, hopefully with rails/rails#27458.EDIT: After upgrading to Rails 5.0.2, these warnings seemed to have gone away =)