-
Notifications
You must be signed in to change notification settings - Fork 897
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
Updates to work with Ruby 2.4.0 #13104
Conversation
wow, so many warnings... for future me, these were the test failures:
|
@miq-bot add_label performance |
@Fryguy I updated manageiq-gems-pending with ManageIQ/manageiq-gems-pending#47 and tried adding 2.4, but there were many failures and deprecations, so I removed that from the PR temporarily |
b144ead
to
0b6f7e0
Compare
class StubSocket | ||
def close | ||
end | ||
end |
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.
We can either upgrade webmock or do this for now. I tried upgrading it locally and it looks at the very least that we'd need to re-record our vcr cassettes. I'm not sure if that's all, so I went the easy route here in hopes we can upgrade in a followup PR.
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.
@@ -34,7 +34,7 @@ | |||
|
|||
expect(FileUtils).to receive(:mkdir_p).with("/repo/mirror") | |||
expect(MiqApache::Conf).to receive(:create_conf_file).once.and_return(true) | |||
expect(FileUtils).to receive(:rm).with("/etc/httpd/conf.d/manageiq-https-mirror.conf", :force => true) | |||
expect(FileUtils).to receive(:rm).with("/etc/httpd/conf.d/manageiq-https-mirror.conf", hash_including(:force => true)) |
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.
See commit, it's basically changed from an options hash to kwargs so the expectations on arguments is different now.
spec/models/custom_attribute_spec.rb
Outdated
|
||
it "returns the value type of Fixnum custom attributes" do | ||
expect(int_custom_attribute.value_type).to eq(:fixnum) | ||
end |
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.
See the commit regarding custom attributes, we should really find out if we're actually using value_type
because it's really brittle to use class names instead of using interfaces such as expecting fixnum/bignum/integer types to respond to comparison operators. CC @alongoldboim (see the commit for more details)
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 was pretty sure that we use integer for these
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.
cc @alongoldboim can you review this commit? It's not clear why we're using hardcoded ruby class names here. Do you know if it's needed?
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.
cc @zgalor do you know if/how CustomAttribute#value_type
is being used?
It's defined as serialized_value.class.to_s.downcase.to_sym
:-(
I doubt removing the spec is enough. We likely want either backward compat (return :fixnum on 2.4), or forward compat (switch to :integer on <=2.3), or remove the functionality.
API is only accepting (on POST?) a whitelist for field_type: https://github.com/ManageIQ/manageiq/search?utf8=%E2%9C%93&q=ALLOWED_API_VALUE_TYPES
https://github.com/ManageIQ/manageiq/blob/caa745c49/app/controllers/api/providers_controller.rb#L90
Not sure if/how field_type
interacts with value_type
and serialized_value
. Do we expose value_type on GET?
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.
Sorry, I was lazy, I didn't put the commit details (which keeps changing the commit sha1 when I rebase/force push), so I'll put it here. Note, I totally agree with @cben here, the test removal is probably not enough. We need to remove usage that cared if it was Fixnum vs Bignum:
CustomAttribute#value_type returns a symbol for a ruby core class, a
hardcoded value instead of the class's behavior.
Do we use this? Why are we returning a value instead of caring about
behavior of the old Fixnum and Integer? If we don't use this, we
should remove the whole method and it's tests added in:
4687394
The PR that added this was:
#11000
Adding CustomAttribute to provider UI/API was added in:
#10897
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.
spec/models/custom_attribute_spec.rb
Outdated
|
||
it "returns the value type of Fixnum custom attributes" do | ||
expect(int_custom_attribute.value_type).to eq(:fixnum) | ||
end |
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 was pretty sure that we use integer for these
@@ -327,7 +327,9 @@ | |||
:parent => :configuration_manager | |||
|
|||
trait(:provider) do | |||
after(:build, &:create_provider) | |||
after(:build) do |x| |
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.
huh. any reasoning behind this one?
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.
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.
Updated again due to rebase e0825a0ff7078641680b1a42a82db8e0875f6dd0
Could you double check [travis]? it seems quite vocal |
ugh, is https://rubygems.org/gems/ezcrypto abandoned? Don't know if https://github.com/pelle/ezcrypto is the new owner - 8 hours old |
Yeah, I don't know @kbrock, I don't know if we can just swap our usage of ezcrypto out for something else at this point. I wish we could. |
oops But yes |
We may want to give them a PR with a |
This pull request is not mergeable. Please rebase and repush. |
@Fryguy @chessbyte I moved the checklist to #14446 |
Seems wise and inexpensive to require beforehand.
Why miq-bot to 2.3 instead of 2.4?
🎉 |
This essentially allows a syntax check against Ruby 2.3, so we can keep Ruby 2.3 as a minimum, without forcing us to run tests for both 2.3 and 2.4 in travis (which is expensive). |
Copied from: ManageIQ/manageiq#13104 Ruby 2.4.0 removed the closed? check in the conditional in: s.close if !s.closed? Webmock was changed to add close to StubSocket along with another change.. ruby/ruby@f845a9e bblimke/webmock@8f2176a WebMock 2.3.1+ fixed the issue with ruby 2.4.0 by adding StubSocket#close.
You know, the parser is the only real change. |
we can even monkey patch ruby parser or use master branch of ruby-parser (see the links in the description) |
@Fryguy @bdunne I'd like to get this in since the providers checkout miq to run their tests. Now that fine is branched, is there any risk to merging this on master? If the ripper ruby parser change is scary, we can use the 3.9.0 version of ruby parser which seems to now support ruby 2.4 and future versions. https://github.com/seattlerb/ruby_parser/blob/master/History.rdoc |
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
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.
This is the only thing I noticed that hasn't been addressed, but even then, it isn't a blocker and I don't have a suggestion of my own. Looks good to me.
.try(:instance_variable_get, '@holder') | ||
.try(:instance_variable_get, '@keeper') | ||
if RUBY_VERSION > "2.3.1" | ||
warn "Remove me: #{__FILE__}:#{__LINE__} and my test. Ruby 2.3.2+ should not be creating timer threads." |
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.
Because you are removing this warn
, does that mean you have given up on ever removing this hack? Or do you have some other method in mind for remembering to remove this when ruby 2.3 is no longer supported?
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.
A spec that fails when the ruby version is higher than some version? (e.g.: 2.5)
(that way assuming that we stopped supporting 2.3)?
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.
Interesting @NickLaMuro @kbrock, especially the 2.5 test. The problem is we usually support two ruby versions on master, so yeah, we can't remove this hack completely until ruby 2.3.0 and below are unsupported.
We can as of bundler 1.12+, add
ruby "~> 2.3", "< 2.4.1"
to the Gemfile, but we can't programmatically access this to check if the current ruby is satisfied.
It would be nice to add a warn if Bundler.load.required_ruby.satisfied_by?(RUBY_VERSION)
was a thing but the ruby requirement isn't exposed in the bundler runtime... See rubygems/bundler@b4a52c3. Maybe I'm missing something but I couldn't access it.
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 maybe some kind of .travis.yml
parser that checks the supported ruby versions be a good way to handle this? Might be over-engineering things a bit, but something to consider so we aren't forgetting to remove this in the future. This of course assumes that the .travis.yml
is consistent here and in downstream as a source of truth for supported ruby versions.
Otherwise I am at a loss on how we could do this.
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.
In an ideal world, we would open a PR now to remove it, which would fail tests now but start passing as soon as .travis.yml tests only >2.3.1.
But something would have to periodically re-run the tests and notify someone once they start passing. Or directly "merge when green", measured in years
"An ideal world is left as an exercise to the reader" — Paul Graham, On Lisp
Silly question:
We're bumping to requiring ≥2.3 now that we'll support 2.4, no?
Can't we simply proclaim we support ≥2.3.2 and drop the hack?
This pull request is not mergeable. Please rebase and repush. |
RubyParser lags Ruby and doesn't yet have Ruby 2.4 support. RipperRubyParser uses Ripper and so will always have the latest version.
ruby/ruby@baa43cd Fixes: expected: ("/etc/httpd/conf.d/manageiq-https-mirror.conf", {:force=>true}) got: ("/etc/httpd/conf.d/manageiq-https-mirror.conf", {:force=>true, :noop=>nil, :verbose=>nil})
Ruby 2.4.0 removed the closed? check in the conditional in: s.close if !s.closed? Webmock was changed to add close to StubSocket along with another change.. ruby/ruby@f845a9e bblimke/webmock@8f2176a WebMock 2.3.1+ fixed the issue with ruby 2.4.0 by adding StubSocket#close.
For now, make the test expectations different for ruby 2.4.0+ vs. prior rubies. CustomAttribute#value_type returns a symbol for a ruby core class, a hardcoded value instead of the class's behavior. Do we use this? Why are we returning a value instead of caring about behavior of the old Fixnum and Integer? If we don't use this, we should remove the whole method and it's tests added in: ManageIQ@4687394 The PR that added this was: ManageIQ#11000 Adding CustomAttribute to provider UI/API was added in: ManageIQ#10897
Checked commits Fryguy/manageiq@90c1d06~...56f5fc9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
https://bugs.ruby-lang.org/issues/12342 has been fixed in 2.3.2+ and 2.4.0+ so we should only do this HACK for 2.3.1 and lower. Extracted from ManageIQ/manageiq#13104
Even though this is my PR, none of the code left is mine anymore 😆 , so I'm going to merge. |
This is one step to ruby 2.4, here is an outline of the steps: #14446
Known issues:
RubyParser didn't support Ruby 2.4 immediately, and now does in 3.9.0 and here, with changelog here. RubyParser tends to lag, so instead I found RipperRubyParser which is a "drop-in replacement for RubyParser using Ripper". Since Ripper comes with Ruby it will always be up-to-date.
Expand symbol to proc in a factory due to a ruby 2.4.0 bug …https://bugs.ruby-lang.org/issues/13074
via
upgrading from 4.7.0 to 4.8.0 breaks symbol to proc callbacks thoughtbot/factory_bot#980
"When executing instance_exec with symbol.to_proc, it ignores firstargument"
Ruby 2.4 unifies Fixnum and Bignum into Integer, the former classes are deprecated
Monkey patch webmock for now, may need to re-record cassettes with new webmock supporting ruby 2.4: Added Update to webmock 2.3.1+ and VCR 3.0.2+ #14270 to upgrade webmock and remove this temporary monkey patch.
NOTE: We are not changing the required ruby version in the
Gemfile
nor the one tested by travis in the.travis.yml
in this PR. These changes will work with ruby 2.4 and 2.3 and probably even 2.2.cc @jrafanie