(CAT-2185) Extend puppet-lint to legacy_check yaml files

* extended legacy facts check to .yaml files
* add test to not detect modern facts
* add test for when no search text is found when searching yaml

Author: alex501212

lib/puppet-lint/bin.rb

@@ -66,7 +66,10 @@ def run
 
       path = path.gsub(File::ALT_SEPARATOR, File::SEPARATOR) if File::ALT_SEPARATOR
       path = if File.directory?(path)
-               Dir.glob("#{path}/**/*.pp")
+               Dir.glob([
+                          "#{path}/**/*.pp",
+                          "#{path}/**/*.{yaml,yml}",
+                        ])
              else
                @args
              end

lib/puppet-lint/checks.rb

@@ -6,6 +6,8 @@ class PuppetLint::Checks
   # Public: Get an Array of problem Hashes.
   attr_accessor :problems
 
+  YAML_COMPATIBLE_CHECKS = [:legacy_facts].freeze
+
   # Public: Initialise a new PuppetLint::Checks object.
   def initialize
     @problems = []
@@ -45,23 +47,31 @@ def load_data(path, content)
     end
   end
 
-  # Internal: Run the lint checks over the manifest code.
+  # Internal: Run the lint checks over the manifest or YAML code.
   #
   # fileinfo - The path to the file as passed to puppet-lint as a String.
-  # data     - The String manifest code to be checked.
+  # data     - The String manifest or YAML code to be checked.
   #
   # Returns an Array of problem Hashes.
   def run(fileinfo, data)
     load_data(fileinfo, data)
 
     checks_run = []
-    enabled_checks.each do |check|
-      klass = PuppetLint.configuration.check_object[check].new
-      # FIXME: shadowing #problems
-      problems = klass.run
-      checks_run << [klass, problems]
+    if File.extname(fileinfo).downcase.match?(%r{\.ya?ml$})
+      enabled_checks.select { |check| YAML_COMPATIBLE_CHECKS.include?(check) }.each do |check|
+        klass = PuppetLint.configuration.check_object[check].new
+        # FIXME: shadowing #problems
+        problems = klass.run
+        checks_run << [klass, problems]
+      end
+    else
+      enabled_checks.each do |check|
+        klass = PuppetLint.configuration.check_object[check].new
+        # FIXME: shadowing #problems
+        problems = klass.run
+        checks_run << [klass, problems]
+      end
     end
-
     checks_run.each do |klass, problems|
       if PuppetLint.configuration.fix
         @problems.concat(klass.fix_problems)

lib/puppet-lint/plugins/legacy_facts/legacy_facts.rb

@@ -1,3 +1,5 @@
+require 'yaml'
+
 # Public: A puppet-lint custom check to detect legacy facts.
 #
 # This check will optionally convert from legacy facts like $::operatingsystem
@@ -112,6 +114,51 @@
 
 PuppetLint.new_check(:legacy_facts) do
   def check
+    if File.extname(PuppetLint::Data.path).downcase.match?(%r{\.ya?ml$})
+      content = PuppetLint::Data.manifest_lines
+      yaml_content = content.join("\n")
+      data = YAML.safe_load(yaml_content, aliases: true, permitted_classes: [Symbol])
+      search_yaml(data)
+    else
+      check_puppet
+    end
+  end
+
+  def search_yaml(data, path = [])
+    case data
+    when Hash
+      data.each do |k, v|
+        search_value(k.to_s, path)
+        search_yaml(v, path + [k.to_s])
+      end
+    when Array
+      data.each_with_index { |v, i| search_yaml(v, path + [i]) }
+    when String
+      search_value(data, path)
+    end
+  end
+
+  def search_value(value, _path)
+    value.scan(%r{%{(?:(?:::?)?|facts\.)([a-zA-Z0-9_]+)(?!\.[a-zA-Z])}}) do |match|
+      base_fact = match[0].split('.').first
+      next unless EASY_FACTS.include?(base_fact) || UNCONVERTIBLE_FACTS.include?(base_fact) || base_fact.match(Regexp.union(REGEX_FACTS))
+
+      notify :warning, {
+        message: "legacy fact '#{base_fact}'",
+        line: find_line_for_content(value),
+        column: 1
+      }
+    end
+  end
+
+  def find_line_for_content(content)
+    PuppetLint::Data.manifest_lines.each_with_index do |line, index|
+      return index + 1 if line.include?(content)
+    end
+    1
+  end
+
+  def check_puppet
     tokens.select { |x| LEGACY_FACTS_VAR_TYPES.include?(x.type) }.each do |token|
       fact_name = ''
 

spec/unit/puppet-lint/checks_spec.rb

@@ -196,6 +196,60 @@
     end
   end
 
+  describe '#run_yaml' do
+    let(:fileinfo) { File.join('path', 'to', 'test.yaml') }
+    let(:data) do
+      <<~EOS
+        ---
+        version: 5
+        defaults:
+          datadir: data
+          data_hash: yaml_data
+        hierarchy:
+          - name: "LEGACY FACT PATH"
+            paths:
+              - "os/%{facts.hostname}.yaml"
+          - name: "osfamily/major release"
+            paths:
+              - "os/%{facts.os.name}/%{facts.os.release.major}.yaml"
+          - name: 'common'
+            path: 'common.yaml'
+      EOS
+    end
+    let(:enabled_checks) { [:legacy_facts] }
+
+    before(:each) do
+      allow(instance).to receive(:enabled_checks).and_return(enabled_checks) # rubocop: disable RSpec/SubjectStub
+      allow(File).to receive(:extname).with(fileinfo).and_return('.yaml')
+    end
+
+    it 'loads the yaml data' do
+      expect(instance).to receive(:load_data).with(fileinfo, data).and_call_original # rubocop: disable RSpec/SubjectStub
+      instance.run(fileinfo, data)
+    end
+
+    context 'when there are checks enabled' do
+      let(:enabled_checks) { [:legacy_facts] }
+      let(:enabled_check_classes) { enabled_checks.map { |r| PuppetLint.configuration.check_object[r] } }
+      let(:disabled_checks) { PuppetLint.configuration.checks - enabled_checks }
+      let(:disabled_check_classes) { disabled_checks.map { |r| PuppetLint.configuration.check_object[r] } }
+
+      it 'runs the enabled checks' do
+        expect(enabled_check_classes).to all(receive(:new).and_call_original)
+
+        instance.run(fileinfo, data)
+      end
+
+      it 'does not run the disabled checks' do
+        disabled_check_classes.each do |check_class|
+          expect(check_class).not_to receive(:new)
+        end
+
+        instance.run(fileinfo, data)
+      end
+    end
+  end
+
   describe '#enabled_checks' do
     subject(:enabled_checks) { instance.enabled_checks }
 

spec/unit/puppet-lint/plugins/legacy_facts/legacy_facts_spec.rb

@@ -129,6 +129,65 @@
         expect(problems.size).to eq(1)
       end
     end
+
+    context 'YAML file processing' do
+      before(:each) do
+        allow(File).to receive(:extname).and_return('.yaml')
+      end
+
+      context 'with YAML string containing legacy fact' do
+        let(:code) { 'some_key: "%{::osfamily}"' }
+
+        it 'detects a single problem' do
+          expect(problems.size).to eq(1)
+        end
+      end
+
+      context 'with YAML string not containing legacy fact' do
+        let(:code) { 'some_key: "%{facts.os.name}"' }
+
+        it 'does not detect any problems' do
+          expect(problems).to be_empty
+        end
+      end
+
+      context 'with YAML nested structure containing legacy fact' do
+        let(:code) { "nested:\n  value: \"%{::architecture}\"" }
+
+        it 'detects a single problem' do
+          expect(problems.size).to eq(1)
+        end
+      end
+
+      context 'with YAML array containing legacy facts' do
+        let(:code) do
+          [
+            'array:',
+            '  - "%{::processor0}"',
+            '  - "%{::ipaddress_eth0}"',
+          ].join("\n")
+        end
+
+        it 'detects multiple problems' do
+          expect(problems.size).to eq(2)
+        end
+      end
+
+      context 'with YAML alias containing legacy fact' do
+        let(:code) do
+          [
+            'template: &template',
+            '  fact: "%{::osfamily}"',
+            'instance:',
+            '  <<: *template',
+          ].join("\n")
+        end
+
+        it 'detects multiple instances' do
+          expect(problems.size).to eq(2)
+        end
+      end
+    end
   end
 
   context 'with fix enabled' do