Merge pull request #2722 from joshcooper/logging

(FACT-3468) Don't mutate global logger state

lib/facter/custom_facts/core/aggregate.rb

@@ -109,7 +109,7 @@ def options(options)
       # @api private
       def evaluate(&block)
         if @last_evaluated
-          msg = "Already evaluated #{@name}"
+          msg = +"Already evaluated #{@name}"
           msg << " at #{@last_evaluated}" if msg.is_a? String
           msg << ', reevaluating anyways'
           log.warn msg
@@ -197,6 +197,10 @@ def resolution_type
 
       private
 
+      def log
+        @log ||= Facter::Log.new(self)
+      end
+
       def evaluate_params(name)
         raise ArgumentError, "#{self.class.name}#chunk requires a block" unless block_given?
         raise ArgumentError, "#{self.class.name}#expected chunk name to be a Symbol" unless name.is_a? Symbol

lib/facter/resolvers/base_resolver.rb

@@ -3,8 +3,8 @@
 module Facter
   module Resolvers
     class BaseResolver
-      def self.log
-        @log ||= Log.new(self)
+      class << self
+        attr_reader :log
       end
 
       def self.invalidate_cache
@@ -14,6 +14,7 @@ def self.invalidate_cache
       def self.init_resolver
         @fact_list = {}
         @semaphore = Mutex.new
+        @log = Log.new(self)
       end
 
       def self.subscribe_to_manager

lib/facter/resolvers/bsd/processors.rb

@@ -5,7 +5,6 @@ module Resolvers
     module Bsd
       class Processors < BaseResolver
         init_resolver
-        @log = Facter::Log.new(self)
 
         class << self
           private

lib/facter/resolvers/disks.rb

@@ -4,8 +4,6 @@ module Facter
   module Resolvers
     module Linux
       class Disks < BaseResolver
-        @log = Facter::Log.new(self)
-
         init_resolver
 
         DIR = '/sys/block'

lib/facter/resolvers/dmi.rb

@@ -4,8 +4,6 @@ module Facter
   module Resolvers
     module Linux
       class DmiBios < BaseResolver
-        @log = Facter::Log.new(self)
-
         init_resolver
 
         class << self

lib/facter/resolvers/filesystems.rb

@@ -8,8 +8,6 @@ class Filesystems < BaseResolver
 
         init_resolver
 
-        @log = Facter::Log.new(self)
-
         class << self
           private
 

lib/facter/resolvers/fips_enabled.rb

@@ -8,8 +8,6 @@ class FipsEnabled < BaseResolver
 
         init_resolver
 
-        @log = Facter::Log.new(self)
-
         class << self
           private
 

lib/facter/resolvers/freebsd/processors.rb

@@ -7,7 +7,6 @@ module Resolvers
     module Freebsd
       class Processors < BaseResolver
         init_resolver
-        @log = Facter::Log.new(self)
 
         class << self
           private

lib/facter/resolvers/hostname.rb

@@ -19,6 +19,11 @@ def post_resolve(fact_name, _options)
         def retrieve_info(fact_name)
           require 'socket'
           output = Socket.gethostname
+          unless output
+            log.debug('Socket.gethostname failed to return hostname')
+            return
+          end
+
           hostname, domain = retrieve_from_fqdn(output)
 
           fqdn = retrieve_with_addrinfo(hostname) if hostname_and_no_domain?(hostname, domain)

lib/facter/resolvers/identity.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class PosxIdentity < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       class << self

lib/facter/resolvers/macosx/mountpoints.rb

@@ -7,8 +7,6 @@ class Mountpoints < BaseResolver
         include Facter::Util::Resolvers::FilesystemHelper
         init_resolver
 
-        @log = Facter::Log.new(self)
-
         class << self
           private
 

lib/facter/resolvers/memory.rb

@@ -6,8 +6,6 @@ module Linux
       class Memory < BaseResolver
         init_resolver
 
-        @log = Facter::Log.new(self)
-
         class << self
           private
 

lib/facter/resolvers/mountpoints.rb

@@ -7,8 +7,6 @@ class Mountpoints < BaseResolver
 
       init_resolver
 
-      @log = Facter::Log.new(self)
-
       class << self
         private
 

lib/facter/resolvers/processors.rb

@@ -4,8 +4,6 @@ module Facter
   module Resolvers
     module Linux
       class Processors < BaseResolver
-        @log = Facter::Log.new(self)
-
         init_resolver
 
         MHZ_TO_HZ = 1_000_000

lib/facter/resolvers/solaris/mountpoints.rb

@@ -7,8 +7,6 @@ class Mountpoints < BaseResolver
         include Facter::Util::Resolvers::FilesystemHelper
         init_resolver
 
-        @log = Facter::Log.new(self)
-
         class << self
           private
 

lib/facter/resolvers/solaris/networking.rb

@@ -6,7 +6,6 @@ module Resolvers
     module Solaris
       class Networking < BaseResolver
         init_resolver
-        @log = Facter::Log.new(self)
 
         class << self
           private

lib/facter/resolvers/ssh.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class Ssh < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       FILE_NAMES = %w[ssh_host_rsa_key.pub ssh_host_dsa_key.pub ssh_host_ecdsa_key.pub ssh_host_ed25519_key.pub].freeze

lib/facter/resolvers/uname.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class Uname < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       class << self

lib/facter/resolvers/windows/dmi_bios.rb

@@ -3,7 +3,6 @@
 module Facter
   module Resolvers
     class DMIBios < BaseResolver
-      @log = Facter::Log.new(self)
       init_resolver
 
       class << self

lib/facter/resolvers/windows/dmi_computersystem.rb

@@ -3,7 +3,6 @@
 module Facter
   module Resolvers
     class DMIComputerSystem < BaseResolver
-      @log = Facter::Log.new(self)
       init_resolver
 
       class << self

lib/facter/resolvers/windows/hardware_architecture.rb

@@ -24,7 +24,6 @@ def read_hardware_information(fact_name)
           build_facts_list(hardware: hard, architecture: arch)
           @fact_list[fact_name]
         rescue LoadError => e
-          log = Facter::Log.new(self)
           log.debug("The ffi gem has not been installed: #{e}")
         end
 

lib/facter/resolvers/windows/identity.rb

@@ -4,7 +4,6 @@ module Facter
   module Resolvers
     class Identity < BaseResolver
       NAME_SAM_COMPATIBLE = 2
-      @log = Facter::Log.new(self)
 
       init_resolver
 

lib/facter/resolvers/windows/kernel.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class Kernel < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       class << self

lib/facter/resolvers/windows/memory.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class Memory < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       class << self

lib/facter/resolvers/windows/networking.rb

@@ -4,7 +4,6 @@ module Facter
   module Resolvers
     module Windows
       class Networking < BaseResolver
-        @log = Facter::Log.new(self)
         init_resolver
 
         class << self

lib/facter/resolvers/windows/ssh.rb

@@ -4,8 +4,6 @@ module Facter
   module Resolvers
     module Windows
       class Ssh < BaseResolver
-        @log = Facter::Log.new(self)
-
         init_resolver
 
         FILE_NAMES = %w[ssh_host_rsa_key.pub ssh_host_dsa_key.pub

lib/facter/resolvers/windows/system32.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class System32 < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       class << self

lib/facter/resolvers/windows/timezone.rb

@@ -34,7 +34,6 @@ def codepage_from_api
             require_relative '../../../facter/resolvers/windows/ffi/winnls_ffi'
             WinnlsFFI.GetACP.to_s
           rescue LoadError => e
-            log = Facter::Log.new(self)
             log.debug("Could not retrieve codepage: #{e}")
           end
         end

lib/facter/resolvers/windows/uptime.rb

@@ -6,8 +6,6 @@ module Facter
   module Resolvers
     module Windows
       class Uptime < BaseResolver
-        @log = Facter::Log.new(self)
-
         init_resolver
 
         class << self

lib/facter/resolvers/windows/virtualization.rb

@@ -4,8 +4,6 @@ module Facter
   module Resolvers
     module Windows
       class Virtualization < BaseResolver
-        @log = Facter::Log.new(self)
-
         init_resolver
 
         class << self

lib/facter/resolvers/windows/win_os_description.rb

@@ -3,8 +3,6 @@
 module Facter
   module Resolvers
     class WinOsDescription < BaseResolver
-      @log = Facter::Log.new(self)
-
       init_resolver
 
       class << self

spec/custom_facts/core/aggregate_spec.rb

@@ -4,6 +4,7 @@
   subject(:aggregate_res) { Facter::Core::Aggregate.new('aggregated', fact) }
 
   let(:fact) { double('stub_fact', name: 'stub_fact') }
+  let(:logger) { Facter::Log.class_variable_get(:@@logger) }
 
   it 'can be resolved' do
     expect(aggregate_res).to be_a_kind_of LegacyFacter::Core::Resolvable
@@ -136,5 +137,11 @@
       expect(aggregate_res).to receive(:has_weight).with(5)
       aggregate_res.evaluate { has_weight(5) }
     end
+
+    it 'warns if the block is evaluated more than once' do
+      expect(logger).to receive(:warn).with(/Already evaluated aggregated at.*reevaluating anyways/)
+      aggregate_res.evaluate {}
+      aggregate_res.evaluate {}
+    end
   end
 end

spec/custom_facts/core/execution/fact_manager_spec.rb

@@ -134,17 +134,15 @@ def handy_method
       before do
         allow(Facter::Core::Execution::Popen3).to receive(:popen3e).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command)
                                                                    .and_return(['', 'some error'])
-        allow(Facter::Log).to receive(:new).with(executor).and_return(logger)
-        allow(Facter::Log).to receive(:new).with('foo').and_return(logger)
 
         allow(File).to receive(:executable?).with(command).and_return(true)
         allow(FileTest).to receive(:file?).with(command).and_return(true)
       end
 
-      it 'loggs warning messages on stderr' do
-        executor.execute(command)
+      it 'logs warning messages on stderr' do
+        executor.execute(command, logger: logger)
         expect(logger).to have_received(:debug).with('Command /bin/foo completed with '\
-                                                                            'the following stderr message: some error')
+          'the following stderr message: some error')
       end
     end
 

spec/custom_facts/core/execution/posix_spec.rb

@@ -107,7 +107,7 @@
   end
 
   context 'when calling execute_command' do
-    let(:logger) { instance_spy(Logger) }
+    let(:logger) { instance_spy(Facter::Log) }
 
     it 'executes a command' do
       expect(posix_executor.execute_command('/usr/bin/true', nil, logger)).to eq(['', ''])

spec/custom_facts/core/execution_spec.rb

@@ -50,7 +50,6 @@
     before do
       allow(impl).to receive(:which).with('waffles').and_return('/under/the/honey/are/the/waffles')
       allow(impl).to receive(:execute_command)
-      allow(logger).to receive(:warn)
     end
 
     context 'with default parameters' do
@@ -75,11 +74,20 @@
         end
       end
 
-      it 'emits a warning' do
+      it 'emits a warning to the default logger' do
+        expect(logger).to receive(:warn)
+          .with('Unexpected key passed to Facter::Core::Execution.execute option: time_limit,bad_opt' \
+            ' - valid keys: on_fail,expand,logger,timeout')
+
         execution.execute('waffles', time_limit: 90, bad_opt: true)
-        expect(logger).to have_received(:warn)
+      end
+
+      it 'ignores the passed in logger when logging the warning' do
+        expect(logger).to receive(:warn)
           .with('Unexpected key passed to Facter::Core::Execution.execute option: time_limit,bad_opt' \
-                ' - valid keys: on_fail,expand,logger,timeout')
+            ' - valid keys: on_fail,expand,logger,timeout')
+
+        execution.execute('waffles', time_limit: 90, bad_opt: true, logger: Facter::Log.new('ignored'))
       end
     end
   end