From cba9b1ced095213b8560f6954cf72e345c647d9f Mon Sep 17 00:00:00 2001 From: Joshua Hoblitt Date: Thu, 27 Jan 2022 15:52:27 -0700 Subject: [PATCH 1/3] add rubocop support --- .github/workflows/test.yml | 2 +- .rubocop.yml | 6 ++++++ Gemfile | 2 ++ Rakefile | 3 +++ 4 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 .rubocop.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 436c56c6..40c51e00 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -25,6 +25,6 @@ jobs: env: BUNDLE_WITHOUT: release - name: Run tests - run: bundle exec rake spec + run: bundle exec rake rubocop spec - name: Build gem run: gem build *.gemspec diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 00000000..53ac1898 --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,6 @@ +--- +# Managed by modulesync - DO NOT EDIT +# https://voxpupuli.org/docs/updating-files-managed-with-modulesync/ + +inherit_gem: + voxpupuli-test: rubocop.yml diff --git a/Gemfile b/Gemfile index 84d56274..e0664c49 100644 --- a/Gemfile +++ b/Gemfile @@ -10,4 +10,6 @@ end group :development do gem 'github_changelog_generator', '>= 1.16.4' + # provides the rubocop config and an awesome circular dependency to boot! + gem 'voxpupuli-test', '~> 5.2' end diff --git a/Rakefile b/Rakefile index 7521fa70..f6a94e9a 100644 --- a/Rakefile +++ b/Rakefile @@ -1,4 +1,7 @@ require 'facterdb/version' +require 'rubocop/rake_task' + +RuboCop::RakeTask.new begin require 'rspec/core/rake_task' From 3f26fef29c427cb197bce1b7706982f32afc1348 Mon Sep 17 00:00:00 2001 From: Joshua Hoblitt Date: Thu, 27 Jan 2022 16:29:44 -0700 Subject: [PATCH 2/3] fix rubocop errors --- .rubocop.yml | 12 +++++-- Rakefile | 5 ++- bin/facterdb | 1 + lib/facterdb.rb | 49 ++++++++++++++++----------- lib/facterdb/bin.rb | 4 ++- lib/facterdb/version.rb | 2 ++ spec/facterdb_spec.rb | 74 +++++++++++++++++++++-------------------- spec/facts_spec.rb | 30 ++++++++--------- spec/spec_helper.rb | 7 +++- 9 files changed, 106 insertions(+), 78 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 53ac1898..a1e63aec 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,12 @@ --- -# Managed by modulesync - DO NOT EDIT -# https://voxpupuli.org/docs/updating-files-managed-with-modulesync/ - inherit_gem: voxpupuli-test: rubocop.yml + +Naming/MethodParameterName: + Enabled: true + MinNameLength: 1 +RSpec/MultipleMemoizedHelpers: + Enabled: true + Max: 6 +Style/OptionalBooleanParameter: + Enabled: false diff --git a/Rakefile b/Rakefile index f6a94e9a..98f58a67 100644 --- a/Rakefile +++ b/Rakefile @@ -152,4 +152,7 @@ task :table do end end -task default: 'spec' +task default: %w[ + rubocop + spec +] diff --git a/bin/facterdb b/bin/facterdb index e78e2063..cbcb2c36 100755 --- a/bin/facterdb +++ b/bin/facterdb @@ -1,4 +1,5 @@ #!/usr/bin/env ruby +# frozen_string_literal: true require 'facterdb/bin' diff --git a/lib/facterdb.rb b/lib/facterdb.rb index 45751cce..9d4d5da1 100644 --- a/lib/facterdb.rb +++ b/lib/facterdb.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'facter' require 'jgrep' @@ -34,9 +36,11 @@ def self.inject_source? def self.read_json_file(f) content = File.read(f) return content unless inject_source? + # Find the opening brace first_brace = content.index('{') return content if first_brace.nil? + # Inject source file information json_injection = "\"_facterdb_filename\": #{File.basename(f).to_json}, " json_injection += "\"_facterdb_path\": #{File.expand_path(f).to_json}, " @@ -47,9 +51,10 @@ def self.read_json_file(f) # @return [Array[String]] - list of all files found in the default facterdb facts path def self.default_fact_files return [] unless use_defaultdb? + proj_root = File.join(File.dirname(File.dirname(__FILE__))) facts_dir = File.expand_path(File.join(proj_root, 'facts')) - Dir.glob(File.join(facts_dir, "**", '*.facts')) + Dir.glob(File.join(facts_dir, '**', '*.facts')) end # @return [Array[String]] - list of all files found in the user supplied facterdb facts path @@ -57,6 +62,7 @@ def self.default_fact_files def self.external_fact_files(fact_paths = ENV['FACTERDB_SEARCH_PATHS']) fact_paths ||= '' return [] if fact_paths.empty? + paths = fact_paths.split(File::PATH_SEPARATOR).map do |fact_path| unless File.directory?(fact_path) warn("[FACTERDB] Ignoring external facts path #{fact_path} as it is not a directory") @@ -74,23 +80,25 @@ def self.facterdb_fact_files (external_fact_files + default_fact_files).uniq end - def self.get_os_facts(facter_version='*', filter=[]) + def self.get_os_facts(facter_version = '*', filter = []) if facter_version == '*' - if filter.is_a?(Array) - filter_str = filter.map { |f| f.map { |k,v | "#{k}=#{v}" }.join(' and ') }.join(' or ') - elsif filter.is_a?(Hash) - filter_str = filter.map { |k,v | "#{k}=#{v}" }.join(' and ') - elsif filter.is_a?(String) + case filter + when Array + filter_str = filter.map { |f| f.map { |k, v| "#{k}=#{v}" }.join(' and ') }.join(' or ') + when Hash + filter_str = filter.map { |k, v| "#{k}=#{v}" }.join(' and ') + when String filter_str = filter else raise 'filter must be either an Array a Hash or a String' end else - if filter.is_a?(Array) - filter_str = "facterversion=/^#{facter_version}/ and (#{filter.map { |f| f.map { |k,v | "#{k}=#{v}" }.join(' and ') }.join(' or ')})" - elsif filter.is_a?(Hash) - filter_str = "facterversion=/^#{facter_version}/ and (#{filter.map { |k,v | "#{k}=#{v}" }.join(' and ')})" - elsif filter.is_a?(String) + case filter + when Array + filter_str = "facterversion=/^#{facter_version}/ and (#{filter.map { |f| f.map { |k, v| "#{k}=#{v}" }.join(' and ') }.join(' or ')})" + when Hash + filter_str = "facterversion=/^#{facter_version}/ and (#{filter.map { |k, v| "#{k}=#{v}" }.join(' and ')})" + when String filter_str = "facterversion=/^#{facter_version}/ and (#{filter})" else raise 'filter must be either an Array a Hash or a String' @@ -104,12 +112,14 @@ def self.get_os_facts(facter_version='*', filter=[]) # @return [String] - the string filter # @param filter [Object] - The filter to convert to jgrep string - def self.generate_filter_str(filter=nil) + def self.generate_filter_str(filter = nil) case filter when ::Array - '(' + filter.map { |f| f.map { |k,v | "#{k}=#{v}" }.join(' and ') }.join(') or (') + ')' + # rubocop:disable Style/StringConcatenation + '(' + filter.map { |f| f.map { |k, v| "#{k}=#{v}" }.join(' and ') }.join(') or (') + ')' + # rubocop:enable Style/StringConcatenation when ::Hash - filter.map { |k,v | "#{k}=#{v}" }.join(' and ') + filter.map { |k, v| "#{k}=#{v}" }.join(' and ') when ::String filter when ::NilClass @@ -130,12 +140,11 @@ def self.valid_filters?(filters) # @return [Array] - array of hashes of facts # @param filter [Object] - The filter to convert to jgrep string - def self.get_facts(filter=nil, cache=true) - if cache && filter && filter == Thread.current[:facterdb_last_filter_seen] - return Thread.current[:facterdb_last_facts_seen] - end + def self.get_facts(filter = nil, cache = true) + return Thread.current[:facterdb_last_facts_seen] if cache && filter && filter == Thread.current[:facterdb_last_filter_seen] + filter_str = generate_filter_str(filter) - result = JGrep.jgrep(database, filter_str).map { |hash| Hash[hash.map{ |k, v| [k.to_sym, v] }] } + result = JGrep.jgrep(database, filter_str).map { |hash| hash.transform_keys(&:to_sym) } if cache Thread.current[:facterdb_last_filter_seen] = filter Thread.current[:facterdb_last_facts_seen] = result diff --git a/lib/facterdb/bin.rb b/lib/facterdb/bin.rb index 9d0b1bb6..71f90529 100644 --- a/lib/facterdb/bin.rb +++ b/lib/facterdb/bin.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'facterdb' require 'json' @@ -8,7 +10,7 @@ def initialize(args) end def run - puts JSON.pretty_generate(FacterDB::get_os_facts('*', @args[0])) + puts JSON.pretty_generate(FacterDB.get_os_facts('*', @args[0])) end end end diff --git a/lib/facterdb/version.rb b/lib/facterdb/version.rb index 4cf6d58e..55a77533 100644 --- a/lib/facterdb/version.rb +++ b/lib/facterdb/version.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module FacterDB module Version STRING = '1.12.2' diff --git a/spec/facterdb_spec.rb b/spec/facterdb_spec.rb index 0df68988..cf476f0d 100644 --- a/spec/facterdb_spec.rb +++ b/spec/facterdb_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe FacterDB do @@ -16,14 +18,14 @@ end let(:facts_24_path) { File.join(project_dir, 'facts', '2.4') } - let(:facts_24_count) { Dir.glob(File.join(facts_24_path,'*.facts')).count } + let(:facts_24_count) { Dir.glob(File.join(facts_24_path, '*.facts')).count } let(:facts_23_path) { File.join(project_dir, 'facts', '2.3') } - let(:facts_23_count) { Dir.glob(File.join(facts_23_path,'*.facts')).count } + let(:facts_23_count) { Dir.glob(File.join(facts_23_path, '*.facts')).count } describe '.use_defaultdb?' do subject { described_class.use_defaultdb? } - before(:each) do + before do ENV.delete('FACTERDB_SKIP_DEFAULTDB') end @@ -32,7 +34,7 @@ end context 'when FACTERDB_SKIP_DEFAULTDB environment variable is set' do - before(:each) do + before do ENV['FACTERDB_SKIP_DEFAULTDB'] = '1' end @@ -45,7 +47,7 @@ let(:non_directory_path) { File.join('this', 'is', 'not', 'a', 'directory') } - before(:each) do + before do ENV.delete('FACTERDB_SEARCH_PATHS') allow(File).to receive(:directory?).and_call_original allow(File).to receive(:directory?).with(non_directory_path).and_return(false) @@ -116,7 +118,7 @@ describe '.default_fact_files' do subject(:default_fact_files) { described_class.default_fact_files } - before(:each) do + before do ENV.delete('FACTERDB_SKIP_DEFAULTDB') end @@ -128,7 +130,7 @@ end context 'when FACTERDB_SKIP_DEFAULTDB environment variable is set' do - before(:each) do + before do ENV['FACTERDB_SKIP_DEFAULTDB'] = '1' end @@ -139,11 +141,11 @@ describe '.facterdb_fact_files' do subject(:facterdb_fact_files) { described_class.facterdb_fact_files } - before(:each) do + before do ENV.delete('FACTERDB_SKIP_DEFAULTDB') end - after(:each) do + after do ENV.delete('FACTERDB_SEARCH_PATHS') end @@ -154,7 +156,7 @@ end context 'with no loaded fact sets' do - before(:each) do + before do ENV['FACTERDB_SKIP_DEFAULTDB'] = '1' ENV.delete('FACTERDB_SEARCH_PATHS') end @@ -166,10 +168,10 @@ describe '.database' do subject(:database) { described_class.database } - before(:each) do + before do ENV.delete('FACTERDB_SKIP_DEFAULTDB') ENV.delete('FACTERDB_SEARCH_PATHS') - FacterDB.instance_variable_set(:@database, nil) + described_class.instance_variable_set(:@database, nil) end it 'returns a String' do @@ -183,7 +185,7 @@ context 'when FACTERDB_INJECT_SOURCE environment variable is set' do subject(:json_database) { JSON.parse(database) } - before(:each) do + before do ENV['FACTERDB_INJECT_SOURCE'] = '1' end @@ -198,16 +200,16 @@ end describe '.get_os_facts' do - subject(:result) { FacterDB.get_os_facts(facter_version, filter) } + subject(:result) { described_class.get_os_facts(facter_version, filter) } - before(:each) do + before do object = defined?(Warning) ? Warning : Kernel allow(object).to receive(:warn).and_call_original allow(object).to receive(:warn).with(a_string_matching(%r{`get_os_facts` is deprecated})) end context 'without parameters' do - subject(:result) { FacterDB.get_os_facts() } + subject(:result) { described_class.get_os_facts } include_examples 'returns a result' end @@ -216,13 +218,13 @@ let(:facter_version) { '*' } context 'with an Array filter' do - let(:filter) { [{ :osfamily => 'Debian' }] } + let(:filter) { [{ osfamily: 'Debian' }] } include_examples 'returns a result' end context 'with a Hash filter' do - let(:filter) { { :osfamily => 'Debian' } } + let(:filter) { { osfamily: 'Debian' } } include_examples 'returns a result' end @@ -247,19 +249,19 @@ shared_examples 'returns only the specified version' do it 'only includes fact sets for the specified version' do - expect(result).to all(include(:facterversion => match(%r{\A2\.4}))) + expect(result).to all(include(facterversion: match(%r{\A2\.4}))) end end context 'with an Array filter' do - let(:filter) { [{ :osfamily => 'Debian' }] } + let(:filter) { [{ osfamily: 'Debian' }] } include_examples 'returns a result' include_examples 'returns only the specified version' end context 'with a Hash filter' do - let(:filter) { { :osfamily => 'Debian' } } + let(:filter) { { osfamily: 'Debian' } } include_examples 'returns a result' include_examples 'returns only the specified version' @@ -284,65 +286,65 @@ describe '.valid_filters?' do it 'invalid and false' do - expect( FacterDB.valid_filters?('and')).to be_falsey + expect(described_class).not_to be_valid_filters('and') end it 'valid and true' do - expect( FacterDB.valid_filters?('foo')).to be_truthy + expect(described_class).to be_valid_filters('foo') end end describe '.generate_filter_str' do it 'invalid type' do - expect{FacterDB.generate_filter_str(3)}.to raise_error(FacterDB::Errors::InvalidFilter) + expect { described_class.generate_filter_str(3) }.to raise_error(FacterDB::Errors::InvalidFilter) end it 'with string' do - expect(FacterDB.generate_filter_str('foo')).to eq("foo") + expect(described_class.generate_filter_str('foo')).to eq('foo') end it 'with hash' do - expect(FacterDB.generate_filter_str({:osfamily => 'Debian'})).to eq("osfamily=Debian") + expect(described_class.generate_filter_str({ osfamily: 'Debian' })).to eq('osfamily=Debian') end it 'with Array' do - expect(FacterDB.generate_filter_str([:osfamily => 'Debian'])).to eq("(osfamily=Debian)") + expect(described_class.generate_filter_str([osfamily: 'Debian'])).to eq('(osfamily=Debian)') end it 'empty' do - expect(FacterDB.generate_filter_str('')).to eq('') + expect(described_class.generate_filter_str('')).to eq('') end it 'nil filter' do - expect(FacterDB.generate_filter_str(nil)).to eq('') + expect(described_class.generate_filter_str(nil)).to eq('') end end describe '.get_facts' do - subject(:result) { FacterDB.get_facts(filter) } + subject(:result) { described_class.get_facts(filter) } let(:filter) { nil } context 'without parameters' do - include_examples "returns a result" + include_examples 'returns a result' end context 'with an Array filter' do - let(:filter) { [:osfamily => 'Debian'] } + let(:filter) { [osfamily: 'Debian'] } - include_examples "returns a result" + include_examples 'returns a result' end context 'with a Hash filter' do - let(:filter) { { :osfamily => 'Debian' } } + let(:filter) { { osfamily: 'Debian' } } - include_examples "returns a result" + include_examples 'returns a result' end context 'with a String filter' do let(:filter) { 'osfamily=Debian' } - include_examples "returns a result" + include_examples 'returns a result' end context 'with a filter of an unsupported type' do diff --git a/spec/facts_spec.rb b/spec/facts_spec.rb index 7e414cc6..92d21c9e 100644 --- a/spec/facts_spec.rb +++ b/spec/facts_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' require 'digest' require 'pathname' @@ -14,11 +16,9 @@ RSpec::Matchers.define :be_valid_json do match do |actual| - content = File.open(actual, 'rb') { |file| file.read } - valid = false + content = File.open(actual, 'rb', &:read) begin - obj = JSON.parse(content) - valid = true + JSON.parse(content) rescue JSON::ParserError => e raise "Invalid JSON file #{actual}.\n#{e}" end @@ -32,7 +32,7 @@ RSpec::Matchers.define :have_facter_version do |expected_facter_version, filepath| match do |actual| # Simple but naive regex check - actual =~ /^#{expected_facter_version}($|\.)/ + actual =~ %r{^#{expected_facter_version}($|\.)} end failure_message do |actual| @@ -41,22 +41,22 @@ end describe 'Default Facts' do - before(:each) do + before do ENV['FACTERDB_SKIP_DEFAULTDB'] = nil FacterDB.instance_variable_set(:@database, nil) end describe 'fact files' do - it 'should not contain duplicate fact sets' do + it 'does not contain duplicate fact sets' do # Gather all of the default files and hash the content file_hashes = {} FacterDB.default_fact_files.each do |filepath| - file_hash = Digest::SHA256.hexdigest( File.open(filepath, 'rb') { |file| file.read } ) + file_hash = Digest::SHA256.hexdigest(File.binread(filepath)) file_hashes[file_hash] ||= [] file_hashes[file_hash] << filepath end - file_hashes.keys.each do |file_hash| + file_hashes.each_key do |file_hash| expect(file_hashes[file_hash]).to have_a_unique_hash end end @@ -67,7 +67,7 @@ relative_path = Pathname.new(filepath).relative_path_from(project_dir).to_s describe relative_path do subject(:content) do - JSON.parse(File.open(filepath, 'rb') { |file| file.read }) + JSON.parse(File.binread(filepath)) end it 'contains a valid JSON document' do @@ -88,9 +88,11 @@ expect(content['networking']['fqdn']).to eq('foo.example.com') end end + it 'contains the legacy ipaddress fact' do expect(content['ipaddress']).to not_be_nil.and not_be_empty end + it 'contains no facts from puppetlabs/stdlib' do expect(content['root_home']).to be_nil expect(content['service_provider']).to be_nil @@ -100,22 +102,18 @@ expect(content['pe_version']).to be_nil expect(content['package_provider']).to be_nil end + it 'contains no facts from puppet/systemd' do expect(content['systemd']).to be_nil expect(content['systemd_version']).to be_nil expect(content['systemd_internal_services']).to be_nil end + it 'contains a legacy hostname, domain and fqdn fact' do expect(content['hostname']).to eq('foo') expect(content['fqdn']).to eq('foo.example.com') expect(content['domain']).to eq('example.com') end - it 'contains the legacy osfamily fact' do - expect(content['osfamily']).to_not be_nil - end - it 'contains the legacy operatingsystem fact' do - expect(content['operatingsystem']).to_not be_nil - end end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9d2ee2bd..42b77242 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + if ENV['COVERAGE'] == 'yes' require 'coveralls' require 'simplecov' @@ -10,7 +12,10 @@ require 'rspec' require 'facterdb' -include FacterDB + +RSpec.configure do |config| + config.include FacterDB +end RSpec::Matchers.define_negated_matcher :not_be_nil, :be_nil RSpec::Matchers.define_negated_matcher :not_be_empty, :be_empty From 3b5ecf253247df085e7e9a5ec92985a85fb833ce Mon Sep 17 00:00:00 2001 From: Joshua Hoblitt Date: Fri, 28 Jan 2022 09:59:14 -0700 Subject: [PATCH 3/3] simplify #project_dir Per @ekohl --- spec/spec_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 42b77242..3ad29800 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -21,5 +21,5 @@ RSpec::Matchers.define_negated_matcher :not_be_empty, :be_empty def project_dir - File.dirname File.dirname(File.expand_path(__FILE__)) + File.dirname(__dir__) end