diff options
| -rw-r--r-- | .rubocop.yml | 508 | ||||
| -rw-r--r-- | .travis.yml | 9 | ||||
| -rw-r--r-- | Gemfile | 57 | ||||
| -rw-r--r-- | README.markdown | 56 | ||||
| -rw-r--r-- | Rakefile | 8 | ||||
| -rw-r--r-- | spec/acceptance/deprecation_spec.rb | 58 | ||||
| -rw-r--r-- | spec/acceptance/nodesets/centos-7-x64.yml | 2 |
7 files changed, 636 insertions, 62 deletions
diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 0000000..5aadd1b --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,508 @@ +require: rubocop-rspec +AllCops: + TargetRubyVersion: 1.9 + Include: + - ./**/*.rb + Exclude: + - vendor/**/* + - .vendor/**/* + - pkg/**/* + - spec/fixtures/**/* +Lint/ConditionPosition: + Enabled: True + +Lint/ElseLayout: + Enabled: True + +Lint/UnreachableCode: + Enabled: True + +Lint/UselessComparison: + Enabled: True + +Lint/EnsureReturn: + Enabled: True + +Lint/HandleExceptions: + Enabled: True + +Lint/LiteralInCondition: + Enabled: True + +Lint/ShadowingOuterLocalVariable: + Enabled: True + +Lint/LiteralInInterpolation: + Enabled: True + +Style/HashSyntax: + Enabled: True + +Style/RedundantReturn: + Enabled: True + +Lint/AmbiguousOperator: + Enabled: True + +Lint/AssignmentInCondition: + Enabled: True + +Style/SpaceBeforeComment: + Enabled: True + +Style/AndOr: + Enabled: True + +Style/RedundantSelf: + Enabled: True + +# Method length is not necessarily an indicator of code quality +Metrics/MethodLength: + Enabled: False + +# Module length is not necessarily an indicator of code quality +Metrics/ModuleLength: + Enabled: False + +Style/WhileUntilModifier: + Enabled: True + +Lint/AmbiguousRegexpLiteral: + Enabled: True + +Lint/Eval: + Enabled: True + +Lint/BlockAlignment: + Enabled: True + +Lint/DefEndAlignment: + Enabled: True + +Lint/EndAlignment: + Enabled: True + +Lint/DeprecatedClassMethods: + Enabled: True + +Lint/Loop: + Enabled: True + +Lint/ParenthesesAsGroupedExpression: + Enabled: True + +Lint/RescueException: + Enabled: True + +Lint/StringConversionInInterpolation: + Enabled: True + +Lint/UnusedBlockArgument: + Enabled: True + +Lint/UnusedMethodArgument: + Enabled: True + +Lint/UselessAccessModifier: + Enabled: True + +Lint/UselessAssignment: + Enabled: True + +Lint/Void: + Enabled: True + +Style/AccessModifierIndentation: + Enabled: True + +Style/AccessorMethodName: + Enabled: True + +Style/Alias: + Enabled: True + +Style/AlignArray: + Enabled: True + +Style/AlignHash: + Enabled: True + +Style/AlignParameters: + Enabled: True + +Metrics/BlockNesting: + Enabled: True + +Style/AsciiComments: + Enabled: True + +Style/Attr: + Enabled: True + +Style/BracesAroundHashParameters: + Enabled: True + +Style/CaseEquality: + Enabled: True + +Style/CaseIndentation: + Enabled: True + +Style/CharacterLiteral: + Enabled: True + +Style/ClassAndModuleCamelCase: + Enabled: True + +Style/ClassAndModuleChildren: + Enabled: False + +Style/ClassCheck: + Enabled: True + +# Class length is not necessarily an indicator of code quality +Metrics/ClassLength: + Enabled: False + +Style/ClassMethods: + Enabled: True + +Style/ClassVars: + Enabled: True + +Style/WhenThen: + Enabled: True + +Style/WordArray: + Enabled: True + +Style/UnneededPercentQ: + Enabled: True + +Style/Tab: + Enabled: True + +Style/SpaceBeforeSemicolon: + Enabled: True + +Style/TrailingBlankLines: + Enabled: True + +Style/SpaceInsideBlockBraces: + Enabled: True + +Style/SpaceInsideBrackets: + Enabled: True + +Style/SpaceInsideHashLiteralBraces: + Enabled: True + +Style/SpaceInsideParens: + Enabled: True + +Style/LeadingCommentSpace: + Enabled: True + +Style/SpaceBeforeFirstArg: + Enabled: True + +Style/SpaceAfterColon: + Enabled: True + +Style/SpaceAfterComma: + Enabled: True + +Style/SpaceAfterMethodName: + Enabled: True + +Style/SpaceAfterNot: + Enabled: True + +Style/SpaceAfterSemicolon: + Enabled: True + +Style/SpaceAroundEqualsInParameterDefault: + Enabled: True + +Style/SpaceAroundOperators: + Enabled: True + +Style/SpaceBeforeBlockBraces: + Enabled: True + +Style/SpaceBeforeComma: + Enabled: True + +Style/CollectionMethods: + Enabled: True + +Style/CommentIndentation: + Enabled: True + +Style/ColonMethodCall: + Enabled: True + +Style/CommentAnnotation: + Enabled: True + +# 'Complexity' is very relative +Metrics/CyclomaticComplexity: + Enabled: False + +Style/ConstantName: + Enabled: True + +Style/Documentation: + Enabled: False + +Style/DefWithParentheses: + Enabled: True + +Style/PreferredHashMethods: + Enabled: True + +Style/DotPosition: + EnforcedStyle: trailing + +Style/DoubleNegation: + Enabled: True + +Style/EachWithObject: + Enabled: True + +Style/EmptyLineBetweenDefs: + Enabled: True + +Style/IndentArray: + Enabled: True + +Style/IndentHash: + Enabled: True + +Style/IndentationConsistency: + Enabled: True + +Style/IndentationWidth: + Enabled: True + +Style/EmptyLines: + Enabled: True + +Style/EmptyLinesAroundAccessModifier: + Enabled: True + +Style/EmptyLiteral: + Enabled: True + +# Configuration parameters: AllowURI, URISchemes. +Metrics/LineLength: + Enabled: False + +Style/MethodCallParentheses: + Enabled: True + +Style/MethodDefParentheses: + Enabled: True + +Style/LineEndConcatenation: + Enabled: True + +Style/TrailingWhitespace: + Enabled: True + +Style/StringLiterals: + Enabled: True + +Style/TrailingCommaInArguments: + Enabled: True + +Style/TrailingCommaInLiteral: + Enabled: True + +Style/GlobalVars: + Enabled: True + +Style/GuardClause: + Enabled: True + +Style/IfUnlessModifier: + Enabled: True + +Style/MultilineIfThen: + Enabled: True + +Style/NegatedIf: + Enabled: True + +Style/NegatedWhile: + Enabled: True + +Style/Next: + Enabled: True + +Style/SingleLineBlockParams: + Enabled: True + +Style/SingleLineMethods: + Enabled: True + +Style/SpecialGlobalVars: + Enabled: True + +Style/TrivialAccessors: + Enabled: True + +Style/UnlessElse: + Enabled: True + +Style/VariableInterpolation: + Enabled: True + +Style/VariableName: + Enabled: True + +Style/WhileUntilDo: + Enabled: True + +Style/EvenOdd: + Enabled: True + +Style/FileName: + Enabled: True + +Style/For: + Enabled: True + +Style/Lambda: + Enabled: True + +Style/MethodName: + Enabled: True + +Style/MultilineTernaryOperator: + Enabled: True + +Style/NestedTernaryOperator: + Enabled: True + +Style/NilComparison: + Enabled: True + +Style/FormatString: + Enabled: True + +Style/MultilineBlockChain: + Enabled: True + +Style/Semicolon: + Enabled: True + +Style/SignalException: + Enabled: True + +Style/NonNilCheck: + Enabled: True + +Style/Not: + Enabled: True + +Style/NumericLiterals: + Enabled: True + +Style/OneLineConditional: + Enabled: True + +Style/OpMethod: + Enabled: True + +Style/ParenthesesAroundCondition: + Enabled: True + +Style/PercentLiteralDelimiters: + Enabled: True + +Style/PerlBackrefs: + Enabled: True + +Style/PredicateName: + Enabled: True + +Style/RedundantException: + Enabled: True + +Style/SelfAssignment: + Enabled: True + +Style/Proc: + Enabled: True + +Style/RaiseArgs: + Enabled: True + +Style/RedundantBegin: + Enabled: True + +Style/RescueModifier: + Enabled: True + +# based on https://github.com/voxpupuli/modulesync_config/issues/168 +Style/RegexpLiteral: + EnforcedStyle: percent_r + Enabled: True + +Lint/UnderscorePrefixedVariableName: + Enabled: True + +Metrics/ParameterLists: + Enabled: False + +Lint/RequireParentheses: + Enabled: True + +Style/SpaceBeforeFirstArg: + Enabled: True + +Style/ModuleFunction: + Enabled: True + +Lint/Debugger: + Enabled: True + +Style/IfWithSemicolon: + Enabled: True + +Style/Encoding: + Enabled: True + +Style/BlockDelimiters: + Enabled: True + +Style/MultilineBlockLayout: + Enabled: True + +# 'Complexity' is very relative +Metrics/AbcSize: + Enabled: False + +# 'Complexity' is very relative +Metrics/PerceivedComplexity: + Enabled: False + +Lint/UselessAssignment: + Enabled: True + +Style/ClosingParenthesisIndentation: + Enabled: False + +# RSpec + +# We don't use rspec in this way +RSpec/DescribeClass: + Enabled: False + +# Example length is not necessarily an indicator of code quality +RSpec/ExampleLength: + Enabled: False + +RSpec/NamedSubject: + Enabled: False diff --git a/.travis.yml b/.travis.yml index f631db0..4e549bf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,21 +7,24 @@ script: "bundle exec rake validate lint spec" matrix: fast_finish: true include: - - rvm: 2.1.6 + - rvm: 2.3.1 dist: trusty env: PUPPET_INSTALL_TYPE=agent BEAKER_debug=true BEAKER_set=docker/ubuntu-14.04 script: bundle exec rake beaker services: docker sudo: required - - rvm: 2.1.6 + - rvm: 2.3.1 dist: trusty env: PUPPET_INSTALL_TYPE=agent BEAKER_debug=true BEAKER_set=docker/centos-7 script: bundle exec rake beaker services: docker sudo: required + - rvm: 2.3.1 + bundler_args: --without system_tests + env: PUPPET_GEM_VERSION="~> 4.0" - rvm: 2.1.9 bundler_args: --without system_tests - env: PUPPET_GEM_VERSION="~> 4.0" STRICT_VARIABLES="yes" + env: PUPPET_GEM_VERSION="~> 4.0" - rvm: 2.1.5 bundler_args: --without system_tests env: PUPPET_GEM_VERSION="~> 3.0" FUTURE_PARSER="yes" @@ -2,41 +2,46 @@ source ENV['GEM_SOURCE'] || "https://rubygems.org" -def location_for(place, version = nil) - if place =~ /^(git[:@][^#]*)#(.*)/ - [version, { :git => $1, :branch => $2, :require => false}].compact - elsif place =~ /^file:\/\/(.*)/ - ['>= 0', { :path => File.expand_path($1), :require => false}] +def location_from_env(env, default_location = []) + if location = ENV[env] + if location =~ /^((?:git|https?)[:@][^#]*)#(.*)/ + [{ :git => $1, :branch => $2, :require => false }] + elsif location =~ /^file:\/\/(.*)/ + ['>= 0', { :path => File.expand_path($1), :require => false }] + else + [location, { :require => false }] + end else - [place, version, { :require => false}].compact + default_location end end group :development, :unit_tests do - gem 'metadata-json-lint', :require => false - gem 'puppet_facts', :require => false - gem 'puppet-blacksmith', '>= 3.4.0', :require => false - gem 'puppetlabs_spec_helper', :git => 'git://github.com/puppetlabs/puppetlabs_spec_helper.git', :ref => '157a7fd', :require => false - gem 'rspec-puppet', '>= 2.3.2', :require => false - gem 'simplecov', :require => false + gem 'metadata-json-lint' + gem 'puppet_facts' + gem 'puppet-blacksmith', '>= 3.4.0' + gem 'puppetlabs_spec_helper', '>= 1.2.1' + gem 'rspec-puppet', '>= 2.3.2' + gem 'rspec-puppet-facts' + gem 'simplecov' + gem 'parallel_tests' + gem 'rubocop', '0.41.2' if RUBY_VERSION < '2.0.0' + gem 'rubocop' if RUBY_VERSION >= '2.0.0' + gem 'rubocop-rspec', '~> 1.6' if RUBY_VERSION >= '2.3.0' + gem 'json_pure', '<= 2.0.1' if RUBY_VERSION < '2.0.0' end group :system_tests do - gem 'beaker-rspec', *location_for(ENV['BEAKER_RSPEC_VERSION'] || '>= 3.4') - gem 'beaker', *location_for(ENV['BEAKER_VERSION']) - gem 'serverspec', :require => false - gem 'beaker-puppet_install_helper', :require => false - gem 'master_manipulator', :require => false - gem 'beaker-hostgenerator', *location_for(ENV['BEAKER_HOSTGENERATOR_VERSION']) + gem 'beaker', *location_from_env('BEAKER_VERSION', []) if RUBY_VERSION >= '2.3.0' + gem 'beaker', *location_from_env('BEAKER_VERSION', ['< 3']) if RUBY_VERSION < '2.3.0' + gem 'beaker-rspec', *location_from_env('BEAKER_RSPEC_VERSION', ['>= 3.4']) + gem 'serverspec' + gem 'beaker-puppet_install_helper' + gem 'master_manipulator' + gem 'beaker-hostgenerator', *location_from_env('BEAKER_HOSTGENERATOR_VERSION', []) end -# json_pure 2.0.2 added a requirement on ruby >= 2. We pin to json_pure 2.0.1 -# if using ruby 1.x -gem 'json_pure', '<=2.0.1', :require => false if RUBY_VERSION =~ /^1\./ -# rubocop 0.42.0 requires ruby >=2 -gem 'rubocop', '0.41.2', :require => false if RUBY_VERSION =~ /^1\./ - -gem 'facter', *location_for(ENV['FACTER_GEM_VERSION']) -gem 'puppet', *location_for(ENV['PUPPET_GEM_VERSION']) +gem 'facter', *location_from_env('FACTER_GEM_VERSION') +gem 'puppet', *location_from_env('PUPPET_GEM_VERSION') if File.exists? "#{__FILE__}.local" eval(File.read("#{__FILE__}.local"), binding) diff --git a/README.markdown b/README.markdown index aee4e90..5130f64 100644 --- a/README.markdown +++ b/README.markdown @@ -1370,6 +1370,62 @@ The following values will fail, causing compilation to abort: #### `validate_legacy` +This function supports updating modules to upgrade from Puppet 3 style argument validation (using the stdlib `validate_*` functions) to Puppet 4 data types, without breaking the world. + +##### For module users + +You are probably here, because you're receiving loads of deprecation warnings about `validate_*` functions. If you're still on Puppet 3, use the options described in [`deprecation`](#deprecation) to silence the messages for now, and avoid upgrading to module versions which already have dropped Puppet 3 support. If you're already running Puppet 4 (shipped in Puppet Enterprise 2015.2 or later), please read on to understand the required steps to fix those issues. + +The `validate_*` functions were the only way on Puppet 3 to easily check the types of class and define arguments. Additionally some of the functions provided additional helpers like [validate_numeric](#validate_numeric), which would not only allow numbers, but also arrays of numbers. Puppet 4 now allows much better defined type checking using [data types](https://docs.puppet.com/puppet/latest/reference/lang_data.html), without such surprises. To avoid breaking the world for people depending on those surprises, `validate_legacy` provides a way to make those edge-cases visible, and assist people with getting onto the clearer Puppet 4 syntax. + +Depending on the current state of development of the modules you use and the data you feed them, you'll encounter different messages: + +* "Notice: Accepting previously invalid value for target type '<type>'": Nothing to worry, you're already using values allowed by the new type, that would have been invalid by the old validation function. This is informational only. +* "Warning: This method is deprecated, please use the stdlib validate_legacy function": The module you're using hasn't yet upgraded to validate_legacy, use the options from [deprecation()](#deprecation) to silence the warnings for now, or submit a patch with you module's developer. See below for details. +* "Warning: validate_legacy(<function>) expected <type> value, got <actual type>": Your code is passing a value to the module that was accepted by the Puppet v3 style validation, but that will not be accepted by the next version of the module. Most often this can be fixed by removing quotes from numbers, or booleans. +* "Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, validate_legacy(<function>) expected <type> value, got <actual type>": Your code is passing a value that is not acceptable to either the new, or the old style validation. + + + + +##### For module developers + +Many `validate_*` functions have surprising holes in their validation. For example, [validate_numeric](#validate_numeric) would not only allow numbers, but also arrays of numbers, or strings that look like numbers, without giving you any control over the specifics. Contrast that to Puppet 4 [data types](https://docs.puppet.com/puppet/latest/reference/lang_data.html) which allow you to choose between `Numeric`, `Array[Numeric]`, or `Optional[Numeric]`. To get from here to there, without leaving your users behind, the validate_legacy function will provide you with a tool to make this migration as painless as possible. + +To start out, for each parameter of your classes and defines, you'll have to decide on a new Puppet 4 data type to use. In most cases the new data type will allow a different set of values than the original `validate_*` function (which is the point of the whole exercise). The situation then looks like this: + +| | `validate_` pass | `validate_` fail | +| ------------ | ---------------- | ---------------- | +| matches type | pass | pass, notice | +| fails type | pass, deprecated | fail | + +The code after the validation will still have to deal with all possible values for now, but users of your code can now change their manifests to only pass values that will match the new type. + +To effect those checks, given a class like this: +~~~ +class example($value) { + validate_numeric($value) +~~~ +which should only accept numbers, the resulting validation code looks like this: +~~~ +class example( + Variant[Stdlib::Compat::Numeric, Numeric] $value +) { + validate_legacy(Numeric, 'validate_numeric', $value) +~~~ + +The type of `$value` is defined as `Variant[Stdlib::Compat::Numeric, Numeric]`, which will allow any `Numeric` (the new type), and all values previously accepted by validate_numeric (through `Stdlib::Compat::Numeric`). For each `validate_*` function in stdlib, there is a matching `Stdlib::Compat::*` type that will allow the appropriate set of values through. See the documentation in the `types/` directory in the stdlib source code for caveats. + +The call to `validate_legacy` will take care of triggering the correct log or fail message for you. It requires the new type, the previous validation function name, and all arguments to that function. + +If you were previously still supporting Puppet 3, this is a breaking change. You'll need to update your `metadata.json` to not support Puppet 3 anymore in the `requirements` section, and bump the major version of your module. This should still pass all previously existing tests your module has. Do not forget to add more tests for the newly possible values. This is also a good time to start calling [`deprecation()`](#deprecation) for all parameters you always wanted to get rid of, or add additional constraints on your parameters. + +After releasing this version, you can at any time release another breaking change release where you remove all compat types, and all calls to `validate_legacy`. At this time you can also go through your code and remove all leftovers dealing with the previously possible values. + +At all times note in the CHANGELOG and the README at what stage of the process your modules currently are. + +##### Reference + Validates a value against both a specified type and a deprecated validation function. Silently passes if both pass, errors if one validation passes and the other does not, fails if both validations return false. Arguments include the type to check the value against, the full name of the previous validation function, the value itself to be checked, and an unspecified amount of arguments needed for the previous validation function. @@ -2,17 +2,9 @@ require 'puppet_blacksmith/rake_tasks' require 'puppet-lint/tasks/puppet-lint' require 'puppetlabs_spec_helper/rake_tasks' -PuppetLint.configuration.fail_on_warnings = true PuppetLint.configuration.send('relative') -PuppetLint.configuration.send('disable_140chars') -PuppetLint.configuration.send('disable_class_inherits_from_params_class') PuppetLint.configuration.send('disable_documentation') PuppetLint.configuration.send('disable_single_quote_string_with_variables') -PuppetLint.configuration.ignore_paths = ["spec/**/*.pp", "pkg/**/*.pp", "bundle/**/*", "vendor/**/*", "types/**/*"] - -if Puppet.version.to_f < 4.0 - PuppetSyntax.exclude_paths << "types/**/*" -end desc 'Generate pooler nodesets' task :gen_nodeset do diff --git a/spec/acceptance/deprecation_spec.rb b/spec/acceptance/deprecation_spec.rb index d0d7fed..ea13700 100644 --- a/spec/acceptance/deprecation_spec.rb +++ b/spec/acceptance/deprecation_spec.rb @@ -1,19 +1,29 @@ #! /usr/bin/env ruby -S rspec require 'spec_helper_acceptance' -require 'shellwords' describe 'deprecation function' do - before :each do - FileUtils.rm_rf '/tmp/deprecation' + + if fact('operatingsystem') == 'windows' + test_file = 'C:/deprecation' + else + test_file = "/tmp/deprecation" + end + + # It seems that Windows needs everything to be on one line when using puppet apply -e, otherwise the manifests would be in an easier format + add_file_manifest = "\"deprecation('key', 'message') file { '#{test_file}': ensure => present, content => 'test', }\"" + remove_file_manifest = "file { '#{test_file}': ensure => absent }" + + before :all do + apply_manifest(remove_file_manifest) end context 'with --strict=error', if: get_puppet_version =~ /^4/ do before :all do - pp = <<-EOS - deprecation('key', 'message') - file { '/tmp/deprecation': ensure => present } - EOS - @result = on(default, puppet('apply', '--strict=error', '-e', Shellwords.shellescape(pp)), acceptable_exit_codes: (0...256)) + @result = on(default, puppet('apply', '--strict=error', '-e', add_file_manifest), acceptable_exit_codes: (0...256)) + end + + after :all do + apply_manifest(remove_file_manifest) end it "should return an error" do @@ -24,18 +34,18 @@ describe 'deprecation function' do expect(@result.stderr).to match(/deprecation. key. message/) end - describe file('/tmp/deprecation') do - it { is_expected.not_to exist } + describe file("#{test_file}") do + it { is_expected.not_to be_file } end end context 'with --strict=warning', if: get_puppet_version =~ /^4/ do before :all do - pp = <<-EOS - deprecation('key', 'message') - file { '/tmp/deprecation': ensure => present } - EOS - @result = on(default, puppet('apply', '--strict=warning', '-e', Shellwords.shellescape(pp)), acceptable_exit_codes: (0...256)) + @result = on(default, puppet('apply', '--strict=warning', '-e', add_file_manifest), acceptable_exit_codes: (0...256)) + end + + after :all do + apply_manifest(remove_file_manifest) end it "should not return an error" do @@ -46,18 +56,18 @@ describe 'deprecation function' do expect(@result.stderr).to match(/Warning: message/) end - describe file('/tmp/deprecation') do - it { is_expected.to exist } + describe file("#{test_file}") do + it { is_expected.to be_file } end end context 'with --strict=off', if: get_puppet_version =~ /^4/ do before :all do - pp = <<-EOS - deprecation('key', 'message') - file { '/tmp/deprecation': ensure => present } - EOS - @result = on(default, puppet('apply', '--strict=off', '-e', Shellwords.shellescape(pp)), acceptable_exit_codes: (0...256)) + @result = on(default, puppet('apply', '--strict=off', '-e', add_file_manifest), acceptable_exit_codes: (0...256)) + end + + after :all do + apply_manifest(remove_file_manifest) end it "should not return an error" do @@ -68,8 +78,8 @@ describe 'deprecation function' do expect(@result.stderr).not_to match(/Warning: message/) end - describe file('/tmp/deprecation') do - it { is_expected.to exist } + describe file("#{test_file}") do + it { is_expected.to be_file } end end end diff --git a/spec/acceptance/nodesets/centos-7-x64.yml b/spec/acceptance/nodesets/centos-7-x64.yml index 1a40c89..5eebdef 100644 --- a/spec/acceptance/nodesets/centos-7-x64.yml +++ b/spec/acceptance/nodesets/centos-7-x64.yml @@ -3,7 +3,7 @@ HOSTS: roles: - agent - default - platform: redhat-7-x86_64 + platform: el-7-x86_64 hypervisor: vagrant box: puppetlabs/centos-7.2-64-nocm CONFIG: |