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.

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

Fixes #36547 - Fix parsing of Ubuntu version in fact parsers #9760

Closed
wants to merge 10 commits into from
9 changes: 8 additions & 1 deletion app/models/operatingsystem.rb
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,14 @@ def self.find_by_to_label(str)
os = find_by_description(str.to_s)
return os if os
a = str.split(" ")
b = a[1].split('.') if a[1]
if a[0] == 'Ubuntu'
goarsna marked this conversation as resolved.
Show resolved Hide resolved
if a[1]
x, y, minor = a[1].split('.', 3)
b = ["#{x}.#{y}", minor]
end
else
b = a[1].split('.') if a[1]
end
cond = {:name => a[0]}
cond[:major] = b[0] if b && b[0]
cond[:minor] = b[1] if b && b[1]
Expand Down
8 changes: 7 additions & 1 deletion app/services/foreman_ansible/operating_system_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ def os_major
(facts[:ansible_distribution_major_version][%r{\/sid}i] ||
facts[:ansible_distribution_major_version] == 'n/a')
debian_os_major_sid
elsif os_name == 'Ubuntu'
facts[:ansible_distribution_major_version]
else
facts[:ansible_distribution_major_version] ||
facts[:ansible_lsb] && facts[:ansible_lsb]['major_release'] ||
Expand All @@ -68,8 +70,12 @@ def os_release
end

def os_minor
_, minor = os_release&.split('.', 2) ||
if os_name == 'Ubuntu'
_, _, minor = os_release&.split('.', 3)
else
_, minor = os_release&.split('.', 2) ||
(facts[:version].split('R') if os_name == 'junos')
end
# Until Foreman supports os.minor as something that's not a number,
# we should remove the extra dots in the version. E.g:
# '6.1.7601.65536' becomes '6.1.760165536'
Expand Down
2 changes: 2 additions & 0 deletions app/services/foreman_chef/fact_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ def operatingsystem
# if we have no release information we can't assign OS properly (e.g. missing redhat-lsb)
if release.nil?
major, minor = 1, nil
elsif os_name == 'Ubuntu'
major, minor = release, nil
else
major, minor = release.split('.')
end
Expand Down
7 changes: 6 additions & 1 deletion app/services/katello/rhsm_fact_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,12 @@ def operatingsystem
return nil if name.nil? || version.nil?

os_name = distribution_to_puppet_os(name)
major, minor = version.split('.')
if os_name == 'Ubuntu'
x, y, minor = version.split('.', 3)
major = "#{x}.#{y}"
else
major, minor = version.split('.', 2)
end
unless facts['ignore_os']
os_attributes = {:major => major, :minor => minor || '', :name => os_name}

Expand Down
19 changes: 16 additions & 3 deletions app/services/puppet_fact_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,14 @@ def operatingsystem
orel = os_release.dup

if orel.present?
major, minor = orel.split('.', 2)
major = major.to_s.gsub(/\D/, '')
minor = minor.to_s.gsub(/[^\d\.]/, '')
if os_name =~ /ubuntu/i
major = os_major_version
minor = os_minor_version
Comment on lines +9 to +10
Copy link
Member

Choose a reason for hiding this comment

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

I've been debating making this the behavior everywhere and would be OK with trying that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we simply leave out the if / else part and use os_major_version and os_minor_version for all operating systems? I also noticed that the if orel.present? isn't necessary here anymore, as the whole part from line 8 to line 24 is no longer dependent on it.

Copy link
Member

Choose a reason for hiding this comment

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

indeed, get rid of orel altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look at relying on the major and minor versions reported by facter and it doesn't fit for all operating systems. The os_release often uses the full release reported by facter and parses it as necessary. Relying on the reported major and minor versions breaks some of the OSes (like Solaris and CentOS 8).

IMO it would be best to rework detection of the OS version in the PuppetFactParser in a separate PR.

else
major, minor = orel.split('.')
major = major.to_s.gsub(/\D/, '')
minor = minor.to_s.gsub(/[^\d\.]/, '')
end
args = {:name => os_name, :major => major, :minor => minor}
os = Operatingsystem.find_or_initialize_by(args)
if os_name[/debian|ubuntu/i] || os.family == 'Debian'
Expand Down Expand Up @@ -227,6 +232,14 @@ def os_name
raise(::Foreman::Exception.new("invalid facts, missing operating system value"))
end

def os_major_version
facts.dig(:os, :release, :major)
end

def os_minor_version
facts.dig(:os, :release, :minor)
end

def os_release
case os_name
when /(windows)/i
Expand Down
12 changes: 12 additions & 0 deletions db/migrate/20230719080900_fix_ubuntu_versions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class FixUbuntuVersions < ActiveRecord::Migration[6.0]
def up
# For every Ubuntu operating system with a set minor version we
# * Check if there is already an operating system present with the same major and minor version combined in the major field
# * If not, we update the version
Operatingsystem.where(name: 'Ubuntu').where.not(minor: [nil, '']).each do |os|
unless Operatingsystem.find_by(name: 'Ubuntu', major: "#{os.major}.#{os.minor}", minor: [nil, ''])
goarsna marked this conversation as resolved.
Show resolved Hide resolved
os.update(major: "#{os.major}.#{os.minor}", minor: nil)
end
end
end
end