make hostname and fact_column arrays

.rubocop_todo.yml

@@ -55,7 +55,6 @@ Layout/EmptyLineAfterGuardClause:
     - 'app/models/host/discovered.rb'
     - 'app/models/host/managed_extensions.rb'
     - 'app/models/nic/managed_extensions.rb'
-    - 'app/models/setting/discovered.rb'
     - 'app/services/foreman_discovery/import_hooks/lldp_neighbor.rb'
     - 'app/services/foreman_discovery/lldp_neighbors.rb'
     - 'app/services/foreman_discovery/node_api/node_resource.rb'
@@ -229,7 +228,6 @@ Layout/SpaceAroundOperators:
 Layout/SpaceBeforeBlockBraces:
   Exclude:
     - 'app/models/host/discovered.rb'
-    - 'app/models/setting/discovered.rb'
     - 'test/test_helper_discovery.rb'
 
 # Offense count: 3
@@ -255,7 +253,6 @@ Layout/SpaceInsideBlockBraces:
   Exclude:
     - 'app/controllers/discovered_hosts_controller.rb'
     - 'app/models/host/discovered.rb'
-    - 'app/models/setting/discovered.rb'
     - 'app/services/foreman_discovery/lldp_neighbors.rb'
     - 'test/functional/api/v2/discovery_rules_controller_test.rb'
     - 'test/test_helper_discovery.rb'
@@ -316,7 +313,6 @@ Lint/RescueException:
   Exclude:
     - 'app/controllers/api/v2/discovered_hosts_controller.rb'
     - 'app/controllers/concerns/foreman/controller/discovered_extensions.rb'
-    - 'app/models/setting/discovered.rb'
     - 'app/services/foreman_discovery/node_api/node_resource.rb'
 
 # Offense count: 7
@@ -450,21 +446,13 @@ Rails/AssertNot:
     - 'test/functional/api/v2/discovered_hosts_controller_test.rb'
     - 'test/functional/api/v2/discovery_rules_controller_test.rb'
 
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: NilOrEmpty, NotPresent, UnlessPresent.
-Rails/Blank:
-  Exclude:
-    - 'app/models/setting/discovered.rb'
-
 # Offense count: 34
 # Cop supports --auto-correct.
 # Configuration parameters: Whitelist.
 # Whitelist: find_by_sql
 Rails/DynamicFindBy:
   Exclude:
     - 'app/controllers/discovered_hosts_controller.rb'
-    - 'app/models/setting/discovered.rb'
     - 'app/services/foreman_discovery/import_hooks/lock_templates.rb'
     - 'app/services/foreman_discovery/import_hooks/subnet_and_taxonomy.rb'
     - 'test/functional/api/v2/settings_controller_test.rb'
@@ -546,7 +534,6 @@ Style/ClassAndModuleChildren:
     - 'app/models/host/discovered.rb'
     - 'app/models/host/managed_extensions.rb'
     - 'app/models/nic/managed_extensions.rb'
-    - 'app/models/setting/discovered.rb'
     - 'app/services/foreman_discovery/host_converter.rb'
     - 'app/services/foreman_discovery/node_api/inventory.rb'
     - 'app/services/foreman_discovery/node_api/node_resource.rb'
@@ -647,7 +634,6 @@ Style/MutableConstant:
 Style/NegatedIf:
   Exclude:
     - 'app/models/host/discovered.rb'
-    - 'app/models/setting/discovered.rb'
 
 # Offense count: 1
 # Cop supports --auto-correct.
@@ -672,12 +658,6 @@ Style/PercentLiteralDelimiters:
     - 'app/controllers/api/v2/discovery_rules_controller.rb'
     - 'test/unit/lldp_neighbors_test.rb'
 
-# Offense count: 6
-# Cop supports --auto-correct.
-Style/Proc:
-  Exclude:
-    - 'app/models/setting/discovered.rb'
-
 # Offense count: 9
 # Cop supports --auto-correct.
 # Configuration parameters: EnforcedStyle.
@@ -728,7 +708,6 @@ Style/RedundantSelf:
     - 'app/models/host/discovered.rb'
     - 'app/models/host/managed_extensions.rb'
     - 'app/models/nic/managed_extensions.rb'
-    - 'app/models/setting/discovered.rb'
     - 'app/services/foreman_discovery/lldp_neighbors.rb'
 
 # Offense count: 4

app/controllers/discovered_hosts_controller.rb

@@ -35,7 +35,7 @@ def index
       :discovery_attribute_set
     ], {:interfaces => :subnet})
     fact_array = @hosts.collect do |host|
-      [host.id, Hash[host.fact_values.joins(:fact_name).where('fact_names.name' => Setting::Discovered.discovery_fact_column_array).pluck(:name, :value)]]
+      [host.id, Hash[host.fact_values.joins(:fact_name).where('fact_names.name' => Setting['discovery_fact_column']).pluck(:name, :value)]]
     end
     @host_facts = Hash[fact_array]
   end

app/models/host/discovered.rb

@@ -63,15 +63,15 @@ def self.import_host facts
     bootif_mac = FacterUtils::bootif_mac(facts).try(:downcase)
     hostname = ''
     if Setting[:discovery_naming] == 'MAC-name'
-      hostname_mac = return_first_valid_mac(Setting::Discovered.discovery_hostname_fact_array, facts) || bootif_mac
+      hostname_mac = return_first_valid_mac(Setting['discovery_hostname'], facts) || bootif_mac
       hostname = NameGenerator.new.generate_next_mac_name(hostname_mac)
     elsif Setting[:discovery_naming] == 'Random-name'
       hostname = NameGenerator.new.generate_next_random_name
     else
       prefix_from_settings = Setting[:discovery_prefix]
       hostname_prefix = prefix_from_settings if prefix_from_settings.present? && prefix_from_settings.match(/^[a-zA-Z].*/)
-      name_fact = return_first_valid_fact(Setting::Discovered.discovery_hostname_fact_array, facts)
-      raise(::Foreman::Exception.new(N_("Invalid facts: hash does not contain a valid value for any of the facts in the discovery_hostname setting: %s"), Setting::Discovered.discovery_hostname_fact_array.join(', '))) unless name_fact && name_fact.present?
+      name_fact = return_first_valid_fact(Setting['discovery_hostname'], facts)
+      raise(::Foreman::Exception.new(N_("Invalid facts: hash does not contain a valid value for any of the facts in the discovery_hostname setting: %s"), Setting['discovery_hostname'].join(', '))) unless name_fact && name_fact.present?
       hostname = normalize_string_for_hostname("#{hostname_prefix}#{name_fact}")
     end
     Rails.logger.warn "Hostname does not start with an alphabetical character" unless hostname.downcase.match(/^[a-z]/)

app/views/discovered_hosts/_discovered_host.html.erb

@@ -6,7 +6,7 @@
 <td class="hidden-tablet hidden-xs"><%= discovery_attribute(host, :disk_count) %></td>
 <td class="hidden-tablet hidden-xs"><%= number_to_human_size(discovery_attribute(host, :disks_size, 0) * 1024 * 1024) %></td>
 <%
-  Setting::Discovered.discovery_fact_column_array.each do |name|
+  Setting['discovery_fact_column'].each do |name|
 %>
 <td class="hidden-tablet hidden-xs"><%= @host_facts[host.id][name] || 'N/A' %></td>
 <% end %>

app/views/discovered_hosts/_discovered_hosts_list.html.erb

@@ -10,7 +10,7 @@
       <th class="hidden-tablet hidden-xs"><%= sort :memory, :as => _('Memory') %></th>
       <th class="hidden-tablet hidden-xs"><%= sort :disk_count, :as => _('Disk Count') %></th>
       <th class="hidden-tablet hidden-xs"><%= sort :disks_size, :as => _('Disks Size') %></th>
-      <% Setting::Discovered.discovery_fact_column_array.each do |fact_column| %>
+      <% Setting['discovery_fact_column'].each do |fact_column| %>
         <th class="hidden-tablet hidden-xs"><%= fact_column.capitalize %></th>
       <% end %>
       <th class="hidden-tablet hidden-xs"><%= sort :location, :as => _('Location') %></th>

db/migrate/20221102075151_migrate_discovery_hostname_and_fact_column_to_array.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class MigrateDiscoveryHostnameAndFactColumnToArray < ActiveRecord::Migration[6.0]
+  def up
+    ['discovery_hostname', 'discovery_fact_column'].each do |setting_name|
+      setting = Setting.find_by(name: setting_name)
+      if !setting.nil? && setting.value.is_a?(String)
+        setting.value = setting.value.split(",").map(&:strip)
+        setting.save
+      end
+    end
+  end
+end

lib/foreman_discovery/engine.rb

@@ -15,12 +15,6 @@ class Engine < ::Rails::Engine
     config.autoload_paths += Dir["#{config.root}/app/models/concerns"]
     config.autoload_paths += Dir["#{config.root}/app/services"]
 
-    # Load this before the Foreman config initializers, so that the Setting.descendants
-    # list includes the plugin STI setting class
-    initializer 'foreman_discovery.load_default_settings', :before => :load_config_initializers do |app|
-      require_dependency File.expand_path("../../../app/models/setting/discovered.rb", __FILE__) if (Setting.table_exists? rescue(false))
-    end
-
     initializer "foreman_discovery.add_rabl_view_path" do |app|
       Rabl.configure do |config|
         config.view_paths << ForemanDiscovery::Engine.root.join('app', 'views')
@@ -85,10 +79,10 @@ class Engine < ::Rails::Engine
               description: N_("Clean all reported facts during provisioning (except discovery facts)")
 
             setting "discovery_hostname",
-              type: :string,
-              default: "discovery_bootif",
+              type: :array,
+              default: ["discovery_bootif"],
               full_name: N_("Hostname facts"),
-              description: N_("List of facts to use for the hostname (separated by comma, first wins)")
+              description: N_("List of facts to use for the hostname (first wins)")
 
             validates "discovery_hostname", presence: true
 
@@ -113,10 +107,10 @@ class Engine < ::Rails::Engine
             validates "discovery_prefix", presence: true
 
             setting "discovery_fact_column",
-              type: :string,
-              default: "",
+              type: :array,
+              default: [],
               full_name: N_("Fact columns"),
-              description: N_("Extra facter columns to show in host lists (separate by comma)")
+              description: N_("Extra facter columns to show in host lists")
 
             setting "discovery_facts_highlights",
               type: :string,

test/functional/api/v2/fact_value_extensions_test.rb

@@ -19,7 +19,7 @@ class FactValuesControllerExtensionsTest < ActionController::TestCase
             "memorysize_mb"     => "42000.42",
             "discovery_version" => "3.0.0",
         }
-        Setting['discovery_hostname'] = 'discovery_bootif'
+        Setting['discovery_hostname'] = ['discovery_bootif']
       end
 
 

test/functional/discovered_hosts_controller_test.rb

@@ -28,7 +28,7 @@ def test_index
   end
 
   def test_index_with_custom_column
-    Setting['discovery_fact_column'] = "bios_vendor"
+    Setting['discovery_fact_column'] = ["bios_vendor"]
     facts = @facts.merge({"bios_vendor" => "QEMU"})
     discover_host_from_facts(facts)
     get :index, params: {}, session: set_session_user_default_reader

test/migrations/20221102075151_migrate_discovery_hostname_and_fact_column_to_array_test.rb

@@ -0,0 +1,89 @@
+require_relative '../test_plugin_helper'
+require ForemanDiscovery::Engine.root.join('db/migrate/20221102075151_migrate_discovery_hostname_and_fact_column_to_array')
+
+class MigrateDiscoveryHostnameAndFactColumnToArrayTest < ActiveSupport::TestCase
+  let(:migrations_paths) { ActiveRecord::Migrator.migrations_paths + [ForemanDiscovery::Engine.root.join('db/migrate/').to_s] }
+
+  let(:previous_version) { '20221102065954'.to_i }
+  let(:current_version) { '20221102075151'.to_i }
+
+  #only load the two migrations we care about (previous one and current one)
+  let(:migrations) do
+    [
+      ActiveRecord::MigrationProxy.new("FixDiscoverySettingsCategoryToDsl", previous_version, "#{ForemanDiscovery::Engine.root}/db/migrate/20221102065954_fix_discovery_settings_category_to_dsl.rb", ""),
+      ActiveRecord::MigrationProxy.new("MigrateDiscoveryHostnameAndFactColumnToArray", current_version, "#{ForemanDiscovery::Engine.root}/db/migrate/20221102075151_migrate_discovery_hostname_and_fact_column_to_array.rb", "")
+    ]
+  end
+
+  def migrate_up
+    ActiveRecord::Migrator.new(:up, migrations, ActiveRecord::SchemaMigration, current_version).migrate
+  end
+
+  def setup
+    ActiveRecord::Migration.suppress_messages do
+      ActiveRecord::Migrator.new(:down, migrations, ActiveRecord::SchemaMigration, previous_version).migrate
+    end
+  end
+
+  def test_discovery_hostname_string
+    Setting['discovery_hostname'] = 'discovery_bootif'
+
+    migrate_up
+
+    assert_equal ['discovery_bootif'], Setting['discovery_hostname']
+  end
+
+  def test_discovery_hostname_multistring
+    setting = Setting.find_or_create_by(name: 'discovery_hostname')
+    setting.value = 'discovery_bootif, fqdn'
+    setting.save(validate: false)
+
+    migrate_up
+
+    assert_equal ['discovery_bootif', 'fqdn'], Setting['discovery_hostname']
+  end
+
+  def test_discovery_hostname_array
+    Setting['discovery_hostname'] = ['discovery_bootif']
+
+    migrate_up
+
+    assert_equal ['discovery_bootif'], Setting['discovery_hostname']
+  end
+
+  def test_discovery_fact_column_empty
+    setting = Setting.find_or_create_by(name: 'discovery_fact_column')
+    setting.value = ''
+    setting.save(validate: false)
+
+    migrate_up
+
+    assert_equal [], Setting['discovery_fact_column']
+  end
+
+  def test_discovery_fact_column_string
+    Setting['discovery_fact_column'] = 'bios_vendor'
+
+    migrate_up
+
+    assert_equal ['bios_vendor'], Setting['discovery_fact_column']
+  end
+
+  def test_discovery_fact_column_multistring
+    setting = Setting.find_or_create_by(name: 'discovery_fact_column')
+    setting.value = 'bios_vendor, fqdn'
+    setting.save(validate: false)
+
+    migrate_up
+
+    assert_equal ['bios_vendor', 'fqdn'], Setting['discovery_fact_column']
+  end
+
+  def test_discovery_fact_column_array
+    Setting['discovery_fact_column'] = ['bios_vendor']
+
+    migrate_up
+
+    assert_equal ['bios_vendor'], Setting['discovery_fact_column']
+  end
+end

test/test_helper_discovery.rb

@@ -53,7 +53,7 @@ def extract_form_errors(response)
 
 def set_default_settings
   Setting['discovery_fact'] = 'discovery_bootif'
-  Setting['discovery_hostname'] = 'discovery_bootif'
+  Setting['discovery_hostname'] = ['discovery_bootif']
   Setting['discovery_auto'] = true
   Setting['discovery_reboot'] = true
   Setting['discovery_organization'] = "Organization 1"

test/unit/discovery_attribute_set_test.rb

@@ -6,7 +6,7 @@ class DiscoveryAttributeSetTest < ActiveSupport::TestCase
 
   setup do
     @facts = parse_json_fixture('regular_host', true)
-    Setting['discovery_hostname'] = 'discovery_bootif'
+    Setting['discovery_hostname'] = ['discovery_bootif']
     Setting['discovery_prefix'] = 'mac'
     ::ForemanDiscovery::HostConverter.stubs(:unused_ip_for_host)
   end

test/unit/host_discovered_test.rb

@@ -181,7 +181,7 @@ class HostDiscoveredTest < ActiveSupport::TestCase
   end
 
   test "should create discovered host with hostname if a fact was supplied" do
-    Setting[:discovery_hostname] = 'somefact'
+    Setting[:discovery_hostname] = ['somefact']
     facts = @facts.merge({"somefact" => "somename"})
     host = discover_host_from_facts(facts)
     assert_equal 'macsomename', host.name
@@ -219,7 +219,7 @@ class HostDiscoveredTest < ActiveSupport::TestCase
 
   test "should create discovered host with fact_name as a name if it is a valid mac" do
     Setting[:discovery_fact] = 'somefact'
-    Setting[:discovery_hostname] = 'somefact'
+    Setting[:discovery_hostname] = ['somefact']
     facts = @facts.merge({"somefact" => "E4:1F:13:CC:36:5A"})
     host = discover_host_from_facts(facts)
     assert_equal 'mace41f13cc365a', host.name
@@ -277,7 +277,7 @@ class HostDiscoveredTest < ActiveSupport::TestCase
   end
 
   test "should raise when hostname fact cannot be found" do
-    Setting[:discovery_hostname] = 'macaddress_foo'
+    Setting[:discovery_hostname] = ['macaddress_foo']
     exception = assert_raises(::Foreman::Exception) do
       discover_host_from_facts(@facts)
     end