Merge pull request #361 from highb/2020-03-05_update_rubocop_exceptions

Manual Rubocop Fixes

.rubocop.yml

@@ -80,3 +80,17 @@ Style/HashTransformKeys:
 # Added in 0.80, don't really care about the change
 Style/HashTransformValues:
   Enabled: false
+
+# These short variable names make sense as exceptions to the rule, but generally I think short variable names do hurt readability
+Naming/MethodParameterName:
+  AllowedNames:
+    - vm
+    - dc
+    - s
+    - x
+    - f
+
+# Standard comparisons seem more readable
+Style/NumericPredicate:
+  Enabled: true
+  EnforcedStyle: comparison

.travis.yml

@@ -16,9 +16,6 @@ matrix:
     - rvm: jruby-9.2.9.0
       env: "CHECK=test"
 
-  # Remove the allow_failures section once
-  # Rubocop is required for Travis to pass a build
-  allow_failures:
     - rvm: 2.4.9
       env: "CHECK=rubocop"
 

lib/vmpooler.rb

@@ -198,7 +198,7 @@ module Vmpooler
     obj.to_s.downcase == 'true'
   end
 
-  def self.set_linked_clone(parsed_config)
+  def self.set_linked_clone(parsed_config) # rubocop:disable Naming/AccessorMethodName
     parsed_config[:config]['create_linked_clones'] = parsed_config[:config]['create_linked_clones'] || true
     parsed_config[:config]['create_linked_clones'] = ENV['CREATE_LINKED_CLONES'] if ENV['CREATE_LINKED_CLONES'] =~ /true|false/
     parsed_config[:config]['create_linked_clones'] = true?(parsed_config[:config]['create_linked_clones']) if parsed_config[:config]['create_linked_clones']

lib/vmpooler/api/dashboard.rb

@@ -83,7 +83,7 @@ module Vmpooler
             history ||= {}
 
             begin
-              buffer = open(graph_link('.ready.*&from=-1hour&format=json')).read
+              buffer = URI.parse(graph_link('.ready.*&from=-1hour&format=json')).read
               history = JSON.parse(buffer)
 
               history.each do |pool|
@@ -136,7 +136,7 @@ module Vmpooler
         if params[:history]
           if graph_url
             begin
-              buffer = open(graph_link('.running.*&from=-1hour&format=json')).read
+              buffer = URI.parse(graph_link('.running.*&from=-1hour&format=json')).read
               JSON.parse(buffer).each do |pool|
                 if pool['target'] =~ /.*\.(.*)$/
                   pool['name'] = Regexp.last_match[1]

lib/vmpooler/api/helpers.rb

@@ -79,38 +79,38 @@ module Vmpooler
 
       def authenticate(auth, username_str, password_str)
         case auth['provider']
-          when 'dummy'
-            return (username_str != password_str)
-          when 'ldap'
-            ldap_base = auth[:ldap]['base']
-            ldap_port = auth[:ldap]['port'] || 389
+        when 'dummy'
+          return (username_str != password_str)
+        when 'ldap'
+          ldap_base = auth[:ldap]['base']
+          ldap_port = auth[:ldap]['port'] || 389
 
-            if ldap_base.is_a? Array
-              ldap_base.each do |search_base|
-                result = authenticate_ldap(
-                  ldap_port,
-                  auth[:ldap]['host'],
-                  auth[:ldap]['user_object'],
-                  search_base,
-                  username_str,
-                  password_str
-                )
-                return true if result == true
-              end
-            else
+          if ldap_base.is_a? Array
+            ldap_base.each do |search_base|
               result = authenticate_ldap(
                 ldap_port,
                 auth[:ldap]['host'],
                 auth[:ldap]['user_object'],
-                ldap_base,
+                search_base,
                 username_str,
                 password_str
               )
-              return result
+              return true if result == true
             end
-
-            return false
+          else
+            result = authenticate_ldap(
+              ldap_port,
+              auth[:ldap]['host'],
+              auth[:ldap]['user_object'],
+              ldap_base,
+              username_str,
+              password_str
+            )
+            return result
           end
+
+          return false
+        end
       end
 
       def export_tags(backend, hostname, tags)
@@ -216,7 +216,7 @@ module Vmpooler
         capacity[:current] = get_total_across_pools_redis_scard(pools, 'vmpooler__ready__', backend)
 
         if capacity[:total] > 0
-          capacity[:percent] = ((capacity[:current].to_f / capacity[:total].to_f) * 100.0).round(1)
+          capacity[:percent] = (capacity[:current].fdiv(capacity[:total]) * 100.0).round(1)
         end
 
         capacity

lib/vmpooler/api/v1.rb

@@ -225,7 +225,7 @@ module Vmpooler
 
       payload.each do |poolname, clone_target|
         unless pools[pool_index[poolname]]['clone_target'] == clone_target
-          pools[pool_index[poolname]]['clone_target'] == clone_target
+          pools[pool_index[poolname]]['clone_target'] = clone_target
           backend.hset('vmpooler__config__clone_target', poolname, clone_target)
           pools_updated += 1
           status 201
@@ -267,7 +267,7 @@ module Vmpooler
           if pool_index.include? poolname
             unless pools[pool_index[poolname]]['clone_target'] == clone_target
               pools[pool_index[poolname]]['clone_target'] == clone_target
-              end
+            end
           end
       end
     end

lib/vmpooler/logger.rb

@@ -16,7 +16,7 @@ module Vmpooler
 
       puts "[#{stamp}] #{string}" if ENV['VMPOOLER_DEBUG']
 
-      open(@file, 'a') do |f|
+      File.open(@file, 'a') do |f|
         f.puts "[#{stamp}] #{string}"
       end
     end

lib/vmpooler/pool_manager.rb

@@ -121,7 +121,7 @@ module Vmpooler
 
     def move_pending_vm_to_ready(vm, pool)
       clone_time = $redis.hget('vmpooler__vm__' + vm, 'clone')
-      finish = format('%.2f', Time.now - Time.parse(clone_time)) if clone_time
+      finish = format('%<time>.2f', time: Time.now - Time.parse(clone_time)) if clone_time
 
       $redis.smove('vmpooler__pending__' + pool, 'vmpooler__ready__' + pool, vm)
       $redis.hset('vmpooler__boot__' + Date.today.to_s, pool + ':' + vm, finish) # maybe remove as this is never used by vmpooler itself?
@@ -181,13 +181,13 @@ module Vmpooler
           end
         end
 
-        return if has_mismatched_hostname?(vm, pool_name, provider)
+        return if mismatched_hostname?(vm, pool_name, provider)
 
         vm_still_ready?(pool_name, vm, provider)
       end
     end
 
-    def has_mismatched_hostname?(vm, pool_name, provider)
+    def mismatched_hostname?(vm, pool_name, provider)
       pool_config = $config[:pools][$config[:pool_index][pool_name]]
       check_hostname = pool_config['check_hostname_for_mismatch']
       check_hostname = $config[:config]['check_ready_vm_hostname_for_mismatch'] if check_hostname.nil?
@@ -323,18 +323,18 @@ module Vmpooler
         $logger.log('d', "[ ] [#{pool_name}] Starting to clone '#{new_vmname}'")
         start = Time.now
         provider.create_vm(pool_name, new_vmname)
-        finish = format('%.2f', Time.now - start)
+        finish = format('%<time>.2f', time: Time.now - start)
 
         $redis.hset('vmpooler__clone__' + Date.today.to_s, pool_name + ':' + new_vmname, finish)
         $redis.hset('vmpooler__vm__' + new_vmname, 'clone_time', finish)
         $logger.log('s', "[+] [#{pool_name}] '#{new_vmname}' cloned in #{finish} seconds")
 
         $metrics.timing("clone.#{pool_name}", finish)
-      rescue StandardError => _e
+      rescue StandardError
         $redis.srem("vmpooler__pending__#{pool_name}", new_vmname)
         expiration_ttl = $config[:redis]['data_ttl'].to_i * 60 * 60
         $redis.expire("vmpooler__vm__#{new_vmname}", expiration_ttl)
-        raise _e
+        raise
       ensure
         $redis.decr('vmpooler__tasks__clone')
       end
@@ -369,7 +369,7 @@ module Vmpooler
 
         $redis.srem('vmpooler__completed__' + pool, vm)
 
-        finish = format('%.2f', Time.now - start)
+        finish = format('%<time>.2f', time: Time.now - start)
         $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds")
         $metrics.timing("destroy.#{pool}", finish)
         get_vm_usage_labels(vm)
@@ -503,7 +503,7 @@ module Vmpooler
 
       result = provider.create_disk(pool_name, vm_name, disk_size.to_i)
 
-      finish = format('%.2f', Time.now - start)
+      finish = format('%<time>.2f', time: Time.now - start)
 
       if result
         rdisks = $redis.hget('vmpooler__vm__' + vm_name, 'disk')
@@ -536,7 +536,7 @@ module Vmpooler
 
       result = provider.create_snapshot(pool_name, vm_name, snapshot_name)
 
-      finish = format('%.2f', Time.now - start)
+      finish = format('%<time>.2f', time: Time.now - start)
 
       if result
         $redis.hset('vmpooler__vm__' + vm_name, 'snapshot:' + snapshot_name, Time.now.to_s)
@@ -565,7 +565,7 @@ module Vmpooler
 
       result = provider.revert_snapshot(pool_name, vm_name, snapshot_name)
 
-      finish = format('%.2f', Time.now - start)
+      finish = format('%<time>.2f', time: Time.now - start)
 
       if result
         $logger.log('s', "[+] [snapshot_manager] '#{vm_name}' reverted to snapshot '#{snapshot_name}' in #{finish} seconds")
@@ -603,7 +603,7 @@ module Vmpooler
     # @param pool_name [String] - the name of the pool
     # @return [Provider] - returns the provider class Object
     def get_provider_for_pool(pool_name)
-      pool = $config[:pools].find { |pool| pool['name'] == pool_name }
+      pool = $config[:pools].find { |p| p['name'] == pool_name }
       return nil unless pool
 
       provider_name = pool.fetch('provider', nil)
@@ -619,7 +619,7 @@ module Vmpooler
           _check_disk_queue
           sleep(loop_delay)
 
-          unless maxloop.zero?
+          unless maxloop == 0
             break if loop_count >= maxloop
 
             loop_count += 1
@@ -655,7 +655,7 @@ module Vmpooler
           _check_snapshot_queue
           sleep(loop_delay)
 
-          unless maxloop.zero?
+          unless maxloop == 0
             break if loop_count >= maxloop
 
             loop_count += 1
@@ -816,7 +816,7 @@ module Vmpooler
             end
             sleep_with_wakeup_events(loop_delay, loop_delay_min, pool_size_change: true, poolname: pool['name'], pool_template_change: true, clone_target_change: true, pool_reset: true)
 
-            unless maxloop.zero?
+            unless maxloop == 0
               break if loop_count >= maxloop
 
               loop_count += 1
@@ -1119,8 +1119,8 @@ module Vmpooler
       $metrics.gauge("running.#{pool_name}", $redis.scard("vmpooler__running__#{pool_name}"))
 
       if $redis.get("vmpooler__empty__#{pool_name}")
-        $redis.del("vmpooler__empty__#{pool_name}") unless ready.zero?
-      elsif ready.zero?
+        $redis.del("vmpooler__empty__#{pool_name}") unless ready == 0
+      elsif ready == 0
         $redis.set("vmpooler__empty__#{pool_name}", 'true')
         $logger.log('s', "[!] [#{pool_name}] is empty")
       end
@@ -1153,7 +1153,7 @@ module Vmpooler
 
       begin
         inventory = create_inventory(pool, provider, pool_check_response)
-      rescue StandardError => e
+      rescue StandardError
         return(pool_check_response)
       end
 
@@ -1304,7 +1304,7 @@ module Vmpooler
 
         sleep(loop_delay)
 
-        unless maxloop.zero?
+        unless maxloop == 0
           break if loop_count >= maxloop
 
           loop_count += 1

lib/vmpooler/providers.rb

@@ -87,7 +87,7 @@ module Vmpooler
     # Returns an Array of Pathname objects.
     def gem_directories
       dirs = []
-      if has_rubygems?
+      if rubygems?
         dirs = gemspecs.map do |spec|
           lib_path = File.expand_path(File.join(spec.full_gem_path, provider_path))
           lib_path if File.exist? lib_path
@@ -99,7 +99,7 @@ module Vmpooler
     # Internal: Check if RubyGems is loaded and available.
     #
     # Returns true if RubyGems is available, false if not.
-    def has_rubygems?
+    def rubygems?
       defined? ::Gem
     end
 

lib/vmpooler/providers/vsphere.rb

@@ -75,7 +75,7 @@ module Vmpooler
           vm_object.PowerOffVM_Task.wait_for_completion if vm_object.runtime&.powerState && vm_object.runtime.powerState == 'poweredOn'
           vm_object.Destroy_Task.wait_for_completion
 
-          finish = format('%.2f', Time.now - start)
+          finish = format('%<time>.2f', time: Time.now - start)
           logger.log('s', "[-] [#{pool}] '#{vm_name}' destroyed in #{finish} seconds")
           metrics.timing("destroy.#{pool}", finish)
         rescue RuntimeError
@@ -132,11 +132,11 @@ module Vmpooler
         def get_folder_children(folder_name, connection)
           folders = []
 
-          propSpecs = {
+          propSpecs = { # rubocop:disable Naming/VariableName
             entity: self,
             inventoryPath: folder_name
           }
-          folder_object = connection.searchIndex.FindByInventoryPath(propSpecs)
+          folder_object = connection.searchIndex.FindByInventoryPath(propSpecs) # rubocop:disable Naming/VariableName
 
           return folders if folder_object.nil?
 
@@ -173,9 +173,9 @@ module Vmpooler
               target[dc]['checking'] = true
               hosts_hash = find_least_used_hosts(cluster, datacenter, percentage)
               target[dc] = hosts_hash
-            rescue StandardError => _e
+            rescue StandardError
               target[dc] = {}
-              raise(_e)
+              raise
             ensure
               target[dc]['check_time_finished'] = Time.now
             end
@@ -206,7 +206,7 @@ module Vmpooler
           loop_count = 1
           until target.key?(dc) && target[dc].key?('check_time_finished')
             sleep(loop_delay)
-            unless maxloop.zero?
+            unless maxloop == 0
               break if loop_count >= maxloop
 
               loop_count += 1
@@ -217,7 +217,7 @@ module Vmpooler
           loop_count = 1
           while Time.now - target[dc]['check_time_finished'] > max_age
             sleep(loop_delay)
-            unless maxloop.zero?
+            unless maxloop == 0
               break if loop_count >= maxloop
 
               loop_count += 1
@@ -242,7 +242,6 @@ module Vmpooler
                 target[dc]['hosts'].delete(host)
                 target[dc]['hosts'] << host
               end
-              return host
             else
               raise("there is no candidate in vcenter that meets all the required conditions, that the cluster has available hosts in a 'green' status, not in maintenance mode and not overloaded CPU and memory") unless target[dc].key?('hosts')
 
@@ -251,8 +250,9 @@ module Vmpooler
               target[dc]['architectures'].each do |arch|
                 target[dc]['architectures'][arch] = arch.partition { |v| v != host }.flatten if arch.include?(host)
               end
-              return host
             end
+
+            return host
           end
         end
 
@@ -344,11 +344,11 @@ module Vmpooler
             begin
               vm_target_folder = find_vm_folder(pool_name, connection)
               vm_target_folder = create_folder(connection, target_folder_path, target_datacenter_name) if vm_target_folder.nil? && @config[:config].key?('create_folders') && (@config[:config]['create_folders'] == true)
-            rescue StandardError => _e
+            rescue StandardError
               if @config[:config].key?('create_folders') && (@config[:config]['create_folders'] == true)
                 vm_target_folder = create_folder(connection, target_folder_path, target_datacenter_name)
               else
-                raise(_e)
+                raise
               end
             end
 
@@ -700,12 +700,12 @@ module Vmpooler
           datacenter = get_target_datacenter_from_config(pool_name)
           return nil if datacenter.nil?
 
-          propSpecs = {
+          propSpecs = { # rubocop:disable Naming/VariableName
             entity: self,
             inventoryPath: "#{datacenter}/vm/#{folder}"
           }
 
-          folder_object = connection.searchIndex.FindByInventoryPath(propSpecs)
+          folder_object = connection.searchIndex.FindByInventoryPath(propSpecs) # rubocop:disable Naming/VariableName
           return nil unless folder_object.class == RbVmomi::VIM::Folder
 
           folder_object
@@ -759,7 +759,7 @@ module Vmpooler
           return nil if cpu_usage.nil?
 
           cpu_size = host.summary.hardware.cpuMhz * host.summary.hardware.numCpuCores
-          (cpu_usage.to_f / cpu_size.to_f) * 100
+          cpu_usage.fdiv(cpu_size) * 100
         end
 
         def memory_utilization_for(host)
@@ -767,7 +767,7 @@ module Vmpooler
           return nil if memory_usage.nil?
 
           memory_size = host.summary.hardware.memorySize / 1024 / 1024
-          (memory_usage.to_f / memory_size.to_f) * 100
+          memory_usage.fdiv(memory_size) * 100
         end
 
         def get_average_cluster_utilization(hosts)
@@ -794,7 +794,6 @@ module Vmpooler
           end
 
           versions.each do |version|
-            targets = []
             targets = select_least_used_hosts(architectures[version], percentage)
             architectures[version] = targets
           end
@@ -891,12 +890,12 @@ module Vmpooler
           get_snapshot_list(vm.snapshot.rootSnapshotList, snapshotname) if vm.snapshot
         end
 
-        def build_propSpecs(datacenter, folder, vmname)
-          propSpecs = {
+        def build_propSpecs(datacenter, folder, vmname) # rubocop:disable Naming/MethodName
+          propSpecs = { # rubocop:disable Naming/VariableName
             entity => self,
             :inventoryPath => "#{datacenter}/vm/#{folder}/#{vmname}"
           }
-          propSpecs
+          propSpecs # rubocop:disable Naming/VariableName
         end
 
         def find_vm(pool_name, vmname, connection)
@@ -909,12 +908,12 @@ module Vmpooler
           datacenter = get_target_datacenter_from_config(pool_name)
           return nil if datacenter.nil?
 
-          propSpecs = {
+          propSpecs = { # rubocop:disable Naming/VariableName
             entity: self,
             inventoryPath: "#{datacenter}/vm/#{folder}/#{vmname}"
           }
 
-          connection.searchIndex.FindByInventoryPath(propSpecs)
+          connection.searchIndex.FindByInventoryPath(propSpecs) # rubocop:disable Naming/VariableName
         end
 
         def get_base_vm_container_from(connection)
@@ -975,7 +974,7 @@ module Vmpooler
               if migration_enabled? @config
                 if migration_count >= migration_limit
                   logger.log('s', "[ ] [#{pool_name}] '#{vm_name}' is running on #{vm_hash['host_name']}. No migration will be evaluated since the migration_limit has been reached")
-                  return
+                  break
                 end
                 run_select_hosts(pool_name, @provider_hosts)
                 if vm_in_target?(pool_name, vm_hash['host_name'], vm_hash['architecture'], @provider_hosts)
@@ -986,9 +985,9 @@ module Vmpooler
               else
                 logger.log('s', "[ ] [#{pool_name}] '#{vm_name}' is running on #{vm_hash['host_name']}")
               end
-            rescue StandardError => _e
+            rescue StandardError
               logger.log('s', "[!] [#{pool_name}] '#{vm_name}' is running on #{vm_hash['host_name']}")
-              raise _e
+              raise
             end
           end
         end
@@ -1008,11 +1007,11 @@ module Vmpooler
         def migrate_vm_and_record_timing(pool_name, vm_name, vm_hash, target_host_object, dest_host_name)
           start = Time.now
           migrate_vm_host(vm_hash['object'], target_host_object)
-          finish = format('%.2f', Time.now - start)
+          finish = format('%<time>.2f', time: Time.now - start)
           metrics.timing("migrate.#{pool_name}", finish)
           metrics.increment("migrate_from.#{vm_hash['host_name']}")
           metrics.increment("migrate_to.#{dest_host_name}")
-          checkout_to_migration = format('%.2f', Time.now - Time.parse($redis.hget("vmpooler__vm__#{vm_name}", 'checkout')))
+          checkout_to_migration = format('%<time>.2f', time: Time.now - Time.parse($redis.hget("vmpooler__vm__#{vm_name}", 'checkout')))
           $redis.hset("vmpooler__vm__#{vm_name}", 'migration_time', finish)
           $redis.hset("vmpooler__vm__#{vm_name}", 'checkout_to_migration', checkout_to_migration)
           finish
@@ -1025,7 +1024,7 @@ module Vmpooler
 
         def create_folder(connection, new_folder, datacenter)
           dc = connection.serviceInstance.find_datacenter(datacenter)
-          folder_object = dc.vmFolder.traverse(new_folder, type = RbVmomi::VIM::Folder, create = true)
+          folder_object = dc.vmFolder.traverse(new_folder, RbVmomi::VIM::Folder, true)
           raise("Cannot create folder #{new_folder}") if folder_object.nil?
 
           folder_object
@@ -1035,12 +1034,12 @@ module Vmpooler
           datacenter = get_target_datacenter_from_config(pool['name'])
           raise('cannot find datacenter') if datacenter.nil?
 
-          propSpecs = {
+          propSpecs = { # rubocop:disable Naming/VariableName
             entity: self,
             inventoryPath: "#{datacenter}/vm/#{pool['template']}"
           }
 
-          template_vm_object = connection.searchIndex.FindByInventoryPath(propSpecs)
+          template_vm_object = connection.searchIndex.FindByInventoryPath(propSpecs) # rubocop:disable Naming/VariableName
           raise("Pool #{pool['name']} specifies a template VM of #{pool['template']} which does not exist for the provider #{name}") if template_vm_object.nil?
 
           template_vm_object

spec/unit/providers/vsphere_spec.rb

@@ -2715,7 +2715,7 @@ EOT
 
     it 'creates a folder' do
       expect(connection.serviceInstance).to receive(:find_datacenter).with(datacenter).and_return(datacenter_object)
-      expect(datacenter_object.vmFolder).to receive(:traverse).with(new_folder, type=RbVmomi::VIM::Folder, create=true).and_return(folder_object)
+      expect(datacenter_object.vmFolder).to receive(:traverse).with(new_folder, RbVmomi::VIM::Folder, true).and_return(folder_object)
 
       result = subject.create_folder(connection, new_folder, datacenter)
       expect(result).to eq(folder_object)
@@ -2724,7 +2724,7 @@ EOT
     context 'with folder_object returning nil' do
       it 'shoud raise an error' do
         expect(connection.serviceInstance).to receive(:find_datacenter).with(datacenter).and_return(datacenter_object)
-        expect(datacenter_object.vmFolder).to receive(:traverse).with(new_folder, type=RbVmomi::VIM::Folder, create=true).and_return(nil)
+        expect(datacenter_object.vmFolder).to receive(:traverse).with(new_folder, RbVmomi::VIM::Folder, true).and_return(nil)
 
         expect{subject.create_folder(connection, new_folder, datacenter)}.to raise_error("Cannot create folder #{new_folder}")
       end