Merge pull request #85 from puppetlabs/CONT-339-add_top_scope_facts_check

(CONT-339) Add top scope facts check

Author: jordanbreen28

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

@@ -0,0 +1,38 @@
+# Public: A puppet-lint plugin that will check for the use of top scope facts.
+# For example, the fact `$facts['kernel']` should be used over
+# `$::kernel`.
+#
+# The check only finds facts using the top-scope: ie it will find $::operatingsystem
+# but not $operatingsystem. It also all top scope variables are facts.
+# If you have top scope variables that aren't facts you should configure the
+# linter to ignore them.
+#
+# You can whitelist top scope variables to ignore via the Rake task.
+# You should insert the following line to your Rakefile.
+# `PuppetLint.configuration.top_scope_variables = ['location', 'role']`
+#
+# This plugin was adoped in to puppet-lint from https://github.com/mmckinst/puppet-lint-top_scope_facts-check
+# Thanks to @mmckinst and @seanmil for the original work.
+PuppetLint.new_check(:top_scope_facts) do
+  TOP_SCOPE_FACTS_VAR_TYPES = Set[:VARIABLE, :UNENC_VARIABLE]
+  def check
+    whitelist = ['trusted', 'facts'] + (PuppetLint.configuration.top_scope_variables || [])
+    whitelist = whitelist.join('|')
+    tokens.select { |x| TOP_SCOPE_FACTS_VAR_TYPES.include?(x.type) }.each do |token|
+      next unless %r{^::}.match?(token.value)
+      next if %r{^::(#{whitelist})\[?}.match?(token.value)
+      next if %r{^::[a-z0-9_][a-zA-Z0-9_]+::}.match?(token.value)
+
+      notify :warning, {
+        message: 'top scope fact instead of facts hash',
+        line: token.line,
+        column: token.column,
+        token: token,
+      }
+    end
+  end
+
+  def fix(problem)
+    problem[:token].value = "facts['" + problem[:token].value.sub(%r{^::}, '') + "']"
+  end
+end

spec/unit/puppet-lint/plugins/top_scope_facts/top_scope_facts_spec.rb

@@ -0,0 +1,194 @@
+require 'spec_helper'
+
+describe 'top_scope_facts' do
+  let(:msg) { 'top scope fact instead of facts hash' }
+
+  context 'with fix disabled' do
+    context 'fact variable using $facts hash' do
+      let(:code) { "$facts['operatingsystem']" }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+    context 'non-fact variable with two colons' do
+      let(:code) { '$foo::bar' }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'top scope $::facts hash' do
+      let(:code) { "$::facts['os']['family']" }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'top scope $::trusted hash' do
+      let(:code) { "$::trusted['certname']" }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'fact variable using top scope' do
+      let(:code) { '$::operatingsystem' }
+
+      it 'onlies detect a single problem' do
+        expect(problems).to have(1).problem
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning(msg).on_line(1).in_column(1)
+      end
+    end
+
+    context 'fact variable using top scope with curly braces in double quote' do
+      let(:code) { '"${::operatingsystem}"' }
+
+      it 'onlies detect a single problem' do
+        expect(problems).to have(1).problem
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning(msg).on_line(1).in_column(4)
+      end
+    end
+
+    context 'out of scope namespaced variable with leading ::' do
+      let(:code) { '$::profile::foo::bar' }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+
+      context 'inside double quotes' do
+        let(:code) { '"$::profile::foo::bar"' }
+
+        it 'does not detect any problems' do
+          expect(problems).to have(0).problem
+        end
+      end
+
+      context 'with curly braces in double quote' do
+        let(:code) { '"${::profile::foo::bar}"' }
+
+        it 'does not detect any problems' do
+          expect(problems).to have(0).problem
+        end
+      end
+    end
+  end
+
+  context 'with fix enabled' do
+    before(:each) do
+      PuppetLint.configuration.fix = true
+    end
+
+    after(:each) do
+      PuppetLint.configuration.fix = false
+    end
+
+    context 'fact variable using $facts hash' do
+      let(:code) { "$facts['operatingsystem']" }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'non-fact variable with two colons' do
+      let(:code) { '$foo::bar' }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'top scope $::facts hash' do
+      let(:code) { "$::facts['os']['family']" }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'top scope $::trusted hash' do
+      let(:code) { "$::trusted['certname']" }
+
+      it 'does not detect any problems' do
+        expect(problems).to have(0).problem
+      end
+    end
+
+    context 'fact variable using top scope' do
+      let(:code) { '$::operatingsystem' }
+
+      it 'onlies detect a single problem' do
+        expect(problems).to have(1).problem
+      end
+
+      it 'fixes the problem' do
+        expect(problems).to contain_fixed(msg).on_line(1).in_column(1)
+      end
+
+      it 'shoulds use the facts hash' do
+        expect(manifest).to eq("$facts['operatingsystem']")
+      end
+    end
+
+    context 'fact variable using top scope with curly braces in double quote' do
+      let(:code) { '"${::operatingsystem}"' }
+
+      it 'fixes the problem' do
+        expect(problems).to contain_fixed(msg).on_line(1).in_column(4)
+      end
+
+      it 'shoulds use the facts hash' do
+        expect(manifest).to eq('"${facts[\'operatingsystem\']}"')
+      end
+    end
+
+    context 'with custom top scope fact variables' do
+      before(:each) do
+        PuppetLint.configuration.top_scope_variables = ['location', 'role']
+      end
+
+      context 'fact variable using $facts hash' do
+        let(:code) { "$facts['operatingsystem']" }
+
+        it 'does not detect any problems' do
+          expect(problems).to have(0).problem
+        end
+      end
+
+      context 'fact variable using $trusted hash' do
+        let(:code) { "$trusted['certname']" }
+
+        it 'does not detect any problems' do
+          expect(problems).to have(0).problem
+        end
+      end
+
+      context 'whitelisted top scope variable $::location' do
+        let(:code) { '$::location' }
+
+        it 'does not detect any problems' do
+          expect(problems).to have(0).problem
+        end
+      end
+
+      context 'non-whitelisted top scope variable $::application' do
+        let(:code) { '$::application' }
+
+        it 'does not detect any problems' do
+          expect(problems).to have(1).problem
+        end
+      end
+    end
+  end
+end