Merge branch 'master' into FACT-2553

.github/PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,13 @@
+PLEASE LABEL YOUR PULL REQUEST ACCORDINGLY!
+
+Choose only one of the following:
+
+"backwards-incompatible" - use this label for PRs that breaks some old functionality
+
+"feature" - use this label for new added functionality
+
+"bugfix" - use this label for PRs that contain fixes
+
+"maintenance" - use this label for PRs that contain trivial changes (eg. changes in unit tests)
+
+Also please add "community" additional label if you're part of puppet community (special attention will be provided for those PRs).

.travis.yml

@@ -23,6 +23,7 @@ jobs:
         - bundle exec rubocop
         - bundle exec rubycritic --no-browser -f console
         - bundle exec rake spec
+        - bundle exec rake commits
 
     - rvm: 2.5
       script:

CHANGELOG.md

@@ -1,5 +1,15 @@
 
 
+## [4.0.17](https://github.com/puppetlabs/facter-ng/tree/4.0.17) (2020-04-21)
+
+[Full Changelog](https://github.com/puppetlabs/facter-ng/compare/4.0.16...4.0.17)
+
+### Fixed
+
+- \(FACT-2562\) Correctly load custom and external fact directories [\#458](https://github.com/puppetlabs/facter-ng/pull/458) ([IrimieBogdan](https://github.com/IrimieBogdan))
+
+
+
 ## [4.0.16](https://github.com/puppetlabs/facter-ng/tree/4.0.16) (2020-04-15)
 
 [Full Changelog](https://github.com/puppetlabs/facter-ng/compare/4.0.15...4.0.16)

Rakefile

@@ -8,7 +8,39 @@ Dir.glob(File.join('tasks/**/*.rake')).each { |file| load file }
 
 task default: :spec
 
+desc 'verify that commit messages match CONTRIBUTING.md requirements'
+task(:commits) do
+  # This rake task looks at the summary from every commit from this branch not
+  # in the branch targeted for a PR. This is accomplished by using the
+  # TRAVIS_COMMIT_RANGE environment variable, which is present in travis CI and
+  # populated with the range of commits the PR contains. If not available, this
+  # falls back to `master..HEAD` as a next best bet as `master` is unlikely to
+  # ever be absent.
+  commit_range = ENV['TRAVIS_COMMIT_RANGE'].nil? ? 'master..HEAD' : ENV['TRAVIS_COMMIT_RANGE'].sub(/\.\.\./, '..')
+  puts "Checking commits #{commit_range}"
+  `git log --no-merges --pretty=%s #{commit_range}`.each_line do |commit_summary|
+    # This regex tests for the currently supported commit summary tokens: maint, doc, gem, or fact-<number>.
+    # The exception tries to explain it in more full.
+    if /^\((maint|doc|docs|gem|fact-\d+)\)|revert/i.match(commit_summary).nil?
+      raise "\n\n\n\tThis commit summary didn't match CONTRIBUTING.md guidelines:\n" \
+        "\n\t\t#{commit_summary}\n" \
+        "\tThe commit summary (i.e. the first line of the commit message) should start with one of:\n"  \
+        "\t\t(FACT-<digits>) # this is most common and should be a ticket at tickets.puppet.com\n" \
+        "\t\t(docs)\n" \
+        "\t\t(docs)(DOCUMENT-<digits>)\n" \
+        "\t\t(maint)\n" \
+        "\t\t(gem)\n" \
+        "\n\tThis test for the commit summary is case-insensitive.\n\n\n"
+    else
+      puts commit_summary.to_s
+    end
+    puts '...passed'
+  end
+end
+
 def retrieve_from_keyboard
+  return unless ARGV =~ /changelog/
+
   puts "Please provide the next release tag:\n"
   next_version = $stdin.gets.chomp
   raise(ArgumentError, ' The string that you entered is invalid!') unless /[0-9]+\.[0-9]+\.[0-9]+/.match?(next_version)

VERSION

@@ -1 +1 @@
-4.0.16
+4.0.17

lib/facter.rb

@@ -131,6 +131,8 @@ def fact(user_query)
     # @api public
     def reset
       LegacyFacter.reset
+      Options[:custom_dir] = []
+      Options[:external_dir] = []
       LegacyFacter.search(*Options.custom_dir)
       LegacyFacter.search_external(Options.external_dir)
       nil
@@ -145,6 +147,7 @@ def reset
     #
     # @api public
     def search(*dirs)
+      Options[:custom_dir] += dirs
       LegacyFacter.search(*dirs)
     end
 
@@ -156,6 +159,7 @@ def search(*dirs)
     #
     # @api public
     def search_external(dirs)
+      Options[:external_dir] += dirs
       LegacyFacter.search_external(dirs)
     end
 
@@ -165,7 +169,7 @@ def search_external(dirs)
     #
     # @api public
     def search_external_path
-      LegacyFacter.search_external_path
+      Options.external_dir
     end
 
     # Returns the registered search directories for custom facts.
@@ -174,7 +178,7 @@ def search_external_path
     #
     # @api public
     def search_path
-      LegacyFacter.search_path
+      Options.custom_dir
     end
 
     # Gets a hash mapping fact names to their values
@@ -186,7 +190,6 @@ def search_path
     def to_hash
       log_blocked_facts
 
-      reset
       resolved_facts = Facter::FactManager.instance.resolve_facts
       Facter::SessionCache.invalidate_all_caches
       Facter::FactCollection.new.build_fact_collection!(resolved_facts)

lib/facts/solaris/zpool_featureflags.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Facts
+  module Solaris
+    class ZpoolFeatureflags
+      FACT_NAME = 'zpool_featureflags'
+
+      def call_the_resolver
+        fact_value = Facter::Resolvers::Solaris::ZPool.resolve(:zpool_featureflags)
+        Facter::ResolvedFact.new(FACT_NAME, fact_value)
+      end
+    end
+  end
+end

lib/framework/core/fact_loaders/external_fact_loader.rb

@@ -19,6 +19,7 @@ def all_facts
     # 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

lib/framework/logging/logger.rb

@@ -65,7 +65,7 @@ def self.set_format_for_legacy_logger
     end
 
     def debug(msg)
-      return unless Facter.debugging?
+      return unless debugging_active?
 
       if msg.nil? || msg.empty?
         invoker = caller(1..1).first.slice(/.*:\d+/)
@@ -94,5 +94,13 @@ def error(msg, colorize = false)
     def colorize(msg, color)
       "\e[#{color}m#{msg}\e[0m"
     end
+
+    private
+
+    def debugging_active?
+      return true unless Facter.respond_to?(:debugging?)
+
+      Facter.debugging?
+    end
   end
 end

lib/resolvers/solaris/zpool_resolver.rb

@@ -22,11 +22,13 @@ def zpool_fact(fact_name)
           def build_zpool_facts
             output, _status = Open3.capture2('zpool upgrade -v')
             features_list = output.scan(/^\s+(\d+)/).flatten
+            features_flags = output.scan(/^([a-z0-9_]+)[[:blank:]]*(\(read-only compatible\))?$/).map(&:first)
 
             return if features_list.empty?
 
             @fact_list[:zpool_featurenumbers] = features_list.join(',')
-            @fact_list[:zpool_version] = features_list.last
+            @fact_list[:zpool_featureflags] = features_flags.join(',')
+            @fact_list[:zpool_version] = features_flags.any? ? '5000' : features_list.last
           end
         end
       end

lib/util/file_helper.rb

@@ -4,19 +4,28 @@ module Facter
   module Util
     class FileHelper
       @log = Log.new(self)
+
       class << self
         DEBUG_MESSAGE = 'File at: %s is not accessible.'
 
-        def safe_read(path, result_if_not_readable = '')
+        def safe_read(path, default_return = '')
           return File.read(path) if File.readable?(path)
 
-          result_if_not_readable
+          log_failed_to_read(path)
+          default_return
         end
 
-        def safe_readlines(path, result_if_not_readable = [])
+        def safe_readlines(path, default_return = [])
           return File.readlines(path) if File.readable?(path)
 
-          result_if_not_readable
+          log_failed_to_read(path)
+          default_return
+        end
+
+        private
+
+        def log_failed_to_read(path)
+          @log.debug(DEBUG_MESSAGE % path)
         end
       end
     end

spec/facter/facter_spec.rb

@@ -285,8 +285,8 @@
   end
 
   describe '#search_path' do
-    it 'sends call to LegacyFacter' do
-      expect(LegacyFacter).to receive(:search_path).once
+    it 'sends call to Facter::Options' do
+      expect(Facter::Options).to receive(:custom_dir).once
       Facter.search_path
     end
   end
@@ -301,8 +301,8 @@
   end
 
   describe '#search_external_path' do
-    it 'sends call to LegacyFacter' do
-      expect(LegacyFacter).to receive(:search_external_path).once
+    it 'sends call to Facter::Options' do
+      expect(Facter::Options).to receive(:external_dir).once
       Facter.search_external_path
     end
   end

spec/facter/facts/solaris/zpool_featureflags_spec.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+describe Facts::Solaris::ZpoolFeatureflags do
+  describe '#call_the_resolver' do
+    subject(:fact) { Facts::Solaris::ZpoolFeatureflags.new }
+
+    let(:zpool_feature_flags) { 'async_destroy,empty_bpobj,lz4_compress,multi_vdev_crash_dump,spacemap_histogram' }
+
+    before do
+      allow(Facter::Resolvers::Solaris::ZPool).to \
+        receive(:resolve).with(:zpool_featureflags).and_return(zpool_feature_flags)
+    end
+
+    it 'calls Facter::Resolvers::Solaris::ZPool' do
+      fact.call_the_resolver
+      expect(Facter::Resolvers::Solaris::ZPool).to have_received(:resolve).with(:zpool_featureflags)
+    end
+
+    it 'returns the zpool_featureflags fact' do
+      expect(fact.call_the_resolver).to be_an_instance_of(Facter::ResolvedFact).and \
+        have_attributes(name: 'zpool_featureflags', value: zpool_feature_flags)
+    end
+  end
+end

spec/facter/resolvers/solaris/zpool_resolver_spec.rb

@@ -28,6 +28,25 @@
       expect(result).to eq('1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,' \
         '24,25,26,27,28,29,30,31,32,33,34')
     end
+
+    context 'when zpool has featureflags' do
+      let(:output) { load_fixture('zpool-with-featureflags').read }
+      let(:zpool_featureflags) do
+        'async_destroy,empty_bpobj,lz4_compress,multi_vdev_crash_dump,spacemap_histogram,enabled_txg,' \
+        'hole_birth,extensible_dataset,embedded_data,bookmarks,filesystem_limits,large_blocks,large_dnode,' \
+        'sha512,skein,device_removal,obsolete_counts,zpool_checkpoint,spacemap_v2'
+      end
+
+      it 'returns zpool version fact' do
+        result = Facter::Resolvers::Solaris::ZPool.resolve(:zpool_version)
+        expect(result).to eq('5000')
+      end
+
+      it 'returns zpool featureflags fact' do
+        result = Facter::Resolvers::Solaris::ZPool.resolve(:zpool_featureflags)
+        expect(result).to eq(zpool_featureflags)
+      end
+    end
   end
 
   context 'when zpool command is not found' do

spec/facter/resolvers/utils/ssh_helper_spec.rb

@@ -6,15 +6,14 @@
   describe '#create_ssh' do
     let(:fingerprint) { instance_spy(Facter::FingerPrint) }
     let(:key) { load_fixture('rsa_key').read.strip }
-    let(:ssh_object) { Facter::Ssh.new(fingerprint, 'ssh-rsa', key, 'rsa') }
 
     before do
       allow(Facter::FingerPrint).to receive(:new).and_return(fingerprint)
-      allow(Facter::Ssh).to receive(:new).and_return(ssh_object)
     end
 
     it 'returns a ssh object' do
-      expect(ssh_helper.create_ssh('ssh-rsa', key)).to eql(ssh_object)
+      expect(ssh_helper.create_ssh('ssh-rsa', key)).to be_an_instance_of(Facter::Ssh).and \
+        have_attributes(name: 'rsa', type: 'ssh-rsa', fingerprint: fingerprint)
     end
   end
 end

spec/facter/util/file_helper_spec.rb

@@ -3,15 +3,19 @@
 describe Facter::Util::FileHelper do
   subject(:file_helper) { Facter::Util::FileHelper }
 
-  let(:logger) { instance_spy(Facter::Log) }
   let(:path) { '/Users/admin/file.txt' }
   let(:content) { 'file content' }
-  let(:error_message) { 'File at: /Users/admin/file.txt is not accessible.' }
+  let(:error_message) { 'Facter::Util::FileHelper - File at: /Users/admin/file.txt is not accessible.' }
   let(:array_content) { ['line 1', 'line 2', 'line 3'] }
+  let(:multi_logger_double) { instance_spy(Facter::MultiLogger, level: :warn) }
 
   before do
-    file_helper.instance_variable_set(:@log, logger)
-    allow(logger).to receive(:debug).with(error_message)
+    Facter::Log.class_variable_set(:@@logger, multi_logger_double)
+    allow(Facter).to receive(:debugging?).and_return(true)
+  end
+
+  after do
+    Facter::Log.class_variable_set(:@@logger, Facter::MultiLogger.new([]))
   end
 
   shared_context 'when file is readable' do
@@ -53,7 +57,7 @@
       it "doesn't log anything" do
         file_helper.safe_read(path)
 
-        expect(logger).not_to have_received(:debug)
+        expect(multi_logger_double).not_to have_received(:debug)
       end
     end
 
@@ -80,11 +84,12 @@
         expect(File).not_to have_received(:read)
       end
 
-      # it 'logs a debug message' do
-      #   file_helper.safe_read(path)
-      #
-      #   expect(logger).to have_received(:debug).with(error_message)
-      # end
+      it 'logs a debug message' do
+        file_helper.safe_read(path)
+
+        expect(multi_logger_double).to have_received(:debug)
+          .with(error_message)
+      end
     end
   end
 
@@ -115,7 +120,7 @@
       it "doesn't log anything" do
         file_helper.safe_readlines(path)
 
-        expect(logger).not_to have_received(:debug)
+        expect(multi_logger_double).not_to have_received(:debug)
       end
     end
 
@@ -142,11 +147,11 @@
         expect(File).not_to have_received(:readlines)
       end
 
-      # it 'logs a debug message' do
-      #   file_helper.safe_read(path)
-      #
-      #   expect(logger).to have_received(:debug).with(error_message)
-      # end
+      it 'logs a debug message' do
+        file_helper.safe_read(path)
+
+        expect(multi_logger_double).to have_received(:debug).with(error_message)
+      end
     end
   end
 end