Fix failures under Ruby 2.7 #1412

KrisShannon · 2019-11-22T01:27:40Z

Ruby 2.7 removes the scanf library which is used in the network plugins for darwin and solaris to parse the hex netmask into a dotted decimal

Replaced the scanf library with simple regex and to_i(16) calls

Related Issue

Pull #1410 added ruby 2.7 to the testing matrix which is now causing failures

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

KrisShannon · 2019-11-22T01:29:02Z

I'm not sure about the code duplication for parsing the hex netmasks.

Should this be extracted and if so where should I put it?

jaymzh · 2019-11-22T05:51:28Z

I would probably rename mixin/network_constants.rb to mixin/network_helper.rb (ala http_helper), rename the module accordingly, and then add your method in there. Seem sane @tas50

tas50 · 2019-11-22T05:59:26Z

lib/ohai/plugins/darwin/network.rb

@@ -50,6 +50,14 @@ def parse_media(media_string)
    media
  end

+  def darwin_parse_netmask(netmask)


Can you toss a YARD comment on these explaining what the arg and return is for anyone that needs to touch it in the future. Also thanks for keeping the names unique so we avoid collisions in the future.

@tas50 I"m pretty sure there these are the same, and thus should not have different names and should not be in different places, no?

tas50 · 2019-11-22T06:00:27Z

it would probably be nice to extract this. Short of that the unique names avoid the problems we had with def collisions in the past.

KrisShannon · 2019-11-22T06:06:48Z

I like the idea of moving it into a renamed mixin/network_helper.rb

I'll give that a go.

KrisShannon · 2019-11-22T06:35:23Z

I found another existing method hex_to_dec_netmask in plugins/aix/network.rb

I'll extract and patch that instead.

jaymzh · 2019-11-22T06:42:30Z

lib/ohai/plugins/darwin/network.rb

@@ -50,6 +50,14 @@ def parse_media(media_string)
    media
  end

+  def darwin_parse_netmask(netmask)
+    if netmask =~ /^([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/
+      return [$1, $2, $3, $4].map { |hex| hex.to_i(16) } * "."


btw. join is a much clearer than array-repetition-bizarre-side-use-if-it-isnt-a-number syntax :)

Yeah, I agree.

The scanf line was using ... * "." and I just didn't change it when I was editing to remove the scanf.

The new helper from AIX is completely different so ¯_(ツ)_/¯

jaymzh

Oh the unittests for hext_to_dec_netmask are still over in the aix plugin specs, you need to move that...

KrisShannon · 2019-11-22T07:04:33Z

What should I do about the test of the hex_to_dec_netmask helper in spec/unit/plugins/aix/network_spec.rb?

It's no longer specific to AIX so should I create a new spec/unit/mixin/network_helper.rb and move the test across to it?

This is going to be extracted into a helper for use on other platforms Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

This is in preparation for adding a helper method to the mixin for converting ipv4 hex strings to dotted decimal Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

KrisShannon · 2019-11-22T07:29:18Z

I created a spec/unit/mixin/network_helper.rb and moved the aix plugin test across to it but it doesn't look like it got executed by rspec on buildkite.

Is there something else that needs to be updated so it will run the new spec file? I tried grepping around for the command_spec and the azure_metadata_spec which are bring run but I couldn't find anything.

I did notice that spec/unit/mixin/dmi_decode.rb also doesn't seem to be running.

jaymzh · 2019-11-22T09:54:32Z

@KrisShannon - I believe the reason those aren't working is because they don't end in _spec.rb

KrisShannon · 2019-11-22T09:57:43Z

Doh! Of course...

jaymzh · 2019-11-22T10:06:33Z

Don't feel bad, at least you noticed yours wasn't running. Whoever wrote dmi_decode.rb never noticed it didn't run. :) We all miss stuff. :)

Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

The 'scanf' library is gone in Ruby 2.7 A helper for parsing hex netmasks was already written for aix so use it instead. Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

KrisShannon · 2019-11-22T10:46:33Z

Ok. Final push I hope.

Test file is renamed and buildkite has run it succesfully

jaymzh

Looks great to me!

Someone should probably file a separate PR to fix that dmi_decode spec you noticed. :)

@tas50 - shall we merge this and then mine?

jaymzh · 2019-11-22T18:26:03Z

Oh, @KrisShannon you already opened a PR for that too! Sorry I missed that. Hadn't had coffee yet. :) <3 Thanks!

tas50 · 2019-11-23T03:39:25Z

Thanks @KrisShannon. You saved us some work next April when we ship with ruby 2.7

jaymzh · 2019-11-23T22:53:28Z

@tas50 - now that you merged this you can merge #1267 :)

tas50 reviewed Nov 22, 2019

View reviewed changes

jaymzh reviewed Nov 22, 2019

View reviewed changes

jaymzh approved these changes Nov 22, 2019

View reviewed changes

jaymzh requested changes Nov 22, 2019

View reviewed changes

KrisShannon added 2 commits November 22, 2019 18:15

Remove the 0x prefix from the argument to hex_to_dec_netmask

cb9f1fa

This is going to be extracted into a helper for use on other platforms Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

Rename Ohai::Mixin::NetworkConstants to NetworkHelper

9649037

This is in preparation for adding a helper method to the mixin for converting ipv4 hex strings to dotted decimal Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

KrisShannon added 2 commits November 22, 2019 21:39

Extract hex_to_dec_netmask into mixin/network_helper.rb

ae3930b

Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

Use network helper to parse netmask instead of 'scanf'

6c6de05

The 'scanf' library is gone in Ruby 2.7 A helper for parsing hex netmasks was already written for aix so use it instead. Signed-off-by: Kris Shannon <k.shannon@amaze.com.au>

KrisShannon mentioned this pull request Nov 22, 2019

Rename Ohai::Mixin::DmiDecode spec file to end in _spec.rb #1413

Merged

10 tasks

jaymzh approved these changes Nov 22, 2019

View reviewed changes

tas50 merged commit 48c85ea into chef:master Nov 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failures under Ruby 2.7 #1412

Fix failures under Ruby 2.7 #1412

KrisShannon commented Nov 22, 2019 •

edited

Loading

KrisShannon commented Nov 22, 2019

jaymzh commented Nov 22, 2019

tas50 Nov 22, 2019

jaymzh Nov 22, 2019

tas50 commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh Nov 22, 2019

KrisShannon Nov 22, 2019

jaymzh left a comment

KrisShannon commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh left a comment

jaymzh commented Nov 22, 2019

tas50 commented Nov 23, 2019

jaymzh commented Nov 23, 2019

Fix failures under Ruby 2.7 #1412

Fix failures under Ruby 2.7 #1412

Conversation

KrisShannon commented Nov 22, 2019 • edited Loading

Related Issue

Types of changes

Checklist:

KrisShannon commented Nov 22, 2019

jaymzh commented Nov 22, 2019

tas50 Nov 22, 2019

Choose a reason for hiding this comment

jaymzh Nov 22, 2019

Choose a reason for hiding this comment

tas50 commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh Nov 22, 2019

Choose a reason for hiding this comment

KrisShannon Nov 22, 2019

Choose a reason for hiding this comment

jaymzh left a comment

Choose a reason for hiding this comment

KrisShannon commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh commented Nov 22, 2019

KrisShannon commented Nov 22, 2019

jaymzh left a comment

Choose a reason for hiding this comment

jaymzh commented Nov 22, 2019

tas50 commented Nov 23, 2019

jaymzh commented Nov 23, 2019

KrisShannon commented Nov 22, 2019 •

edited

Loading