From d69076553d67787945fe9f2f41636c0353cea0a2 Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Mon, 9 Nov 2020 19:03:51 +0100 Subject: [PATCH] Do not query for the exact facter version This assumes that no exact match can be found in most cases. This saves a call to FacterDB. Some testing on puppet-nginx this reduced the time for a dry run by about 1 to 2 seconds, from about ~11 to ~9 seconds. Implementation wise this switches to using Gem::Requirement to detect matching versions. This means you can also specify `~> 3.8` as a valid facterversion, or `>= 3 < 5`. --- lib/rspec-puppet-facts.rb | 73 ++++++++++----- spec/rspec_puppet_facts_spec.rb | 155 ++++++++++++++++++++++++-------- 2 files changed, 169 insertions(+), 59 deletions(-) diff --git a/lib/rspec-puppet-facts.rb b/lib/rspec-puppet-facts.rb index 2a37e1e1..9f706887 100644 --- a/lib/rspec-puppet-facts.rb +++ b/lib/rspec-puppet-facts.rb @@ -113,32 +113,31 @@ def on_supported_os_implementation(opts = {}) end end - facterversion_obj = Gem::Version.new(facterversion) + strict_requirement = RspecPuppetFacts::facter_version_to_strict_requirement(facterversion) + + loose_requirement = RspecPuppetFacts::facter_version_to_loose_requirement(facterversion) # FacterDB may have newer versions of facter data for which it contains a subset of all possible # facter data (see FacterDB 0.5.2 for Facter releases 3.8 and 3.9). In this situation we need to # cycle through and downgrade Facter versions per platform type until we find matching Facter data. filter.each do |filter_spec| - facter_version_filter = RspecPuppetFacts.facter_version_to_filter(facterversion) - db = FacterDB.get_facts(filter_spec.merge({ :facterversion => facter_version_filter })) - - if db.empty? - if RspecPuppetFacts.spec_facts_strict? - raise ArgumentError, "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, aborting" - end + versions = FacterDB.get_facts(filter_spec).map { |facts| Gem::Version.new(facts[:facterversion]) }.sort.reverse + next unless versions.any? - version = FacterDB.get_facts(filter_spec).map { |facts| Gem::Version.new(facts[:facterversion]) }.sort.reverse.detect { |v| v <= facterversion_obj } + version = versions.detect { |v| strict_requirement =~ v } + unless version + version = versions.detect { |v| loose_requirement =~ v } if loose_requirement next unless version - version = version.to_s - facter_version_filter = "/\\A#{Regexp.escape(version)}/" - unless version == facterversion - RspecPuppetFacts.warning "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, using v#{version} instead" + if RspecPuppetFacts.spec_facts_strict? + raise ArgumentError, "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, aborting" end + + RspecPuppetFacts.warning "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, using v#{version} instead" end - filter_spec[:facterversion] = facter_version_filter + filter_spec[:facterversion] = "/\\A#{Regexp.escape(version.to_s)}\\Z/" end received_facts = FacterDB::get_facts(filter) @@ -353,13 +352,47 @@ def self.reset @metadata = nil end - # Generates a JGrep statement expression for a specific facter version - # @return [String] JGrep statement expression - # @param version [String] the Facter version + # Construct the strict facter version requirement + # @return [Gem::Requirement] The version requirement to match # @api private - def self.facter_version_to_filter(version) - major, minor = version.split('.') - "/\\A#{major}\\.#{minor}\\./" + def self.facter_version_to_strict_requirement(version) + Gem::Requirement.new(facter_version_to_strict_requirement_string(version)) + end + + # Construct the strict facter version requirement string + # @return [String] The version requirement to match + # @api private + def self.facter_version_to_strict_requirement_string(version) + if /\A[0-9]+(\.[0-9]+)*\Z/.match?(version) + # Interpret 3 as ~> 3.0 + "~> #{version}.0" + else + version + end + end + + # Construct the loose facter version requirement + # @return [Optional[Gem::Requirement]] The version requirement to match + # @api private + def self.facter_version_to_loose_requirement(version) + string = facter_version_to_loose_requirement_string(version) + Gem::Requirement.new(string) if string + end + + # Construct the facter version requirement string + # @return [String] The version requirement to match + # @api private + def self.facter_version_to_loose_requirement_string(version) + if (m = /\A(?[0-9]+)\.(?[0-9]+)(?:\.(?[0-9]+))?\Z/.match(version)) + # Interpret 3.1 as < 3.2 and 3.2.1 as < 3.3 + "< #{m[:major]}.#{m[:minor].to_i + 1}" + elsif /\A[0-9]+\Z/.match?(version) + # Interpret 3 as < 4 + "< #{version.to_i + 1}" + else + # This would be the same as the strict requirement + nil + end end def self.facter_version_for_puppet_version(puppet_version) diff --git a/spec/rspec_puppet_facts_spec.rb b/spec/rspec_puppet_facts_spec.rb index 5a29a752..e1fbc483 100644 --- a/spec/rspec_puppet_facts_spec.rb +++ b/spec/rspec_puppet_facts_spec.rb @@ -689,7 +689,7 @@ end before do - RSpec.configuration.default_facter_version = '3.1.0' + RSpec.configuration.default_facter_version = '3.1.6' end after do @@ -721,11 +721,10 @@ end - it 'returns facts from a facter version matching future and below' do - major, minor = Facter.version.split('.') + it 'returns facts from a facter version matching version and below' do is_expected.to match( 'centos-7-x86_64' => include( - :facterversion => /\A#{major}\.[#{minor}#{minor.to_i + 1}]\./, + :facterversion => /\A2\.[0-6]\./, ), ) end @@ -856,8 +855,10 @@ before do allow(FacterDB).to receive(:get_facts).and_call_original allow(FacterDB).to receive(:get_facts).with( - a_hash_including(facterversion: "/\\A3\\.9\\./", operatingsystem: 'CentOS'), - ).and_return([]) + {:operatingsystem=>"CentOS", :operatingsystemrelease=>"/^7/", :hardwaremodel=>"x86_64"}, + ).and_wrap_original do |m, *args| + m.call(*args).reject { |facts| facts[:facterversion].start_with?('3.9.') } + end end it 'returns CentOS facts from a facter version matching 3.8' do @@ -976,69 +977,145 @@ module MCollectiveStub # rubocop:todo Lint/ConstantDefinitionInBlock end end - describe '.facter_version_to_filter' do + describe '.facter_version_to_strict_requirement' do + subject { described_class.facter_version_to_strict_requirement(version) } + + context 'when passed a version that is a complex requirement' do + let(:version) { '~> 2.4' } + + it { is_expected.to be_instance_of(Gem::Requirement) } + end + + context 'when passed a version that is major' do + let(:version) { '1' } + + it { is_expected.to be_instance_of(Gem::Requirement) } + end + end + + describe '.facter_version_to_strict_requirement_string' do + subject { described_class.facter_version_to_strict_requirement_string(version) } + + context 'when passed a version that is a complex requirement' do + let(:version) { '~> 2.4' } + + it { is_expected.to eq('~> 2.4') } + end + + context 'when passed a version that is major' do + let(:version) { '1' } + + it { is_expected.to eq('~> 1.0') } + end + + context 'when passed a version that is major.minor' do + let(:version) { '1.2' } + + it { is_expected.to eq('~> 1.2.0') } + end + + context 'when passed a version that is major.minor.patch' do + let(:version) { '1.2.3' } + + it { is_expected.to eq('~> 1.2.3.0') } + end + end + + describe '.facter_version_to_loose_requirement' do + subject { described_class.facter_version_to_loose_requirement(version) } + + context 'when passed a version that is a complex requirement' do + let(:version) { '~> 2.4' } + + it { is_expected.to be_nil } + end + + context 'when passed a version that is major' do + let(:version) { '1' } + + it { is_expected.to be_instance_of(Gem::Requirement) } + end + end + + describe '.facter_version_to_loose_requirement_string' do + subject { described_class.facter_version_to_loose_requirement_string(version) } + + context 'when passed a version that is a complex requirement (1)' do + let(:version) { '~> 2.4' } + + it { is_expected.to be_nil } + end + + context 'when passed a version that is a complex requirement (2)' do + let(:version) { '>= 3 < 5' } + + it { is_expected.to be_nil } + end + + context 'when passed a version that is major (1)' do + let(:version) { '1' } + + it { is_expected.to eq('< 2') } + end + + context 'when passed a version that is major (2)' do + let(:version) { '9' } + + it { is_expected.to eq('< 10') } + end + + context 'when passed a version that is major (3)' do + let(:version) { '10' } + + it { is_expected.to eq('< 11') } + end + context 'when passed a version that is major.minor (1)' do - subject { described_class.facter_version_to_filter('1.2') } + let(:version) { '1.2' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A1\.2\./') - end + it { is_expected.to eq('< 1.3') } end context 'when passed a version that is major.minor (2)' do - subject { described_class.facter_version_to_filter('10.2') } + let(:version) { '10.2' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A10\.2\./') - end + it { is_expected.to eq('< 10.3') } end context 'when passed a version that is major.minor (3)' do - subject { described_class.facter_version_to_filter('1.20') } + let(:version) { '1.20' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A1\.20\./') - end + it { is_expected.to eq('< 1.21') } end context 'when passed a version that is major.minor (4)' do - subject { described_class.facter_version_to_filter('10.20') } + let(:version) { '10.20' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A10\.20\./') - end + it { is_expected.to eq('< 10.21') } end context 'when passed a version that is major.minor.patch (1)' do - subject { described_class.facter_version_to_filter('1.2.3') } + let(:version) { '1.2.3' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A1\.2\./') - end + it { is_expected.to eq('< 1.3') } end context 'when passed a version that is major.minor.patch (2)' do - subject { described_class.facter_version_to_filter('10.2.3') } + let(:version) { '10.2.3' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A10\.2\./') - end + it { is_expected.to eq('< 10.3') } end context 'when passed a version that is major.minor.patch (3)' do - subject { described_class.facter_version_to_filter('1.20.3') } + let(:version) { '1.20.3' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A1\.20\./') - end + it { is_expected.to eq('< 1.21') } end context 'when passed a version that is major.minor.patch (4)' do - subject { described_class.facter_version_to_filter('10.20.3') } + let(:version) { '10.20.3' } - it 'returns the correct JGrep statement expression' do - is_expected.to eq('/\A10\.20\./') - end + it { is_expected.to eq('< 10.21') } end end end