diff --git a/.travis.yml b/.travis.yml index 489846c..bfed563 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,12 +1,12 @@ language: ruby -sudo: false cache: bundler script: -- bundle exec rake spec + - bundle exec rake spec rvm: -- 1.9.3 -- 2.0.0 -- 2.1.5 + - "2.1" + - "2.4" + - "2.5" + - "2.6" matrix: fast_finish: true deploy: @@ -14,7 +14,7 @@ deploy: api_key: secure: "QpxxVQ8+0af/LVbFtZRYlJcY431xX7VTqPT7jjwO3x0PV3MLJcH+baGvoxAceAEbD0xgHPQGiFxSNhAZIwDBg9hW28ADP089HO0yxPxY/G7hBdLU9ZpSVe14p2V2M30SKcJ+vmNugK4oq26/ebe5NmoUCALClwyjZMNCXHr8OoQ=" on: - rvm: 2.1.5 + rvm: "2.6" tags: true all_branches: true notifications: diff --git a/Gemfile b/Gemfile index f7b5be0..5007e33 100644 --- a/Gemfile +++ b/Gemfile @@ -3,5 +3,5 @@ source 'https://rubygems.org' gemspec group :release do - gem 'github_changelog_generator', :require => false, :git => 'https://github.com/github-changelog-generator/github-changelog-generator' if RUBY_VERSION >= '2.2.2' + gem 'github_changelog_generator', :require => false end diff --git a/README.md b/README.md index e3359c5..1cb5b00 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,6 @@ puppet-lint-absolute_classname-check [![Gem Version](https://img.shields.io/gem/v/puppet-lint-absolute_classname-check.svg)](https://rubygems.org/gems/puppet-lint-absolute_classname-check) [![Gem Downloads](https://img.shields.io/gem/dt/puppet-lint-absolute_classname-check.svg)](https://rubygems.org/gems/puppet-lint-absolute_classname-check) [![Coverage Status](https://img.shields.io/coveralls/voxpupuli/puppet-lint-absolute_classname-check.svg)](https://coveralls.io/r/voxpupuli/puppet-lint-absolute_classname-check?branch=master) -[![Gemnasium](https://img.shields.io/gemnasium/voxpupuli/puppet-lint-absolute_classname-check.svg)](https://gemnasium.com/voxpupuli/puppet-lint-absolute_classname-check) [![Donated by Camptocamp](https://img.shields.io/badge/donated%20by-camptocamp-fb7047.svg)](#transfer-notice) A puppet-lint plugin to check that classes are included by their absolute name. @@ -39,26 +38,18 @@ gem 'puppet-lint-absolute_classname-check', :require => false ### Relative class name inclusion -Including a class by a relative name might lead to unexpected results [in Puppet 3](https://docs.puppet.com/puppet/3/lang_namespaces.html#relative-name-lookup-and-incorrect-name-resolution). +Including a class by a relative name might lead to unexpected results [in Puppet 3](https://docs.puppet.com/puppet/3/lang_namespaces.html#relative-name-lookup-and-incorrect-name-resolution). That's why a lot of manifests explicitly include by the absolute name. Since Puppet 4 names are always absolute and this is no longer needed. This lint check helps to clean up your manifests. #### What you have done ```puppet -include foobar +include ::foobar ``` #### What you should have done ```puppet -include ::foobar -``` - -#### Reverse this check - -This check can be reversed to check for Puppet > 4. - -```ruby -PuppetLint.configuration.absolute_classname_reverse = true +include foobar ``` #### Disabling the check diff --git a/lib/puppet-lint/plugins/check_absolute_classname.rb b/lib/puppet-lint/plugins/check_absolute_classname.rb index c546dea..ecd7cbf 100644 --- a/lib/puppet-lint/plugins/check_absolute_classname.rb +++ b/lib/puppet-lint/plugins/check_absolute_classname.rb @@ -1,14 +1,8 @@ PuppetLint.new_check(:relative_classname_inclusion) do def check + message = 'class included by absolute name (::$class)' + tokens.each_with_index do |token, token_idx| - reverse = PuppetLint.configuration.absolute_classname_reverse || false - if reverse - pattern = '^(?!::)' - message = 'class included by absolute name (::$class)' - else - pattern = '^::' - message = 'class included by relative name' - end if [:NAME,:FUNCTION_NAME].include?(token.type) && ['include','contain','require'].include?(token.value) s = token.next_code_token next if s.nil? @@ -22,7 +16,7 @@ def check in_function += 1 elsif in_function > 0 && n && n.type == :RPAREN in_function -= 1 - elsif in_function.zero? && s.value !~ /#{pattern}/ + elsif in_function.zero? && s.value.start_with?('::') notify :warning, { message: message, line: s.line, @@ -36,7 +30,7 @@ def check elsif token.type == :CLASS and token.next_code_token.type == :LBRACE s = token.next_code_token while s.type != :COLON - if (s.type == :NAME || s.type == :SSTRING) && s.value !~ /#{pattern}/ + if (s.type == :NAME || s.type == :SSTRING) && s.value.start_with?('::') notify :warning, { message: message, line: s.line, @@ -51,11 +45,6 @@ def check end def fix(problem) - reverse = PuppetLint.configuration.absolute_classname_reverse || false - problem[:token].value = if reverse - problem[:token].value[2..-1] - else - '::' + problem[:token].value - end + problem[:token].value = problem[:token].value[2..-1] end end diff --git a/puppet-lint-absolute_classname-check.gemspec b/puppet-lint-absolute_classname-check.gemspec index 590b9a1..c16aa8f 100644 --- a/puppet-lint-absolute_classname-check.gemspec +++ b/puppet-lint-absolute_classname-check.gemspec @@ -17,6 +17,8 @@ Gem::Specification.new do |spec| A puppet-lint plugin to check that classes are included by their absolute name. EOF + spec.required_ruby_version = '>= 2.1.0' + spec.add_dependency 'puppet-lint', '>= 1.0', '< 3.0' spec.add_development_dependency 'coveralls' spec.add_development_dependency 'mime-types' diff --git a/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb b/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb index 90bbd6d..0768f4c 100644 --- a/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb +++ b/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb @@ -1,419 +1,202 @@ require 'spec_helper' describe 'relative_classname_inclusion' do - describe '(default)' do - let(:msg) { 'class included by relative name' } - - context 'with fix disabled' do - context 'when absolute names are used' do - let(:code) do - <<-EOS - include ::foobar - include('::foobar') - include(foobar(baz)) - include(foobar('baz')) - - include ::foo, ::bar - include('::foo', '::bar') - - class { '::foobar': } - - class foobar { - } - - contain ::foobar - contain('::foobar') - contain(foobar(baz)) - contain(foobar('baz')) - - require ::foobar - require('::foobar') - require(foobar(baz)) - require(foobar('baz')) - EOS - end - - it 'should not detect any problems' do - expect(problems).to have(0).problems - end - end + let(:msg) { 'class included by absolute name (::$class)' } + + context 'with fix disabled' do + context 'when absolute names are used' do + let(:code) do + <<-EOS + include ::foobar + include('::foobar') + include(foobar(baz)) + include(foobar('baz')) + + include ::foo, ::bar + include('::foo', '::bar') + + class { '::foobar': } - context 'when relative names are used' do - let(:code) do - <<-EOS - include foobar - include(foobar) - class { 'foobar': } - contain foobar - contain(foobar) - require foobar - require(foobar) - EOS - end - - it 'should detect 7 problems' do - expect(problems).to have(7).problems - end - - it 'should create warnings' do - expect(problems).to contain_warning(msg).on_line(1).in_column(19) - expect(problems).to contain_warning(msg).on_line(2).in_column(19) - expect(problems).to contain_warning(msg).on_line(3).in_column(19) - expect(problems).to contain_warning(msg).on_line(4).in_column(19) - expect(problems).to contain_warning(msg).on_line(5).in_column(19) - expect(problems).to contain_warning(msg).on_line(6).in_column(19) - expect(problems).to contain_warning(msg).on_line(7).in_column(19) - end + class foobar { + } + + contain ::foobar + contain('::foobar') + contain(foobar(baz)) + contain(foobar('baz')) + + require ::foobar + require('::foobar') + require(foobar(baz)) + require(foobar('baz')) + EOS end - context 'when the require metadata parameter is used' do - let(:code) do - <<-EOS - file { '/path': - ensure => present, - require => Shellvar['http_proxy'], - } - EOS - end - - it 'should detect no problems' do - expect(problems).to have(0).problems - end + it 'should detect 11 problems' do + expect(problems).to have(11).problems end - context 'when require is a hash key' do - let(:code) do - <<-EOS - $defaults = { - require => Exec['apt_update'], - } - $defaults = { - 'require' => Exec['apt_update'], - } - EOS - end - - it 'should detect no problems' do - expect(problems).to have(0).problems - end + it 'should create warnings' do + expect(problems).to contain_warning(msg).on_line(1).in_column(17) + expect(problems).to contain_warning(msg).on_line(2).in_column(17) + expect(problems).to contain_warning(msg).on_line(6).in_column(17) + expect(problems).to contain_warning(msg).on_line(6).in_column(24) + expect(problems).to contain_warning(msg).on_line(7).in_column(17) + expect(problems).to contain_warning(msg).on_line(7).in_column(26) + expect(problems).to contain_warning(msg).on_line(9).in_column(17) + expect(problems).to contain_warning(msg).on_line(14).in_column(17) + expect(problems).to contain_warning(msg).on_line(15).in_column(17) + expect(problems).to contain_warning(msg).on_line(19).in_column(17) + expect(problems).to contain_warning(msg).on_line(20).in_column(17) end end - context 'with fix enabled' do - before do - PuppetLint.configuration.fix = true + context 'when relative names are used' do + let(:code) do + <<-EOS + include foobar + include(foobar) + class { 'foobar': } + contain foobar + contain(foobar) + require foobar + require(foobar) + EOS end - after do - PuppetLint.configuration.fix = false + it 'should not detect a problem' do + expect(problems).to have(0).problems end + end - context 'when absolute names are used' do - let(:code) do - <<-EOS - include ::foobar - include('::foobar') - include(foobar(baz)) - include(foobar('baz')) - - include ::foo, ::bar - include('::foo', '::bar') - - class { '::foobar': } - - class foobar { - } - - contain ::foobar - contain('::foobar') - contain(foobar(baz)) - contain(foobar('baz')) - - require ::foobar - require('::foobar') - require(foobar(baz)) - require(foobar('baz')) - EOS - end - - it 'should not detect any problems' do - expect(problems).to have(0).problems - end + context 'when the require metadata parameter is used' do + let(:code) do + <<-EOS + file { '/path': + ensure => present, + require => Shellvar['http_proxy'], + } + EOS end - context 'when relative names are used' do - let(:code) do - <<-EOS - include foobar - include(foobar) - class { 'foobar': } - contain foobar - contain(foobar) - require foobar - require(foobar) - EOS - end - - it 'should detect 7 problems' do - expect(problems).to have(7).problems - end - - it 'should fix the problems' do - expect(problems).to contain_fixed(msg).on_line(1).in_column(19) - expect(problems).to contain_fixed(msg).on_line(2).in_column(19) - expect(problems).to contain_fixed(msg).on_line(3).in_column(19) - expect(problems).to contain_fixed(msg).on_line(4).in_column(19) - expect(problems).to contain_fixed(msg).on_line(5).in_column(19) - expect(problems).to contain_fixed(msg).on_line(6).in_column(19) - expect(problems).to contain_fixed(msg).on_line(7).in_column(19) - end - - it 'should should add colons' do - expect(manifest).to eq( - <<-EOS - include ::foobar - include(::foobar) - class { '::foobar': } - contain ::foobar - contain(::foobar) - require ::foobar - require(::foobar) - EOS - ) - end + it 'should detect no problems' do + expect(problems).to have(0).problems end end - describe '(#12) behavior of lookup("foo", {merge => unique}).include' do - let(:msg) { '(#12) class included with lookup("foo", {merge => unique}).include' } - + context 'when require is a hash key' do let(:code) do <<-EOS - lookup(foo, {merge => unique}).include + $defaults = { + require => Exec['apt_update'], + } + $defaults = { + 'require' => Exec['apt_update'], + } EOS end - it 'should not detect any problems' do + it 'should detect no problems' do expect(problems).to have(0).problems end end end - describe '(reversed)' do + context 'with fix enabled' do before do - PuppetLint.configuration.absolute_classname_reverse = true + PuppetLint.configuration.fix = true end - let(:msg) { 'class included by absolute name (::$class)' } - - context 'with fix disabled' do - context 'when absolute names are used' do - let(:code) do - <<-EOS - include ::foobar - include('::foobar') - include(foobar(baz)) - include(foobar('baz')) - - include ::foo, ::bar - include('::foo', '::bar') - - class { '::foobar': } - - class foobar { - } - - contain ::foobar - contain('::foobar') - contain(foobar(baz)) - contain(foobar('baz')) - - require ::foobar - require('::foobar') - require(foobar(baz)) - require(foobar('baz')) - EOS - end - - it 'should detect 11 problems' do - expect(problems).to have(11).problems - end - - it 'should create warnings' do - expect(problems).to contain_warning(msg).on_line(1).in_column(19) - expect(problems).to contain_warning(msg).on_line(2).in_column(19) - expect(problems).to contain_warning(msg).on_line(6).in_column(19) - expect(problems).to contain_warning(msg).on_line(6).in_column(26) - expect(problems).to contain_warning(msg).on_line(7).in_column(19) - expect(problems).to contain_warning(msg).on_line(7).in_column(28) - expect(problems).to contain_warning(msg).on_line(9).in_column(19) - expect(problems).to contain_warning(msg).on_line(14).in_column(19) - expect(problems).to contain_warning(msg).on_line(15).in_column(19) - expect(problems).to contain_warning(msg).on_line(19).in_column(19) - expect(problems).to contain_warning(msg).on_line(20).in_column(19) - end - end - context 'when relative names are used' do - let(:code) do - <<-EOS - include foobar - include(foobar) - class { 'foobar': } - contain foobar - contain(foobar) - require foobar - require(foobar) - EOS - end - - it 'should not detect a problem' do - expect(problems).to have(0).problems - end - end + after do + PuppetLint.configuration.fix = false + end - context 'when the require metadata parameter is used' do - let(:code) do - <<-EOS - file { '/path': - ensure => present, - require => Shellvar['http_proxy'], - } - EOS - end - - it 'should detect no problems' do - expect(problems).to have(0).problems - end - end + context 'when absolute names are used' do + let(:code) do + <<-EOS + include ::foobar + include('::foobar') + include(foobar(baz)) + include(foobar('baz')) - context 'when require is a hash key' do - let(:code) do - <<-EOS - $defaults = { - require => Exec['apt_update'], - } - $defaults = { - 'require' => Exec['apt_update'], - } - EOS - end - - it 'should detect no problems' do - expect(problems).to have(0).problems - end - end - end + include ::foo, ::bar + include('::foo', '::bar') + + class { '::foobar': } - context 'with fix enabled' do - before do - PuppetLint.configuration.fix = true + class foobar { + } + + contain ::foobar + contain('::foobar') + contain(foobar(baz)) + contain(foobar('baz')) + + require ::foobar + require('::foobar') + require(foobar(baz)) + require(foobar('baz')) + EOS end - after do - PuppetLint.configuration.fix = false + it 'should detect 11 problems' do + expect(problems).to have(11).problems end - context 'when absolute names are used' do - let(:code) do - <<-EOS - include ::foobar - include('::foobar') - include(foobar(baz)) - include(foobar('baz')) - - include ::foo, ::bar - include('::foo', '::bar') - - class { '::foobar': } - - class foobar { - } - - contain ::foobar - contain('::foobar') - contain(foobar(baz)) - contain(foobar('baz')) - - require ::foobar - require('::foobar') - require(foobar(baz)) - require(foobar('baz')) - EOS - end - - it 'should detect 11 problems' do - expect(problems).to have(11).problems - end - - it 'should fix the problems' do - expect(problems).to contain_fixed(msg).on_line(1).in_column(19) - expect(problems).to contain_fixed(msg).on_line(2).in_column(19) - expect(problems).to contain_fixed(msg).on_line(6).in_column(19) - expect(problems).to contain_fixed(msg).on_line(6).in_column(26) - expect(problems).to contain_fixed(msg).on_line(7).in_column(19) - expect(problems).to contain_fixed(msg).on_line(7).in_column(28) - expect(problems).to contain_fixed(msg).on_line(9).in_column(19) - expect(problems).to contain_fixed(msg).on_line(14).in_column(19) - expect(problems).to contain_fixed(msg).on_line(15).in_column(19) - expect(problems).to contain_fixed(msg).on_line(19).in_column(19) - expect(problems).to contain_fixed(msg).on_line(20).in_column(19) - end - - it 'should should remove colons' do - expect(manifest).to eq( - <<-EOS - include foobar - include('foobar') - include(foobar(baz)) - include(foobar('baz')) - - include foo, bar - include('foo', 'bar') - - class { 'foobar': } - - class foobar { - } - - contain foobar - contain('foobar') - contain(foobar(baz)) - contain(foobar('baz')) - - require foobar - require('foobar') - require(foobar(baz)) - require(foobar('baz')) - EOS - ) - end + it 'should fix the problems' do + expect(problems).to contain_fixed(msg).on_line(1).in_column(17) + expect(problems).to contain_fixed(msg).on_line(2).in_column(17) + expect(problems).to contain_fixed(msg).on_line(6).in_column(17) + expect(problems).to contain_fixed(msg).on_line(6).in_column(24) + expect(problems).to contain_fixed(msg).on_line(7).in_column(17) + expect(problems).to contain_fixed(msg).on_line(7).in_column(26) + expect(problems).to contain_fixed(msg).on_line(9).in_column(17) + expect(problems).to contain_fixed(msg).on_line(14).in_column(17) + expect(problems).to contain_fixed(msg).on_line(15).in_column(17) + expect(problems).to contain_fixed(msg).on_line(19).in_column(17) + expect(problems).to contain_fixed(msg).on_line(20).in_column(17) end - context 'when relative names are used' do - let(:code) do - <<-EOS - include foobar - include(foobar) - class { 'foobar': } - contain foobar - contain(foobar) - require foobar - require(foobar) - EOS - end - - it 'should not detect any problems' do - expect(problems).to have(0).problems - end + it 'should should remove colons' do + expect(manifest).to eq( + <<-EOS + include foobar + include('foobar') + include(foobar(baz)) + include(foobar('baz')) + + include foo, bar + include('foo', 'bar') + + class { 'foobar': } + + class foobar { + } + + contain foobar + contain('foobar') + contain(foobar(baz)) + contain(foobar('baz')) + + require foobar + require('foobar') + require(foobar(baz)) + require(foobar('baz')) + EOS + ) end end - describe '(#12) behavior of lookup("foo", {merge => unique}).include' do - let(:msg) { '(#12) class included with lookup("foo", {merge => unique}).include' } - + context 'when relative names are used' do let(:code) do <<-EOS - lookup(foo, {merge => unique}).include + include foobar + include(foobar) + class { 'foobar': } + contain foobar + contain(foobar) + require foobar + require(foobar) EOS end @@ -422,4 +205,18 @@ class { 'foobar': } end end end + + describe '(#12) behavior of lookup("foo", {merge => unique}).include' do + let(:msg) { '(#12) class included with lookup("foo", {merge => unique}).include' } + + let(:code) do + <<-EOS + lookup(foo, {merge => unique}).include + EOS + end + + it 'should not detect any problems' do + expect(problems).to have(0).problems + end + end end