Optimize

.rubocop_todo.yml

@@ -90,6 +90,7 @@ Style/ClassAndModuleChildren:
 Style/ClassVars:
   Exclude:
     - 'lib/puppet-ghostbuster/puppetdb.rb'
+    - 'lib/puppet-ghostbuster/util.rb'
 
 # Offense count: 9
 # Configuration parameters: AllowedConstants.
@@ -98,6 +99,7 @@ Style/Documentation:
     - 'spec/**/*'
     - 'test/**/*'
     - 'lib/puppet-ghostbuster/puppetdb.rb'
+    - 'lib/puppet-ghostbuster/util.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_classes.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_defines.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_facts.rb'
@@ -114,6 +116,7 @@ Style/Documentation:
 Style/FrozenStringLiteralComment:
   Exclude:
     - 'lib/puppet-ghostbuster/puppetdb.rb'
+    - 'lib/puppet-ghostbuster/util.rb'
     - 'lib/puppet-ghostbuster/version.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_classes.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_defines.rb'
@@ -167,4 +170,4 @@ Style/ZeroLengthPredicate:
 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns.
 # URISchemes: http, https
 Layout/LineLength:
-  Max: 189
+  Max: 195

lib/puppet-ghostbuster/puppetdb.rb

@@ -40,19 +40,19 @@ def client
 
     def self.classes
       @@classes ||= client.request('',
-                                   'resources[title] { type = "Class" and nodes { deactivated is null } }').data.map do |r|
+                                   'resources[title] { type = "Class" and nodes { deactivated is null } group by title }').data.map do |r|
         r['title']
-      end.uniq
+      end
     end
 
     def classes
       self.class.classes
     end
 
     def self.resources
-      @@resources ||= client.request('', 'resources[type] { nodes { deactivated is null } }').data.map do |r|
+      @@resources ||= client.request('', 'resources[type] { nodes { deactivated is null } group by type }').data.map do |r|
         r['type']
-      end.uniq
+      end
     end
 
     def resources

lib/puppet-ghostbuster/util.rb

@@ -0,0 +1,19 @@
+class PuppetGhostbuster
+  class Util
+    class << self
+      def search_file(name, search)
+        return search_file_regexp(name, search) if search.is_a?(Regexp)
+
+        File.foreach(name) do |line|
+          return true if line.include?(search)
+        end
+      end
+
+      def search_file_regexp(name, search)
+        File.foreach(name) do |line|
+          return true if line.match?(search)
+        end
+      end
+    end
+  end
+end

lib/puppet-lint/plugins/check_ghostbuster_facts.rb

@@ -1,3 +1,5 @@
+require 'puppet-ghostbuster/util'
+
 class PuppetLint::Checks
   def load_data(path, content)
     lexer = PuppetLint::Lexer.new
@@ -25,29 +27,30 @@ def check
     m = path.match(%r{.*/([^/]+)/lib/facter/(.+)$})
     return if m.nil?
 
-    File.readlines(path).grep(/Facter.add\(["']([^"']+)["']\)/).each do |line|
-      fact_name = line.match(/Facter.add\(["']([^"']+)["']\)/).captures[0]
+    File.foreach(path) do |line|
+      if line =~ /Facter.add\(["':](?<fact>[^"'\)]+)["']?\)/
+        fact_name = Regexp.last_match(:fact)
 
-      found = false
+        found = false
 
-      manifests.each do |manifest|
-        found = true if File.readlines(manifest).grep(/\$\{?::#{fact_name}\}?/).size > 0
-        found = true if File.readlines(manifest).grep(/@#{fact_name}/).size > 0
-        break if found
-      end
+        manifests.each do |manifest|
+          found = true unless PuppetGhostbuster::Util.search_file(manifest, /(\$\{?::#{fact_name}\}?|@#{fact_name})/).nil?
+          break if found
+        end
 
-      templates.each do |template|
-        found = true if File.readlines(template).grep(/@#{fact_name}/).size > 0
-        break if found
-      end
+        templates.each do |template|
+          found = true unless PuppetGhostbuster::Util.search_file(template, /@#{fact_name}/).nil?
+          break if found
+        end
 
-      next if found
+        next if found
 
-      notify :warning, {
-        message: "Fact #{fact_name} seems unused",
-        line: 1,
-        column: 1,
-      }
+        notify :warning, {
+          message: "Fact #{fact_name} seems unused",
+          line: 1,
+          column: 1,
+        }
+      end
     end
   end
 end

lib/puppet-lint/plugins/check_ghostbuster_files.rb

@@ -1,4 +1,5 @@
 require 'puppet-ghostbuster/puppetdb'
+require 'puppet-ghostbuster/util'
 
 class PuppetLint::Checks
   def load_data(path, content)
@@ -42,9 +43,9 @@ def check
     end
 
     manifests.each do |manifest|
-      return if File.readlines(manifest).grep(%r{["']#{module_name}/#{file_name}["']}).size > 0
+      return if PuppetGhostbuster::Util.search_file(manifest, %r{["']#{module_name}/#{file_name}["']})
 
-      if (match = manifest.match(%r{.*/([^/]+)/manifests/.+$})) && (match.captures[0] == module_name) && (File.readlines(manifest).grep(%r{["']\$\{module_name\}/#{file_name}["']}).size > 0)
+      if (match = manifest.match(%r{.*/([^/]+)/manifests/.+$})) && (match.captures[0] == module_name) && PuppetGhostbuster::Util.search_file(manifest, %r{["']\$\{module_name\}/#{file_name}["']})
         return
       end
     end

lib/puppet-lint/plugins/check_ghostbuster_functions.rb

@@ -1,3 +1,5 @@
+require 'puppet-ghostbuster/util'
+
 class PuppetLint::Checks
   def load_data(path, content)
     lexer = PuppetLint::Lexer.new
@@ -28,12 +30,11 @@ def check
     function_name = m.captures[0]
 
     manifests.each do |manifest|
-      return if File.readlines(manifest).grep(/#{function_name}\(/).size > 0
+      return if PuppetGhostbuster::Util.search_file(manifest, "#{function_name}(")
     end
 
     templates.each do |template|
-      return if File.readlines(template).grep(/scope.function_#{function_name}\(/).size > 0
-      return if File.readlines(template).grep(/Puppet::Parser::Functions.function\(:#{function_name}/).size > 0
+      return if PuppetGhostbuster::Util.search_file(template, /(Puppet::Parser::Functions\.function\(:|scope\.function_)#{function_name}/)
     end
 
     notify :warning, {

lib/puppet-lint/plugins/check_ghostbuster_hiera_files.rb

@@ -13,13 +13,26 @@ def load_data(path, content)
 end
 
 PuppetLint.new_check(:ghostbuster_hiera_files) do
+  def hiera
+    @hiera ||= YAML.load_file(ENV['HIERA_YAML_PATH'] || '/etc/puppetlabs/code/hiera.yaml')
+  end
+
+  def default_datadir
+    hiera.dig('defaults', 'datadir') || 'hieradata'
+  end
+
+  def path_in_datadirs?(path)
+    @data_dirs ||= hiera['hierarchy'].map { |h| (h['datadir'] || default_datadir).chomp('/') }.uniq
+    path.match?(%(./#{Regexp.union(@data_dirs)}/))
+  end
+
   def regexprs
-    hiera_yaml_file = ENV['HIERA_YAML_PATH'] || '/etc/puppetlabs/code/hiera.yaml'
-    hiera = YAML.load_file(hiera_yaml_file)
     regs = {}
     hiera['hierarchy'].each do |hierarchy|
+      path_datadir = Regexp.escape((hierarchy['datadir'] || default_datadir).chomp('/'))
       ([*hierarchy['path']] + [*hierarchy['paths']]).each do |level|
-        regex = level.gsub(/%\{(::)?(trusted|server_facts|facts)\.[^\}]+\}/, '(.+)').gsub(/%\{[^\}]+\}/, '.+')
+        level = File.join(path_datadir, level)
+        regex = Regexp.new(level.gsub(/%\{(::)?(trusted|server_facts|facts)\.[^\}]+\}/, '(.+)').gsub(/%\{[^\}]+\}/, '.+'))
         facts = level.match(regex).captures.map { |f| f[/%{(::)?(trusted|server_facts|facts)\.(.+)}/, 3] }
         regs[regex] = facts
       end
@@ -28,14 +41,14 @@ def regexprs
   end
 
   def check
-    return if path.match(%r{./hieradata/.*\.yaml}).nil?
+    return unless path_in_datadirs? path
 
     puppetdb = PuppetGhostbuster::PuppetDB.new
-    _path = path.gsub('./hieradata/', '')
 
     regexprs.each do |k, v|
-      m = _path.match(Regexp.new(k))
+      m = path.match(k)
       next if m.nil?
+
       return if m.captures.size == 0
 
       if m.captures.size > 1
@@ -68,7 +81,7 @@ def check
     end
 
     notify :warning, {
-      message: "Hiera File #{_path} seems unused",
+      message: "Hiera File #{path} seems unused",
       line: 1,
       column: 1,
     }

lib/puppet-lint/plugins/check_ghostbuster_templates.rb

@@ -1,3 +1,5 @@
+require 'puppet-ghostbuster/util'
+
 class PuppetLint::Checks
   def load_data(path, content)
     lexer = PuppetLint::Lexer.new
@@ -28,19 +30,17 @@ def check
     module_name, template_name = m.captures
 
     manifests.each do |manifest|
-      return if File.readlines(manifest).grep(%r{["']#{module_name}/#{template_name}["']}).size > 0
+      return if PuppetGhostbuster::Util.search_file(manifest, %r{["']#{module_name}/#{template_name}["']})
 
       next unless match = manifest.match(%r{.*/([^/]+)/manifests/.+$})
 
-      if match.captures[0] == module_name && (File.readlines(manifest).grep(%r{["']\$\{module_name\}/#{template_name}["']}).size > 0)
+      if match.captures[0] == module_name && PuppetGhostbuster::Util.search_file(manifest, %r{["']\$\{module_name\}/#{template_name}["']})
         return
       end
     end
 
     templates.each do |template|
-      if File.readlines(template).grep(%r{scope.function_template\(\['#{module_name}/#{template_name}'\]\)}).size > 0
-        return
-      end
+      return if PuppetGhostbuster::Util.search_file(template, "scope.function_template(['#{module_name}/#{template_name}'])")
     end
 
     notify :warning, {

spec/fixtures/hiera.yaml

@@ -1,6 +1,12 @@
 ---
 version: 5
+defaults:
+  datadir: data
 hierarchy:
+  - name: "Some other location"
+    datadir: private/
+    path: "nodes/%{trusted.certname}.eyaml"
+
   - name: "Per-node data (yaml version)"
     path: "nodes/%{trusted.certname}.yaml"
 

spec/fixtures/modules/foo/lib/facter/asym.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+# Fact is a symbol
+Facter.add(:asym) do
+  setcode do
+    'asym'
+  end
+end

spec/puppet-lint/plugins/ghostbuster_facts_spec.rb

@@ -53,5 +53,17 @@
         expect(problems.size).to eq(0)
       end
     end
+
+    context 'when added fact is a symbol and unused' do
+      let(:path) { './spec/fixtures/modules/foo/lib/facter/asym.rb' }
+
+      it 'detects one problem' do
+        expect(problems.size).to eq(1)
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning('Fact asym seems unused')
+      end
+    end
   end
 end

spec/puppet-lint/plugins/ghostbuster_hiera_files_spec.rb

@@ -9,71 +9,95 @@
 
   context 'with fix disabled' do
     context 'when a certname file is NOT used' do
-      let(:path) { './hieradata/nodes/foo.example.com.yaml' }
+      let(:path) { './data/nodes/foo.example.com.yaml' }
 
       it 'detects one problem' do
         expect(problems.size).to eq(1)
       end
 
       it 'creates a warning' do
-        expect(problems).to contain_warning('Hiera File nodes/foo.example.com.yaml seems unused')
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
       end
     end
 
     context 'when a certname file is used' do
-      let(:path) { './hieradata/nodes/bar.example.com.yaml' }
+      let(:path) { './data/nodes/bar.example.com.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
       end
     end
 
     context 'when an environment file is NOT used' do
-      let(:path) { './hieradata/environment/foo.yaml' }
+      let(:path) { './data/environment/foo.yaml' }
 
       it 'detects one problem' do
         expect(problems.size).to eq(1)
       end
 
       it 'creates a warning' do
-        expect(problems).to contain_warning('Hiera File environment/foo.yaml seems unused')
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
       end
     end
 
     context 'when an environment file is used' do
-      let(:path) { './hieradata/environment/production.yaml' }
+      let(:path) { './data/environment/production.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
       end
     end
 
     context 'when an fact is NOT used' do
-      let(:path) { './hieradata/virtual/false.yaml' }
+      let(:path) { './data/virtual/false.yaml' }
 
       it 'detects one problem' do
         expect(problems.size).to eq(1)
       end
 
       it 'creates a warning' do
-        expect(problems).to contain_warning('Hiera File virtual/false.yaml seems unused')
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
       end
     end
 
     context 'when an fact file is used' do
-      let(:path) { './hieradata/virtual/true.yaml' }
+      let(:path) { './data/virtual/true.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
       end
     end
 
+    context 'when an fact file is used with wrong extension' do
+      let(:path) { './data/virtual/true.eyaml' }
+
+      it 'detects one problem' do
+        expect(problems.size).to eq(1)
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
+      end
+    end
+
     context 'when using a variable in hierarchy' do
-      let(:path) { './hieradata/domain/example.com.yaml' }
+      let(:path) { './data/domain/example.com.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
       end
     end
+
+    context 'when hierarchy datadir is NOT default and NOT used' do
+      let(:path) { './private/nodes/privates.example.com.eyaml' }
+
+      it 'detects one problem' do
+        expect(problems.size).to eq(1)
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
+      end
+    end
   end
 end