Skip to content

Commit

Permalink
(FACT-3207) Don't rescue NoMethodError in base resolver
Browse files Browse the repository at this point in the history
This commit removes the logic to specifically rescue for just NoMethodError.
Instead, since NameError is a superclass of NoMethodError, NoMethodErrors will
be rescued when NameError is rescued and log at the error level instead of the
debug level. By doing this, bugs in resolvers are no longer hidden.

This commit also fixes various bugs in resolvers such as calling methods on nil
objects and requiring/relying gems that may not be installed to resolve facts.
  • Loading branch information
AriaXLi committed Jul 12, 2023
1 parent 18ef45d commit ed29723
Show file tree
Hide file tree
Showing 8 changed files with 12 additions and 12 deletions.
3 changes: 3 additions & 0 deletions lib/facter/resolvers/augeas.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ def read_augeas_version(fact_name)
@fact_list[:augeas_version] ||= read_augeas_from_gem

@fact_list[fact_name]
rescue LoadError => e
log.debug("Resolving fact #{fact_name}, but got #{e} at #{e.backtrace[0]}")
@fact_list[:augeas_version] = nil
end

def read_augeas_from_cli
Expand Down
5 changes: 1 addition & 4 deletions lib/facter/resolvers/base_resolver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,8 @@ def self.resolve(fact_name, options = {})

cache_nil_for_unresolved_facts(fact_name)
end
rescue NoMethodError => e
log.debug("Could not resolve #{fact_name}, got #{e} at #{e.backtrace[0]}")
@fact_list[fact_name] = nil
rescue LoadError, NameError => e
log.debug("Resolving fact #{fact_name}, but got #{e} at #{e.backtrace[0]}")
log.error("Resolving fact #{fact_name}, but got #{e} at #{e.backtrace[0]}")
@fact_list[fact_name] = nil
end

Expand Down
2 changes: 1 addition & 1 deletion lib/facter/resolvers/dmi.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def read_facts(fact_name)
return unless File.directory?('/sys/class/dmi')

file_content = Facter::Util::FileHelper.safe_read("/sys/class/dmi/id/#{fact_name}", nil)
.encode('UTF-8', invalid: :replace)
file_content = file_content.encode('UTF-8', invalid: :replace) if file_content
if files.include?(fact_name.to_s) && file_content
file_content = file_content.strip
@fact_list[fact_name] = file_content unless file_content.empty?
Expand Down
2 changes: 1 addition & 1 deletion lib/facter/resolvers/solaris/mountpoints.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def read_mounts(fact_name) # rubocop:disable Metrics/MethodLength
@mounts = []
@auto_home_paths = []

Facter::Util::Resolvers::FilesystemHelper.read_mountpoints.each do |fs|
Facter::Util::Resolvers::FilesystemHelper.read_mountpoints&.each do |fs|
if fs.name == 'auto_home'
@auto_home_paths << fs.mount_point
next
Expand Down
2 changes: 1 addition & 1 deletion lib/facter/resolvers/windows/timezone.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def determine_timezone

def codepage
result = codepage_from_api
result.empty? ? codepage_from_registry : result
result&.empty? ? codepage_from_registry : result
end

def codepage_from_registry
Expand Down
2 changes: 1 addition & 1 deletion lib/facter/util/resolvers/filesystem_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def compute_capacity(used, total)
private

def force_utf(mounts)
mounts.each do |mount|
mounts&.each do |mount|
mount.name.force_encoding('UTF-8')
mount.mount_type.force_encoding('UTF-8')
mount.mount_point.force_encoding('UTF-8')
Expand Down
2 changes: 1 addition & 1 deletion spec/facter/resolvers/augeas_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
allow(Facter::Resolvers::Augeas).to receive(:require).with('augeas').and_raise(exception)
end

it 'raises a LoadError error' do
it 'rescues LoadError error and logs at debug level' do
augeas.resolve(:augeas_version)

expect(log_spy).to have_received(:debug).with(/Resolving fact augeas_version, but got load_error_message/)
Expand Down
6 changes: 3 additions & 3 deletions spec/facter/resolvers/base_resolver_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,13 @@ def self.post_resolve(fact_name, _options)
context 'when Load Error is raised' do
before do
allow(resolver).to receive(:post_resolve).and_raise(LoadError)
allow(Facter::Log).to receive(:new).with(resolver).and_return(instance_double(Facter::Log, debug: nil))
allow(Facter::Log).to receive(:new).with(resolver).and_return(instance_double(Facter::Log, error: nil))
end

it 'logs the Load Error exception' do
it 'logs the Load Error exception at the error level' do
resolver.resolve(fact)

expect(resolver.log).to have_received(:debug).with(/Resolving fact #{fact}, but got LoadError/)
expect(resolver.log).to have_received(:error).with(/Resolving fact #{fact}, but got LoadError/)
end

it 'sets the fact to nil' do
Expand Down

0 comments on commit ed29723

Please sign in to comment.