Fixes #38930 - Make fact parsers use proper os

app/models/operatingsystem.rb

@@ -46,6 +46,18 @@ class Operatingsystem < ApplicationRecord
 
   default_scope -> { order(:title) }
 
+  # Find all operatingsystems that are duplicates of the given operating system according to all unique constraints
+  def self.find_by_attributes(name: nil, major: nil, minor: nil, description: nil)
+    where_attributes = {
+      name: name,
+      major: major,
+      minor: minor,
+    }.compact
+    scope = where(where_attributes)
+    scope = scope.or(where(description: description)) if description.present?
+    scope.or(where(title: generate_title(**where_attributes.merge(description: description))))
+  end
+
   scoped_search :on => :id, :complete_enabled => false, :only_explicit => true, :validator => ScopedSearch::Validators::INTEGER
   scoped_search :on => :name,        :complete_value => :true
   scoped_search :on => :major,       :complete_value => :true
@@ -139,10 +151,26 @@ def self.families_as_collection
     end
   end
 
+  def self.generate_title(description:, name:, major:, minor:)
+    to_label(description: description, name: name, major: major, minor: minor).to_s[0..254]
+  end
+
+  def self.to_label(description:, name:, major:, minor:)
+    return description if description.present?
+    fullname(name: name, major: major, minor: minor)
+  end
+
+  def self.fullname(name:, major:, minor:)
+    "#{name} #{release(major: major, minor: minor)}"
+  end
+
+  def self.release(major:, minor:)
+    "#{major}#{('.' + minor.to_s) if minor.present?}"
+  end
+
   # The OS is usually represented as the concatenation of the OS and the revision
   def to_label
-    return description if description.present?
-    fullname
+    self.class.to_label(description: description, name: name, major: major, minor: minor)
   end
 
   # to_label setter updates description and does not try to parse and update major, minor attributes
@@ -155,11 +183,11 @@ def to_param
   end
 
   def release
-    "#{major}#{('.' + minor.to_s) if minor.present?}"
+    self.class.release(major: major, minor: minor)
   end
 
   def fullname
-    "#{name} #{release}"
+    self.class.fullname(name: name, major: major, minor: minor)
   end
 
   def to_s
@@ -384,7 +412,7 @@ def set_family
   end
 
   def set_title
-    self.title = to_label.to_s[0..254]
+    self.title = self.class.generate_title(description: description, name: name, major: major, minor: minor)
   end
 
   def stringify_major_and_minor

app/services/foreman_ansible/operating_system_parser.rb

@@ -8,6 +8,7 @@ def operatingsystem
       args[:release_name] = os_release_name if os_name == 'Debian' || os_name == 'Ubuntu'
       # for Ansible, the CentOS Stream can be identified by missing minor version only
       args[:name] = "CentOS_Stream" if os_name == 'CentOS' && os_minor.blank?
+      args[:description] = os_description
       return @local_os if local_os(args).present?
       return @new_os if new_os(args).present?
       logger.debug do
@@ -20,15 +21,19 @@ def operatingsystem
     end
 
     def local_os(args)
-      @local_os = Operatingsystem.where(args).first
+      @local_os = Operatingsystem.find_by_attributes(**args.slice(:name, :major, :minor, :description)).first
     end
 
     def new_os(args)
       return @new_os if @new_os.present?
-      @new_os = Operatingsystem.new(args.merge(:description => os_description))
+      @new_os = generate_new_os(args)
       @new_os if @new_os.valid? && @new_os.save
     end
 
+    def generate_new_os(args)
+      Operatingsystem.new(args.merge(:description => os_description))
+    end
+
     def debian_os_major_sid
       release = if facts[:ansible_distribution_major_version] == 'n/a'
                   facts[:ansible_distribution_release]

app/services/foreman_chef/fact_parser.rb

@@ -51,7 +51,7 @@ def operatingsystem
       end
 
       args = { :name => os_name, :major => major.to_s, :minor => minor.to_s }
-      klass.where(args).first || klass.new(args.merge(:release_name => release_name)).tap(&:save!)
+      Operatingsystem.find_by_attributes(**args).first || klass.new(args.merge(:release_name => release_name)).tap(&:save!)
     end
 
     # we don't want to parse puppet environment, foreman_chef already has its own environment

app/services/foreman_salt/fact_parser.rb

@@ -3,7 +3,7 @@ class FactParser < ::FactParser
     attr_reader :facts
 
     def operatingsystem
-      os = Operatingsystem.where(os_hash).first_or_initialize
+      os = Operatingsystem.find_by_attributes(**os_hash).first || Operatingsystem.new(os_hash)
       if os.new_record?
         os.deduce_family
         os.release_name = facts[:oscodename] || facts[:lsb_distrib_codename]

app/services/katello/rhsm_fact_parser.rb

@@ -73,9 +73,9 @@ def operatingsystem
           os_attributes[:name] = "CentOS"
         end
 
-        os = ::Operatingsystem.find_by(os_attributes)
+        os = ::Operatingsystem.find_by_attributes(**os_attributes.slice(:name, :major, :minor, :description)).first
         if os.blank?
-          created = ::Operatingsystem.create_or_find_by(os_attributes)
+          created = ::Operatingsystem.create(os_attributes)
           created.errors.any? ? ::Operatingsystem.find_by(os_attributes) : created
         else
           os

app/services/puppet_fact_parser.rb

@@ -4,6 +4,8 @@ class PuppetFactParser < FactParser
   def operatingsystem
     orel = os_release.dup
 
+    args = { :name => os_name }
+
     if orel.present?
       if os_name =~ /ubuntu/i
         major = os_major_version.to_s
@@ -13,29 +15,24 @@ def operatingsystem
         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'
-        if distro_codename
-          os.release_name = distro_codename
-        elsif os.release_name.blank?
-          os.release_name = 'unknown'
-        end
+      args[:major] = major
+      args[:minor] = minor
+      if os_name[/debian|ubuntu/i]
+        args[:release_name] = distro_codename if distro_codename
+        args[:release_name] = 'unknown' if args[:release_name].blank?
       end
-    else
-      os = Operatingsystem.find_or_initialize_by(:name => os_name)
     end
 
-    if os.description.blank?
-      if os_name == 'SLES'
-        os.description = os_name + ' ' + orel.gsub('.', ' SP')
-      elsif distro_description
-        family = os.deduce_family || 'Operatingsystem'
-        os = os.becomes(family.constantize)
-        os.description = os.shorten_description(distro_description)
-      end
+    if os_name == 'SLES'
+      args[:description] = "#{os_name} #{orel.gsub('.', ' SP')}"
+    elsif distro_description
+      family = Operatingsystem.deduce_family(os_name) || 'Operatingsystem'
+      os_class = family.constantize
+      args[:description] = os_class.new(args).shorten_description(distro_description)
     end
 
+    os = Operatingsystem.find_by_attributes(**args.slice(:name, :major, :minor, :description)).first || Operatingsystem.new(args)
+
     if os.new_record?
       os.save!
       Operatingsystem.find_by_id(os.id) # complete reload to be an instance of the STI subclass