Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from May 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ecd55e7
Fixes #29110 - updated fact parser to handle v3 debian OS
lzap Mar 3, 2020
cffc41e
Refs #29110 - append minor version to debian description
lzap Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/operatingsystem.rb
View file
Open in desktop
Expand Up @@ -261,7 +261,7 @@ def display_family
"Unknown"
end

def self.shorten_description(description)
def shorten_description(description)
# This method should be overridden in the OS subclass
# to handle shortening the specific formats of lsbdistdescription
# returned by Facter on that OS
Expand Down
3 changes: 2 additions & 1 deletion app/models/operatingsystems/debian.rb
View file
Open in desktop
Expand Up @@ -43,13 +43,14 @@ def display_family
"Debian"
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

s.presence || description
end

Expand Down
2 changes: 1 addition & 1 deletion app/models/operatingsystems/redhat.rb
View file
Open in desktop
Expand Up @@ -37,7 +37,7 @@ def display_family
"Red Hat"
end

def self.shorten_description(description)
def shorten_description(description)
return "" if description.blank?
s = description.dup
s.gsub!('Red Hat Enterprise Linux', 'RHEL')
Expand Down
2 changes: 1 addition & 1 deletion app/models/operatingsystems/suse.rb
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ def display_family
"SUSE"
end

def self.shorten_description(description)
def shorten_description(description)
return "" if description.blank?
s = description.dup
s.gsub!('SUSE Linux Enterprise Server', 'SLES')
Expand Down
15 changes: 8 additions & 7 deletions app/services/puppet_fact_parser.rb
View file
Open in desktop
Expand Up @@ -11,8 +11,8 @@ def operatingsystem
args = {:name => os_name, :major => major, :minor => minor}
os = Operatingsystem.find_or_initialize_by(args)
if os_name[/debian|ubuntu/i] || os.family == 'Debian'
if facts[:lsbdistcodename]
os.release_name = facts[:lsbdistcodename]
if facts.dig(:os, :distro, :codename).presence || facts[:lsbdistcodename]
os.release_name = facts.dig(:os, :distro, :codename).presence || facts[:lsbdistcodename]
ekohl marked this conversation as resolved.
Show resolved Hide resolved
elsif os.release_name.blank?
os.release_name = 'unknown'
end
Expand All @@ -24,9 +24,10 @@ def operatingsystem
if os.description.blank?
if os_name == 'SLES'
os.description = os_name + ' ' + orel.gsub('.', ' SP')
elsif facts[:lsbdistdescription]
elsif facts.dig(:os, :distro, :description).presence || facts[:lsbdistdescription]
family = os.deduce_family || 'Operatingsystem'
os.description = family.constantize.shorten_description facts[:lsbdistdescription]
os = os.becomes(family.constantize)
os.description = os.shorten_description(facts.dig(:os, :distro, :description).presence || facts[:lsbdistdescription])
end
end

Expand Down Expand Up @@ -185,7 +186,7 @@ def use_legacy_facts?
end

def os_name
os_name = facts[:operatingsystem].presence || raise(::Foreman::Exception.new("invalid facts, missing operating system value"))
os_name = facts.dig(:os, :name).presence || facts[:operatingsystem].presence || raise(::Foreman::Exception.new("invalid facts, missing operating system value"))

if os_name == 'RedHat' && facts[:lsbdistid] == 'RedHatEnterpriseWorkstation'
os_name += '_Workstation'
Expand Down Expand Up @@ -218,9 +219,9 @@ def os_release
'1.0'
when /Debian/i
return "99" if facts[:lsbdistcodename] =~ /sid/
facts[:lsbdistrelease] || facts[:operatingsystemrelease]
facts.dig(:os, :release, :full) || facts[:lsbdistrelease] || facts[:operatingsystemrelease]
else
facts[:lsbdistrelease] || facts[:operatingsystemrelease]
facts.dig(:os, :release, :full) || facts[:lsbdistrelease] || facts[:operatingsystemrelease]
end
end
end
14 changes: 7 additions & 7 deletions test/models/operatingsystem_test.rb
View file
Open in desktop
Expand Up @@ -168,31 +168,31 @@ class OperatingsystemTest < ActiveSupport::TestCase

describe "descriptions" do
test "Redhat LSB description should be correctly shortened" do
assert_equal 'RHEL 6.4', Redhat.shorten_description("Red Hat Enterprise Linux release 6.4 (Santiago)")
assert_equal 'RHEL 6.4', Redhat.new.shorten_description("Red Hat Enterprise Linux release 6.4 (Santiago)")
end

test "Fedora LSB description should be correctly shortened" do
assert_equal 'Fedora 19', Redhat.shorten_description("Fedora release 19 (Schrodinger's Cat)")
assert_equal 'Fedora 19', Redhat.new.shorten_description("Fedora release 19 (Schrodinger's Cat)")
end

test "Debian LSB description should be correctly shortened" do
assert_equal 'Debian 7.1', Debian.shorten_description("Debian GNU/Linux 7.1 (wheezy)")
assert_equal 'Debian 7.1', Debian.new.shorten_description("Debian GNU/Linux 7.1 (wheezy)")
end

test "Ubuntu LSB is unaltered" do
assert_equal 'Ubuntu 12.04.3 LTS', Debian.shorten_description("Ubuntu 12.04.3 LTS")
assert_equal 'Ubuntu 12.04.3 LTS', Debian.new.shorten_description("Ubuntu 12.04.3 LTS")
end

test "SLES LSB description should be correctly shortened" do
assert_equal 'SLES 11', Suse.shorten_description("SUSE Linux Enterprise Server 11 (x86_64)")
assert_equal 'SLES 11', Suse.new.shorten_description("SUSE Linux Enterprise Server 11 (x86_64)")
end

test "openSUSE LSB description should be correctly shortened" do
assert_equal 'openSUSE 11.4', Suse.shorten_description("openSUSE 11.4 (x86_64)")
assert_equal 'openSUSE 11.4', Suse.new.shorten_description("openSUSE 11.4 (x86_64)")
end

test "OSes without a shorten_description method fall back to description" do
assert_equal 'Arch Linux', Archlinux.shorten_description("Arch Linux")
assert_equal 'Arch Linux', Archlinux.new.shorten_description("Arch Linux")
end
end

Expand Down