Merge remote-tracking branch 'upstream/pr/3427'

* upstream/pr/3427:
  (maint) Enable rubocop checking for shadowed variables

.rubocop.yml

@@ -37,9 +37,8 @@ Lint/HandleExceptions:
 Lint/LiteralInCondition:
   Enabled: false
 
-# MAYBE useful - but too many instances
 Lint/ShadowingOuterLocalVariable:
-  Enabled: false
+  Enabled: true
 
 # Can catch complicated strings.
 Lint/LiteralInInterpolation:

ext/nagios/check_puppet.rb

@@ -16,7 +16,7 @@ class CheckPuppet
     :interval  => 30,
   }
 
-  o = OptionParser.new do |o|
+  OptionParser.new do |o|
     o.set_summary_indent('  ')
     o.banner =    "Usage: #{script_name} [OPTIONS]"
     o.define_head "The check_puppet Nagios plug-in checks that specified Puppet process is running and the state file is no older than specified interval."

lib/puppet/application/face_base.rb

@@ -121,9 +121,9 @@ def parse_options
 
     # Now we can interact with the default option code to build behaviour
     # around the full set of options we now know we support.
-    @action.options.each do |option|
-      option = @action.get_option(option) # make it the object.
-      self.class.option(*option.optparse) # ...and make the CLI parse it.
+    @action.options.each do |o|
+      o = @action.get_option(o) # make it the object.
+      self.class.option(*o.optparse) # ...and make the CLI parse it.
     end
 
     # ...and invoke our parent to parse all the command line options.

lib/puppet/external/nagios/base.rb

@@ -283,8 +283,8 @@ def to_ldif
     oc.sub!(/Nagios/,'nagios')
     oc.sub!(/::/,'')
     ocs.push oc
-    ocs.each { |oc|
-      str += "objectclass: #{oc}\n"
+    ocs.each { |objclass|
+      str += "objectclass: #{objclass}\n"
     }
     @parameters.each { |name,value|
       next if self.class.suppress.include?(name)

lib/puppet/file_bucket/dipper.rb

@@ -94,10 +94,9 @@ def restore(file, sum)
         end
         ::File.open(file, ::File::WRONLY|::File::TRUNC|::File::CREAT) { |of|
           of.binmode
-          source_stream = newcontents.stream do |source_stream|
+          newcontents.stream do |source_stream|
             FileUtils.copy_stream(source_stream, of)
           end
-          #of.print(newcontents)
         }
         ::File.chmod(changed, file) if changed
       else

lib/puppet/file_serving/mount/pluginfacts.rb

@@ -6,7 +6,7 @@
 class Puppet::FileServing::Mount::PluginFacts < Puppet::FileServing::Mount
   # Return an instance of the appropriate class.
   def find(relative_path, request)
-    return nil unless mod = request.environment.modules.find { |mod|  mod.pluginfact(relative_path) }
+    return nil unless mod = request.environment.modules.find { |m|  m.pluginfact(relative_path) }
 
     path = mod.pluginfact(relative_path)
 

lib/puppet/file_serving/mount/plugins.rb

@@ -6,7 +6,7 @@
 class Puppet::FileServing::Mount::Plugins < Puppet::FileServing::Mount
   # Return an instance of the appropriate class.
   def find(relative_path, request)
-    return nil unless mod = request.environment.modules.find { |mod|  mod.plugin(relative_path) }
+    return nil unless mod = request.environment.modules.find { |m|  m.plugin(relative_path) }
 
     path = mod.plugin(relative_path)
 

lib/puppet/graph/relationship_graph.rb

@@ -126,9 +126,9 @@ def traverse(options = {}, &block)
         if made_progress
           enqueue(*deferred_resources)
         else
-          deferred_resources.each do |resource|
-            overly_deferred_resource_handler.call(resource)
-            finish(resource)
+          deferred_resources.each do |res|
+            overly_deferred_resource_handler.call(res)
+            finish(res)
           end
         end
 

lib/puppet/indirector/catalog/static_compiler.rb

@@ -140,10 +140,10 @@ def get_child_resources(host, catalog, resource, file)
     source = resource[:source]
 
     # This is largely a copy of recurse_remote in File
-    total = file[:source].collect do |source|
-      next unless result = file.perform_recursion(source)
+    total = file[:source].collect do |src|
+      next unless result = file.perform_recursion(src)
       return if top = result.find { |r| r.relative_path == "." } and top.ftype != "directory"
-      result.each { |data| data.source = "#{source}/#{data.relative_path}" }
+      result.each { |data| data.source = "#{src}/#{data.relative_path}" }
       break result if result and ! result.empty? and sourceselect == :first
       result
     end.flatten.compact

lib/puppet/indirector/rest.rb

@@ -85,13 +85,13 @@ def find(request)
     uri, body = Puppet::Network::HTTP::API::V1.request_to_uri_and_body(request)
     uri_with_query_string = "#{uri}?#{body}"
 
-    response = do_request(request) do |request|
+    response = do_request(request) do |req|
       # WEBrick in Ruby 1.9.1 only supports up to 1024 character lines in an HTTP request
       # http://redmine.ruby-lang.org/issues/show/3991
       if "GET #{uri_with_query_string} HTTP/1.1\r\n".length > 1024
-        http_post(request, uri, body, headers)
+        http_post(req, uri, body, headers)
       else
-        http_get(request, uri_with_query_string, headers)
+        http_get(req, uri_with_query_string, headers)
       end
     end
 
@@ -119,8 +119,8 @@ def find(request)
   end
 
   def head(request)
-    response = do_request(request) do |request|
-      http_head(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), headers)
+    response = do_request(request) do |req|
+      http_head(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), headers)
     end
 
     if is_http_200?(response)
@@ -131,8 +131,8 @@ def head(request)
   end
 
   def search(request)
-    response = do_request(request) do |request|
-      http_get(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), headers)
+    response = do_request(request) do |req|
+      http_get(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), headers)
     end
 
     if is_http_200?(response)
@@ -146,8 +146,8 @@ def search(request)
   def destroy(request)
     raise ArgumentError, "DELETE does not accept options" unless request.options.empty?
 
-    response = do_request(request) do |request|
-      http_delete(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), headers)
+    response = do_request(request) do |req|
+      http_delete(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), headers)
     end
 
     if is_http_200?(response)
@@ -161,8 +161,8 @@ def destroy(request)
   def save(request)
     raise ArgumentError, "PUT does not accept options" unless request.options.empty?
 
-    response = do_request(request) do |request|
-      http_put(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), request.instance.render, headers.merge({ "Content-Type" => request.instance.mime }))
+    response = do_request(request) do |req|
+      http_put(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), req.instance.render, headers.merge({ "Content-Type" => req.instance.mime }))
     end
 
     if is_http_200?(response)
@@ -180,7 +180,7 @@ def save(request)
   # to request.do_request from here, thus if we change what we pass or how we
   # get it, we only need to change it here.
   def do_request(request)
-    request.do_request(self.class.srv_service, self.class.server, self.class.port) { |request| yield(request) }
+    request.do_request(self.class.srv_service, self.class.server, self.class.port) { |req| yield(req) }
   end
 
   def validate_key(request)

lib/puppet/interface/face_collection.rb

@@ -27,8 +27,8 @@ def self.get_action_for_face(name, action_name, version)
       # ...we need to search for it bound to an o{lder,ther} version.  Since
       # we load all actions when the face is first references, this will be in
       # memory in the known set of versions of the face.
-      (@faces[name].keys - [ :current ]).sort.reverse.each do |version|
-        break if action = @faces[name][version].get_action(action_name)
+      (@faces[name].keys - [ :current ]).sort.reverse.each do |vers|
+        break if action = @faces[name][vers].get_action(action_name)
       end
     end
 

lib/puppet/module_tool/applications/installer.rb

@@ -57,11 +57,11 @@ def run
         results = { :action => :install, :module_name => name, :module_version => version }
 
         begin
-          if mod = installed_modules[name]
+          if installed_module = installed_modules[name]
             unless forced?
-              if Semantic::VersionRange.parse(version).include? mod.version
+              if Semantic::VersionRange.parse(version).include? installed_module.version
                 results[:result] = :noop
-                results[:version] = mod.version
+                results[:version] = installed_module.version
                 return results
               else
                 changes = Checksummer.run(installed_modules[name].mod.path) rescue []
@@ -135,8 +135,8 @@ def run
           unless forced?
             # Check for module name conflicts.
             releases.each do |rel|
-              if mod = installed_modules_source.by_name[rel.name.split('-').last]
-                next if mod.has_metadata? && mod.forge_name.tr('/', '-') == rel.name
+              if installed_module = installed_modules_source.by_name[rel.name.split('-').last]
+                next if installed_module.has_metadata? && installed_module.forge_name.tr('/', '-') == rel.name
 
                 if rel.name != name
                   dependency = {
@@ -149,8 +149,8 @@ def run
                   :requested_module  => name,
                   :requested_version => options[:version] || 'latest',
                   :dependency        => dependency,
-                  :directory         => mod.path,
-                  :metadata          => mod.metadata
+                  :directory         => installed_module.path,
+                  :metadata          => installed_module.metadata
               end
             end
           end

lib/puppet/module_tool/applications/upgrader.rb

@@ -107,9 +107,9 @@ def installed_release.priority
             add_module_name_constraints_to_graph(graph)
           end
 
-          installed_modules.each do |mod, release|
-            mod = mod.tr('/', '-')
-            next if mod == name
+          installed_modules.each do |installed_module, release|
+            installed_module = installed_module.tr('/', '-')
+            next if installed_module == name
 
             version = release.version
 
@@ -118,7 +118,7 @@ def installed_release.priority
               # we'll place constraints on the graph for each installed
               # module, locking it to upgrades within the same major version.
               ">=#{version} #{version.major}.x".tap do |range|
-                graph.add_constraint('installed', mod, range) do |node|
+                graph.add_constraint('installed', installed_module, range) do |node|
                   Semantic::VersionRange.parse(range).include? node.version
                 end
               end
@@ -127,7 +127,7 @@ def installed_release.priority
                 dep_name = dep['name'].tr('/', '-')
 
                 dep['version_requirement'].tap do |range|
-                  graph.add_constraint("#{mod} constraint", dep_name, range) do |node|
+                  graph.add_constraint("#{installed_module} constraint", dep_name, range) do |node|
                     Semantic::VersionRange.parse(range).include? node.version
                   end
                 end

lib/puppet/module_tool/contents_description.rb

@@ -42,17 +42,17 @@ def data
           end
         end
 
-        type_names.each do |type_name|
-          if type = Puppet::Type.type(type_name.to_sym)
-            type_hash = {:name => type_name, :doc => type.doc}
+        type_names.each do |name|
+          if type = Puppet::Type.type(name.to_sym)
+            type_hash = {:name => name, :doc => type.doc}
             type_hash[:properties] = attr_doc(type, :property)
             type_hash[:parameters] = attr_doc(type, :param)
             if type.providers.size > 0
               type_hash[:providers] = provider_doc(type)
             end
             @data << type_hash
           else
-            Puppet.warning "Could not find/load type: #{type_name}"
+            Puppet.warning "Could not find/load type: #{name}"
           end
         end
       end

lib/puppet/module_tool/installed_modules.rb

@@ -69,11 +69,11 @@ def initialize(source, mod)
         super(source, name, version, {})
 
         if mod.dependencies
-          mod.dependencies.each do |dep|
-            results = Puppet::ModuleTool.parse_module_dependency(release, dep)
+          mod.dependencies.each do |dependency|
+            results = Puppet::ModuleTool.parse_module_dependency(release, dependency)
             dep_name, parsed_range, range = results
 
-            dep.tap do |dep|
+            dependency.tap do |dep|
               add_constraint('initialize', dep_name, range.to_s) do |node|
                 parsed_range === node.version
               end

lib/puppet/network/auth_config_parser.rb

@@ -40,8 +40,8 @@ def parse_rights
     # Verify each of the rights are valid.
     # We let the check raise an error, so that it can raise an error
     # pointing to the specific problem.
-    rights.each { |name, right|
-      right.valid?
+    rights.each { |name, r|
+      r.valid?
     }
     rights
   end

lib/puppet/network/format_handler.rb

@@ -77,16 +77,16 @@ def self.format_to_canonical_name(format)
   # @return [Puppet::Network::Format, nil] the most suitable format
   # @api private
   def self.most_suitable_format_for(accepted, supported)
-    format_name = accepted.collect do |accepted|
-      accepted.to_s.sub(/;q=.*$/, '')
-    end.collect do |accepted|
-      if accepted == ALL_MEDIA_TYPES
+    format_name = accepted.collect do |format|
+      format.to_s.sub(/;q=.*$/, '')
+    end.collect do |format|
+      if format == ALL_MEDIA_TYPES
         supported.first
       else
-        format_to_canonical_name_or_nil(accepted)
+        format_to_canonical_name_or_nil(format)
       end
-    end.find do |accepted|
-      supported.include?(accepted)
+    end.find do |format|
+      supported.include?(format)
     end
 
     if format_name

lib/puppet/network/http/handler.rb

@@ -57,7 +57,7 @@ def process(request, response)
     profiler = configure_profiler(request_headers, request_params)
 
     Puppet::Util::Profiler.profile("Processed request #{request_method} #{request_path}", [:http, request_method, request_path]) do
-      if route = @routes.find { |route| route.matches?(new_request) }
+      if route = @routes.find { |r| r.matches?(new_request) }
         route.process(new_request, new_response)
       else
         raise Puppet::Network::HTTP::Error::HTTPNotFoundError.new("No route for #{new_request.method} #{new_request.path}", HANDLER_NOT_FOUND)

lib/puppet/network/resolver.rb

@@ -31,8 +31,8 @@ def self.each_srv_record(domain, service_name = :puppet, &block)
       # for the specific service.
       each_srv_record(domain, :puppet, &block)
     else
-      each_priority(records) do |priority, records|
-        while next_rr = records.delete(find_weighted_server(records))
+      each_priority(records) do |priority, recs|
+        while next_rr = recs.delete(find_weighted_server(recs))
           Puppet.debug "Yielding next server of #{next_rr.target.to_s}:#{next_rr.port}"
           yield next_rr.target.to_s, next_rr.port
         end

lib/puppet/network/rights.rb

@@ -20,8 +20,8 @@ def is_request_forbidden_and_why?(method, path, params)
                        else
                          [method]
                        end
-    authorization_failure_exceptions = methods_to_check.map do |method|
-      is_forbidden_and_why?(path, params.merge({:method => method}))
+    authorization_failure_exceptions = methods_to_check.map do |m|
+      is_forbidden_and_why?(path, params.merge({:method => m}))
     end
     if authorization_failure_exceptions.include? nil
       # One of the methods we checked is ok, therefore this request is ok.

lib/puppet/parameter.rb

@@ -108,8 +108,8 @@ def doc
           @doc << "\n\n#{vals}"
         end
 
-        if f = self.required_features
-          @doc << "\n\nRequires features #{f.flatten.collect { |f| f.to_s }.join(" ")}."
+        if features = self.required_features
+          @doc << "\n\nRequires features #{features.flatten.collect { |f| f.to_s }.join(" ")}."
         end
         @addeddocvals = true
       end
@@ -542,7 +542,7 @@ def to_s
   #
   def self.format_value_for_display(value)
     if value.is_a? Array
-      formatted_values = value.collect {|value| format_value_for_display(value)}.join(', ')
+      formatted_values = value.collect {|v| format_value_for_display(v)}.join(', ')
       "[#{formatted_values}]"
     elsif value.is_a? Hash
       # Sorting the hash keys for display is largely for having stable

lib/puppet/pops/evaluator/access_operator.rb

@@ -89,11 +89,11 @@ def access_Array(o, scope, keys)
     when 0
       fail(Puppet::Pops::Issues::BAD_ARRAY_SLICE_ARITY, @semantic.left_expr, {:actual => keys.size})
     when 1
-      k = coerce_numeric(keys[0], @semantic.keys[0], scope)
-      unless k.is_a?(Integer)
-        bad_access_key_type(o, 0, k, Integer)
+      key = coerce_numeric(keys[0], @semantic.keys[0], scope)
+      unless key.is_a?(Integer)
+        bad_access_key_type(o, 0, key, Integer)
       end
-      o[k]
+      o[key]
     when 2
       # A slice [from, to] with support for -1 to mean start, or end respectively.
       k1 = coerce_numeric(keys[0], @semantic.keys[0], scope)