(FACT-2570) Use Facter options to store custom and external facts (#467)

* (FACT-2562) Remove reset from to_hash. Save external and custom facts in options.

* (FACT-2562) Read custom and external facts from options instead of LegacyFacter.

* (FACT-2562) Fix tests.

* (FACT-2562) Store custom and external directories in options in LegacyFacter

* (FACT-2570) Fix tests.

* (FACT-2570) Remove commented code.

* (FACT-2570) Revert cli.

* (FACT-2570) Remove unused methods.

* (FACT-2570) Remove commented code.

* (FACT-2570) When setting external facts to false, erase external_dir.

* (FACT-2570) Expect variable number of arguments in search method.

* (FACT-2570) Store default external dir separately.

* (FACT-2570) Reset default dir if external facts is false.

* (FACT-2570) Fix tests and rubocop.

* (FACT-2570) Remove commented code.

* (FACT-2570) Change the way default external facts are used.

* (FACT-2570) Rubocop fixes.

* (FACT-2570) If no custom dir is set, get the one from config file if it exists.

* (FACT-2570) If external dir is empty return external dir specified in config file.

* (FACT-2570) Rework condition for external dir.

* (FACT-2570) Append to external facts.

* (FACT-2570) Add nil checks and conversion to array.

* (FACT-2570) Return empty array if no custom dir in config file.

* (FACT-2570) Initialise config_file_custom_dir and config_file_external_dir with empty array if they have no value in config file.

* (FACT-2570) Remove unnecessary condition.

* (FACT-2570) Reformat condition for external dir.

* (FACT-2570) Extract config file external dir and default external dir in a different method.

* (FACT-2570) Remove unnecessary condition.

* (FACT-2570) Add test for external dir.

* (FACT-2570) Add test for custom_dir

lib/custom_facts/core/legacy_facter.rb

@@ -195,7 +195,6 @@ def self.clear
   # @api public
   def self.reset
     @collection = nil
-    reset_search_path!
   end
 
   # Loads all facts.
@@ -206,55 +205,4 @@ def self.reset
   def self.loadfacts
     collection.load_all
   end
-
-  # Register directories to be searched for facts. The registered directories
-  # must be absolute paths or they will be ignored.
-  #
-  # @param dirs [String] directories to search
-  #
-  # @return [void]
-  #
-  # @api public
-  def self.search(*dirs)
-    @search_path += dirs
-  end
-
-  # Returns the registered search directories.
-  #
-  # @return [Array<String>] An array of the directories searched
-  #
-  # @api public
-  def self.search_path
-    @search_path.dup
-  end
-
-  # Reset the Facter search directories.
-  #
-  # @api private
-  # @return [void]
-  def self.reset_search_path!
-    @search_path = []
-  end
-
-  reset_search_path!
-
-  # Registers directories to be searched for external facts.
-  #
-  # @param dirs [Array<String>] directories to search
-  #
-  # @return [void]
-  #
-  # @api public
-  def self.search_external(dirs)
-    LegacyFacter::Util::Config.external_facts_dirs += dirs
-  end
-
-  # Returns the registered search directories.
-  #
-  # @return [Array<String>] An array of the directories searched
-  #
-  # @api public
-  def self.search_external_path
-    LegacyFacter::Util::Config.external_facts_dirs.dup
-  end
 end

lib/custom_facts/util/config.rb

@@ -28,12 +28,8 @@ def self.windows_data_dir
         ENV['ProgramData'] || ENV['APPDATA'] if LegacyFacter::Util::Config.windows?
       end
 
-      def self.external_facts_dirs=(dir)
-        @external_facts_dirs = dir
-      end
-
       def self.external_facts_dirs
-        @external_facts_dirs
+        Facter::Options.external_dir
       end
 
       def self.facts_cache_dir
@@ -43,16 +39,16 @@ def self.facts_cache_dir
       def self.setup_default_ext_facts_dirs
         if LegacyFacter::Util::Root.root?
           windows_dir = windows_data_dir
-          @external_facts_dirs = if windows_dir
-                                   [File.join(windows_dir, 'PuppetLabs', 'facter', 'facts.d')]
-                                 else
-                                   ['/opt/puppetlabs/facter/facts.d']
-                                 end
+          Facter::Options[:default_external_dir] = if windows_dir
+                                                     [File.join(windows_dir, 'PuppetLabs', 'facter', 'facts.d')]
+                                                   else
+                                                     ['/opt/puppetlabs/facter/facts.d']
+                                                   end
         elsif ENV['HOME']
-          @external_facts_dirs =
+          Facter::Options[:default_external_dir] =
             [File.expand_path(File.join(ENV['HOME'], '.puppetlabs', 'opt', 'facter', 'facts.d'))]
         else
-          @external_facts_dirs = []
+          Facter::Options[:default_external_dir] = []
         end
       end
 

lib/custom_facts/util/loader.rb

@@ -72,7 +72,7 @@ def search_path
 
         search_paths.delete_if { |path| !valid_search_path?(path) }
 
-        LegacyFacter.search_path.each do |path|
+        Facter::Options.custom_dir.each do |path|
           if valid_search_path?(path)
             search_paths << path
           else

lib/facter.rb

@@ -58,6 +58,7 @@ def add(name, options = {}, &block)
     def clear
       @already_searched = {}
       LegacyFacter.clear
+      Options[:custom_dir] = []
       LegacyFacter.collection.invalidate_custom_facts
       LegacyFacter.collection.reload_custom_facts
     end
@@ -133,8 +134,6 @@ def reset
       LegacyFacter.reset
       Options[:custom_dir] = []
       Options[:external_dir] = []
-      LegacyFacter.search(*Options.custom_dir)
-      LegacyFacter.search_external(Options.external_dir)
       nil
     end
 
@@ -148,7 +147,6 @@ def reset
     # @api public
     def search(*dirs)
       Options[:custom_dir] += dirs
-      LegacyFacter.search(*dirs)
     end
 
     # Registers directories to be searched for external facts.
@@ -160,7 +158,6 @@ def search(*dirs)
     # @api public
     def search_external(dirs)
       Options[:external_dir] += dirs
-      LegacyFacter.search_external(dirs)
     end
 
     # Returns the registered search directories.for external facts.

lib/framework/core/fact_loaders/external_fact_loader.rb

@@ -16,18 +16,9 @@ def all_facts
 
     private
 
-    # The search paths must be set before creating the fact collection.
-    # If we set them after, they will not be visible.
-    def load_search_paths
-      LegacyFacter.reset_search_path!
-      LegacyFacter.search(*Options.custom_dir) if Options.custom_dir?
-      LegacyFacter.search_external(Options.external_dir) if Options.external_dir?
-    end
-
     def load_custom_facts
       custom_facts = []
 
-      load_search_paths
       custom_facts_to_load = LegacyFacter.collection.custom_facts
 
       custom_facts_to_load&.each do |custom_fact_name|
@@ -41,7 +32,6 @@ def load_custom_facts
     def load_external_facts
       external_facts = []
 
-      load_search_paths
       external_facts_to_load = LegacyFacter.collection.external_facts
 
       external_facts_to_load&.each do |external_fact_name|

lib/framework/core/options.rb

@@ -24,7 +24,7 @@ def custom_dir?
       end
 
       def custom_dir
-        OptionStore.custom_dir.flatten
+        [OptionStore.custom_dir].flatten
       end
 
       def external_dir?

lib/framework/core/options/config_file_options.rb

@@ -46,6 +46,7 @@ def augment_custom(file_global_conf)
         end
 
         @options[:custom_dir] = file_global_conf['custom-dir'] unless file_global_conf['custom-dir'].nil?
+        @options[:config_file_custom_dir] = @options[:custom_dir] || []
       end
 
       def augment_external(global_conf)
@@ -56,6 +57,7 @@ def augment_external(global_conf)
         end
 
         @options[:external_dir] = [global_conf['external-dir']].flatten unless global_conf['external-dir'].nil?
+        @options[:config_file_external_dir] = @options[:external_dir] || []
       end
 
       def augment_ruby(global_conf)

lib/framework/core/options/option_store.rb

@@ -11,21 +11,26 @@ class OptionStore
     @show_legacy = true
     @block = true
     @custom_dir = []
+    @config_file_custom_dir = []
     @custom_facts = true
     @external_dir = []
+    @config_file_external_dir = []
+    @default_external_dir = []
     @external_facts = true
     @ruby = true
     @cache = true
     @blocked_facts = []
     @user_query = []
 
     class << self
-      attr_reader :debug, :verbose, :log_level, :show_legacy, :trace,
-                  :custom_dir, :external_dir, :ruby,
+      attr_reader :debug, :verbose, :log_level, :show_legacy, :trace, :ruby,
                   :custom_facts, :blocked_facts
 
       attr_accessor :config, :user_query, :strict, :json, :haml, :external_facts,
-                    :cache, :yaml, :puppet, :ttls, :block, :cli
+                    :cache, :yaml, :puppet, :ttls, :block, :cli, :config_file_custom_dir,
+                    :config_file_external_dir, :default_external_dir
+
+      attr_writer :external_dir
 
       def all
         options = {}
@@ -46,10 +51,10 @@ def ruby=(bool)
         end
       end
 
-      def external_dir=(dirs)
-        return unless dirs.any?
+      def external_dir
+        return fallback_external_dir if @external_dir.empty? && @external_facts
 
-        @external_dir = dirs
+        @external_dir
       end
 
       def blocked_facts=(*facts)
@@ -58,9 +63,13 @@ def blocked_facts=(*facts)
         @blocked_facts.flatten!
       end
 
-      def custom_dir=(*dirs)
-        return unless dirs.any?
+      def custom_dir
+        return @config_file_custom_dir unless @custom_dir.any?
+
+        @custom_dir
+      end
 
+      def custom_dir=(*dirs)
         @ruby = true
 
         @custom_dir = [*dirs]
@@ -146,13 +155,20 @@ def reset
         @custom_dir = []
         @custom_facts = true
         @external_dir = []
+        @default_external_dir = []
         @external_facts = true
         @ruby = true
         @blocked_facts = []
         @user_query = []
         @cli = nil
         @cache = true
       end
+
+      def fallback_external_dir
+        return @config_file_external_dir if @config_file_external_dir.any?
+
+        @default_external_dir
+      end
     end
   end
 end

spec/custom_facts/util/config_spec.rb

@@ -94,7 +94,7 @@
     it 'onlies output new values when explicitly set' do
       LegacyFacter::Util::Config.setup_default_ext_facts_dirs
       new_value = ['/usr/share/newdir']
-      LegacyFacter::Util::Config.external_facts_dirs = new_value
+      Facter::Options[:external_dir] = new_value
       expect(LegacyFacter::Util::Config.external_facts_dirs).to eq new_value
     end
   end

spec/custom_facts/util/loader_spec.rb

@@ -92,7 +92,8 @@ def loader_from(places)
     end
 
     it 'includes all search paths registered with Facter' do
-      allow(LegacyFacter).to receive(:search_path).and_return %w[/one /two]
+      allow(Facter::Options).to receive(:custom_dir).and_return %w[/one /two]
+
       allow(loader).to receive(:valid_search_path?).and_return true
 
       allow(File).to receive(:directory?).and_return false
@@ -104,7 +105,8 @@ def loader_from(places)
     end
 
     it 'warns on invalid search paths registered with Facter' do
-      allow(LegacyFacter).to receive(:search_path).and_return %w[/one two/three]
+      allow(Facter::Options).to receive(:custom_dir).and_return %w[/one two/three]
+
       allow(loader).to receive(:valid_search_path?).and_return false
       allow(loader).to receive(:valid_search_path?).with('/one').and_return true
       allow(loader).to receive(:valid_search_path?).with('two/three').and_return false
@@ -119,8 +121,9 @@ def loader_from(places)
       expect(paths).to match_array(['/one'])
     end
 
-    it 'strips paths that are valid paths but not are not present' do
-      allow(LegacyFacter).to receive(:search_path).and_return %w[/one /two]
+    it 'strips paths that are valid paths but are not present' do
+      allow(Facter::Options).to receive(:custom_dir).and_return %w[/one /two]
+
       allow(loader).to receive(:valid_search_path?).and_return false
       allow(loader).to receive(:valid_search_path?).with('/one').and_return true
       allow(loader).to receive(:valid_search_path?).with('/two').and_return true

spec/facter/facter_spec.rb

@@ -276,11 +276,11 @@
   end
 
   describe '#search' do
-    it 'sends call to LegacyFacter' do
+    it 'sends call to Facter::Options' do
       dirs = ['/dir1', '/dir2']
 
-      expect(LegacyFacter).to receive(:search).with(dirs)
-      Facter.search(dirs)
+      expect(Facter::Options).to receive(:[]=).with(:custom_dir, dirs)
+      Facter.search(*dirs)
     end
   end
 
@@ -292,10 +292,10 @@
   end
 
   describe '#search_external' do
-    it 'sends call to LegacyFacter' do
+    it 'sends call to Facter::Options' do
       dirs = ['/dir1', '/dir2']
+      expect(Facter::Options).to receive(:[]=).with(:external_dir, dirs)
 
-      expect(LegacyFacter).to receive(:search_external).with(dirs)
       Facter.search_external(dirs)
     end
   end
@@ -315,15 +315,15 @@
     end
 
     it 'adds custom facts dirs' do
-      allow(LegacyFacter).to receive(:search)
+      allow(Facter::Options).to receive(:[]=)
       Facter.reset
-      expect(LegacyFacter).to have_received(:search).once
+      expect(Facter::Options).to have_received(:[]=).with(:custom_dir, [])
     end
 
     it 'add external facts dirs' do
-      allow(LegacyFacter).to receive(:search_external)
+      allow(Facter::Options).to receive(:[]=)
       Facter.reset
-      expect(LegacyFacter).to have_received(:search_external).once
+      expect(Facter::Options).to have_received(:[]=).with(:external_dir, [])
     end
   end