Fixes #29110 - updated fact parser to handle v3 debian OS #7482

lzap · 2020-03-03T09:21:30Z

Added example JSON output from Facter v3 (VMWare, Debian 10.3) and
updated fact parser to handle LSB information from v3 "os" tree.

As part of this patch, I am adding stubbing of DNS calls as I noticed it
slows down fact tests 10x. Chances are there are other places in our
huge test codebase.

theforeman-bot · 2020-03-03T09:21:48Z

Issues: #29110

app/services/puppet_fact_parser.rb

ekohl · 2020-03-03T09:25:09Z

app/services/puppet_fact_parser.rb

        family = os.deduce_family || 'Operatingsystem'
-        os.description = family.constantize.shorten_description facts[:lsbdistdescription]
+        os.description = family.constantize.shorten_description(facts.dig(:os, :distro, :description).presence || facts[:lsbdistdescription])


Same here:

Suggested change

os.description = family.constantize.shorten_description(facts.dig(:os, :distro, :description).presence || facts[:lsbdistdescription])

os.description = family.constantize.shorten_description(facts.dig(:os, :distro, :description) || facts[:lsbdistdescription])

ekohl · 2020-03-03T09:29:32Z

test/test_helper.rb

@@ -89,9 +89,20 @@ def after_teardown
  end
 end

+module TestCaseNetworkStubExtensions
+  def setup


What about Resolv.getaddress and Resolv.getname? Or are they covered because they just instantiate a Resolv::DNS instance?

Yep, that's how I understand the code however I haven't tested. Let me check.

ekohl · 2020-03-03T09:31:55Z

test/test_helper.rb

+  def setup
+    Resolv::DNS.any_instance.stubs(:getname).returns("stubbed-host.example.com")
+    Resolv::DNS.any_instance.stubs(:getnames).returns(["stubbed-host.example.com"])
+    Resolv::DNS.any_instance.stubs(:getaddress).returns("127.0.0.13")


As said on IRC, I think we should raise an exception and don't pretend it exists:

[4] pry(main)> Resolv.getaddress('doesnotexist.example.com') Resolv::ResolvError: no address for doesnotexist.example.com from /usr/share/ruby/resolv.rb:94:in `getaddress'

ekohl · 2020-03-03T09:32:29Z

test/test_helper.rb

+    Resolv::DNS.any_instance.stubs(:getname).returns("stubbed-host.example.com")
+    Resolv::DNS.any_instance.stubs(:getnames).returns(["stubbed-host.example.com"])
+    Resolv::DNS.any_instance.stubs(:getaddress).returns("127.0.0.13")
+    Resolv::DNS.any_instance.stubs(:getaddresses).returns(["127.0.0.13"])


We shouldn't pretend it exists:

[6] pry(main)> Resolv.getaddresses('doesnotexist.example.com') => []

Suggested change

Resolv::DNS.any_instance.stubs(:getaddresses).returns(["127.0.0.13"])

Resolv::DNS.any_instance.stubs(:getaddresses).returns([])

Ok ok, I will do exceptions. I will do this in a separate PR tho, I want this one to be backported.

lzap · 2020-03-03T09:45:57Z

Rebased, DNS stubs are out!

lzap · 2020-03-05T12:28:02Z

Failure irrelevant? HostJSTest::hosts index multiple actions.test_0001_show action buttons

lzap · 2020-03-05T13:39:17Z

A user on the discourse reports it worked for them.

ekohl · 2020-03-11T11:40:36Z

app/models/operatingsystems/debian.rb

    return "" if description.blank?
    s = description.dup
    s.gsub!('GNU/Linux', '')
    s.gsub!(/\(.+?\)/, '')
    s.squeeze! " "
    s.strip!
+    s += '.' + minor unless s.include?('.')


Right now it's hard to see what the benefit is. I assume you want to change Debian GNU/Linux 10 (buster) into Debian 10.3 but I'm wondering if that's needed. Debian has dropped the use of minor versions since a few releases. Is there a reason we want to divert from the OS? Does the description column need to be unique?

Could you also add a test case for this?

Description was previously "Debian 10", with the change it's gonna be "Debian 10.3".

Uniquness is a good question, I'd say yes we want it to be unique because that's the field we show in lists. You do not want to see this:

Debian 10

Debian 10

Debian 10

Debian 10

I vaguely remember that this field is preferred because users want to define their own naming convention e.g. RHEL 7.7 instead RedHat 7.7. So generally speaking, i'd rather keep them unique and do this patch than doing a big change on that.

We actually follow the same naming scheme for Red Hat systems and it makes sense to me. Every minor release of RHEL (I assume Debian too) has a installable tree (kickstart). To be able to install from different version trees, you actually need two OS entries otherwise you'd need to change minor version before every provisioning.

I will add a test case for this of couse.

@ekohl wait there is a test already, can you elaborate what you want me to do?

https://github.com/theforeman/foreman/pull/7482/files#diff-f6c009374a71e00e01084f53f35f47ba

ekohl

I was wondering whether we should even store the minor for Debian since version 7 since it's more like a patch level. However, I think it may also influence boot media so from a fact handling perspective I think this is correct.

I'm having some doubts about making it a non-static method. Should we keep it static and pass the minor as an optional parameter. On the other hand, in many languages using static methods with class inheritance doesn't even work. The method is only used in the fact importer so the impact is minimal. Given my uncertainty on this area, I'd like someone more familiar with Ruby (on Rails) to take a look as well. @tbrisker perhaps?

For what it's worth, if I'd design this today, I'd move the shorten_description into the PuppetFactParser but that'd make this particular PR too big.

lzap · 2020-03-23T09:00:42Z

Should we keep it static and pass the minor as an optional parameter.

Passing attributes of an instance into method that is defined as static on the class is almost always a bad idea, no matter the language. What we had (static methods inheritance) is something that can only be used with some languages and IMHO is also generally a bad idea. This patch fixes it, I believe.

I agree this would need an overhaul. If I had to pick, I'd rather work on the way we store reports, then facts :)

ekohl · 2020-03-23T16:58:26Z

@tbrisker mind having a look?

tbrisker · 2020-03-30T10:00:54Z

I don't have a strong preference here.
if we're passing a parameter to the method and not using any instance variables in it, does it actually matter if it's defined on the class or the instance? It would make sense to make it an instance method if it manipulated the description attribute for example, but as it is - it doesn't matter much to me where it's defined.
Regarding the naming scheme for debian I have no idea - i guess that depends on what is expected for the installation media.

tbrisker

My only thought regarding maybe keeping it as class method is that this is something we want to backport to stable releases, and the smaller the change the easier that will be.

lzap · 2020-04-02T12:27:00Z

@tbrisker @ekohl please give me clear statement if I should be refactoring this back to class methods or not. I believe class method inheritance is bad practice, particularly when it requires a parameter that is from the instance of the class. Data should be encapsulated.

lzap · 2020-04-02T12:28:32Z

Rebased conflicts.

tbrisker · 2020-04-02T12:41:38Z

@lzap ah, now i see you actually use an instance parameter in the debian one. I guess then it does make sense to make this into an instance method, although it is still a bit odd to me that it gets the description as a parameter and returns a new description rather than manipulating the instance's attributes directly. Still no idea regarding what makes sense from the debian install media pov

lzap · 2020-04-24T11:16:28Z

It's been long, so I am rebasing.

tuxmea · 2020-04-29T07:59:13Z

@tbrisker @ekohl Any update on when this will be reviewed and merged?

ekohl · 2020-04-29T08:46:20Z

I've been assuming that @tbrisker would continue his review since he had comments.

tbrisker · 2020-04-30T08:13:03Z

My only comment was that I don't know what makes sense regarding the install media path for debian, other than that i'm fine with this change.

mmoll

Seems to work just fine. The mediumpath doesn't contain the version, but the installer uses the distro code name.

tbrisker

Thanks @lzap !

tbrisker · 2020-05-03T07:30:04Z

Please open CP PRs against stable branches, this does not cherry-pick cleanly.

…#7482) (cherry picked from commit 5d61c18)

lzap · 2020-05-04T14:07:18Z

Done, they are associated to the ticket. Thanks!

…#7482) (cherry picked from commit 5d61c18)

(cherry picked from commit 5d61c18)

theforeman-bot added Needs testing Not yet reviewed labels Mar 3, 2020

ekohl reviewed Mar 3, 2020

View reviewed changes

lzap force-pushed the debian-fact-parsing-v3-29110 branch from 0d3652c to 246fe36 Compare March 3, 2020 09:43

lzap mentioned this pull request Mar 5, 2020

Document behavior for various facts theforeman/foreman-documentation#124

Open

ekohl reviewed Mar 11, 2020

View reviewed changes

ekohl previously approved these changes Mar 20, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Mar 20, 2020

tbrisker reviewed Apr 1, 2020

View reviewed changes

lzap dismissed ekohl’s stale review via 1d9beab April 2, 2020 12:28

lzap force-pushed the debian-fact-parsing-v3-29110 branch from 99a9198 to 1d9beab Compare April 2, 2020 12:28

tbrisker self-assigned this Apr 22, 2020

lzap force-pushed the debian-fact-parsing-v3-29110 branch from 1d9beab to 51db133 Compare April 24, 2020 11:16

lzap added 2 commits May 2, 2020 19:49

Fixes #29110 - updated fact parser to handle v3 debian OS

ecd55e7

Refs #29110 - append minor version to debian description

cffc41e

mmoll force-pushed the debian-fact-parsing-v3-29110 branch from 51db133 to cffc41e Compare May 2, 2020 17:49

mmoll approved these changes May 2, 2020

View reviewed changes

tbrisker approved these changes May 3, 2020

View reviewed changes

tbrisker merged commit 5d61c18 into theforeman:develop May 3, 2020

lzap deleted the debian-fact-parsing-v3-29110 branch May 4, 2020 13:45

lzap added a commit to lzap/foreman that referenced this pull request May 4, 2020

Fixes #29110 - updated fact parser to handle v3 debian OS (theforeman…

295bf0a

…#7482) (cherry picked from commit 5d61c18)

lzap added a commit to lzap/foreman that referenced this pull request May 4, 2020

Fixes #29110 - updated fact parser to handle v3 debian OS (theforeman…

945ba5e

…#7482) (cherry picked from commit 5d61c18)

mmoll pushed a commit to lzap/foreman that referenced this pull request May 5, 2020

Fixes #29110 - updated fact parser to handle v3 debian OS (theforeman…

06350d1

…#7482) (cherry picked from commit 5d61c18)

tbrisker pushed a commit that referenced this pull request May 6, 2020

Fixes #29110 - updated fact parser to handle v3 debian OS (#7482)

e96fb64

(cherry picked from commit 5d61c18)

tbrisker pushed a commit that referenced this pull request May 6, 2020

Fixes #29110 - updated fact parser to handle v3 debian OS (#7482)

41fa058

(cherry picked from commit 5d61c18)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #29110 - updated fact parser to handle v3 debian OS #7482

Fixes #29110 - updated fact parser to handle v3 debian OS #7482

lzap commented Mar 3, 2020

theforeman-bot commented Mar 3, 2020

ekohl Mar 3, 2020

ekohl Mar 3, 2020

lzap Mar 3, 2020

ekohl Mar 3, 2020

ekohl Mar 3, 2020

lzap Mar 3, 2020

lzap commented Mar 3, 2020

lzap commented Mar 5, 2020

lzap commented Mar 5, 2020

ekohl Mar 11, 2020

lzap Mar 17, 2020

lzap Mar 18, 2020

ekohl left a comment

lzap commented Mar 23, 2020

ekohl commented Mar 23, 2020

tbrisker commented Mar 30, 2020

tbrisker left a comment

lzap commented Apr 2, 2020

lzap commented Apr 2, 2020

tbrisker commented Apr 2, 2020

lzap commented Apr 24, 2020

tuxmea commented Apr 29, 2020

ekohl commented Apr 29, 2020

tbrisker commented Apr 30, 2020

mmoll left a comment

tbrisker left a comment

tbrisker commented May 3, 2020

lzap commented May 4, 2020

	os.description = family.constantize.shorten_description(facts.dig(:os, :distro, :description).presence \|\| facts[:lsbdistdescription])
	os.description = family.constantize.shorten_description(facts.dig(:os, :distro, :description) \|\| facts[:lsbdistdescription])

	Resolv::DNS.any_instance.stubs(:getaddresses).returns(["127.0.0.13"])
	Resolv::DNS.any_instance.stubs(:getaddresses).returns([])

Fixes #29110 - updated fact parser to handle v3 debian OS #7482

Fixes #29110 - updated fact parser to handle v3 debian OS #7482

Conversation

lzap commented Mar 3, 2020

theforeman-bot commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Mar 3, 2020

lzap commented Mar 5, 2020

lzap commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

lzap commented Mar 23, 2020

ekohl commented Mar 23, 2020

tbrisker commented Mar 30, 2020

tbrisker left a comment

Choose a reason for hiding this comment

lzap commented Apr 2, 2020

lzap commented Apr 2, 2020

tbrisker commented Apr 2, 2020

lzap commented Apr 24, 2020

tuxmea commented Apr 29, 2020

ekohl commented Apr 29, 2020

tbrisker commented Apr 30, 2020

mmoll left a comment

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment

tbrisker commented May 3, 2020

lzap commented May 4, 2020