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 @@ def self.true?(obj)
     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 @@ def graph_link(target = '')
             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 @@ def graph_link(target = '')
         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 @@ def authenticate_ldap(port, host, user_object, base, username_str, password_str)
 
       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
-
-            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
+        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'],
-                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 @@ def get_capacity_metrics(pools, backend)
         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 @@ def update_clone_target(payload)
 
       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 @@ def sync_clone_targets
           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 @@ def log(_level, string)
 
       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 @@ def fail_pending_vm(vm, pool, timeout, exists = true)
 
     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 @@ def _check_ready_vm(vm, pool_name, ttl, provider)
           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 @@ def _clone_vm(pool_name, provider)
         $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 @@ def _destroy_vm(vm, pool, provider)
 
         $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 @@ def _create_vm_disk(pool_name, vm_name, disk_size, provider)
 
       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 @@ def _create_vm_snapshot(pool_name, vm_name, snapshot_name, provider)
 
       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 @@ def _revert_vm_snapshot(pool_name, vm_name, snapshot_name, provider)
 
       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 @@ def get_pool_name_for_vm(vm_name)
     # @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 @@ def check_disk_queue(maxloop = 0, loop_delay = 5)
           _check_disk_queue
           sleep(loop_delay)
 
-          unless maxloop.zero?
+          unless maxloop == 0
             break if loop_count >= maxloop
 
             loop_count += 1
@@ -655,7 +655,7 @@ def check_snapshot_queue(maxloop = 0, loop_delay = 5)
           _check_snapshot_queue
           sleep(loop_delay)
 
-          unless maxloop.zero?
+          unless maxloop == 0
             break if loop_count >= maxloop
 
             loop_count += 1
@@ -816,7 +816,7 @@ def check_pool(pool,
             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 @@ def repopulate_pool_vms(pool_name, provider, pool_check_response, pool_size)
       $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 @@ def _check_pool(pool, provider)
 
       begin
         inventory = create_inventory(pool, provider, pool_check_response)
-      rescue StandardError => e
+      rescue StandardError
         return(pool_check_response)
       end
 
@@ -1304,7 +1304,7 @@ def execute!(maxloop = 0, loop_delay = 1)
 
         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 @@ def plugin_map
     # 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 @@ def gem_directories
     # 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