From 5c67073dad33235684c281d2962513d5c76764d7 Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Thu, 9 Dec 2021 18:43:36 -0600
Subject: [PATCH] rubocop fixes

---
 lib/vmpooler/providers/gce.rb   | 307 ++++++++++++++++----------------
 spec/computeservice_helper.rb   |  49 ++---
 spec/helpers.rb                 |  23 ++-
 spec/spec_helper.rb             |  24 +--
 spec/unit/providers/gce_spec.rb | 278 ++++++++++++++---------------
 5 files changed, 342 insertions(+), 339 deletions(-)

diff --git a/lib/vmpooler/providers/gce.rb b/lib/vmpooler/providers/gce.rb
index b23ceca..b4f5f8e 100644
--- a/lib/vmpooler/providers/gce.rb
+++ b/lib/vmpooler/providers/gce.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 require 'googleauth'
 require 'google/apis/compute_v1'
 require 'bigdecimal'
@@ -8,6 +9,7 @@ require 'vmpooler/providers/base'
 module Vmpooler
   class PoolManager
     class Provider
+      # This class represent a GCE provider to CRUD resources in a gce cloud.
       class Gce < Vmpooler::PoolManager::Provider::Base
         # The connection_pool method is normally used only for testing
         attr_reader :connection_pool
@@ -75,7 +77,7 @@ module Vmpooler
           return provider_config['machine_type'] if provider_config['machine_type']
         end
 
-        #Base methods that are implemented:
+        # Base methods that are implemented:
 
         # vms_in_pool lists all the VM names in a pool, which is based on the VMs
         # having a label "pool" that match a pool config name.
@@ -87,10 +89,11 @@ module Vmpooler
         #     [Hashtable]
         #       [String] name : the name of the VM instance (unique for whole project)
         def vms_in_pool(pool_name)
-          debug_logger("vms_in_pool")
+          debug_logger('vms_in_pool')
           vms = []
           pool = pool_config(pool_name)
           raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil?
+
           zone = zone(pool_name)
           filter = "(labels.pool = #{pool_name})"
           instance_list = connection.list_instances(project, zone, filter: filter)
@@ -121,13 +124,14 @@ module Vmpooler
         #    [String] zone       : URL of the zone where the instance resides.
         #    [String] machine_type : Full or partial URL of the machine type resource to use for this instance, in the format: zones/zone/machineTypes/machine-type.
         def get_vm(pool_name, vm_name)
-          debug_logger("get_vm")
+          debug_logger('get_vm')
           vm_hash = nil
           begin
             vm_object = connection.get_instance(project, zone(pool_name), vm_name)
           rescue ::Google::Apis::ClientError => e
             raise e unless e.status_code == 404
-            #swallow the ClientError error 404 and return nil when the VM was not found
+
+            # swallow the ClientError error 404 and return nil when the VM was not found
             return nil
           end
 
@@ -150,38 +154,37 @@ module Vmpooler
         # returns
         #   [Hashtable] of the VM as per get_vm(pool_name, vm_name)
         def create_vm(pool_name, new_vmname)
-          debug_logger("create_vm")
+          debug_logger('create_vm')
           pool = pool_config(pool_name)
           raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil?
-          vm_hash = nil
+
           # harcoded network info
           network_interfaces = Google::Apis::ComputeV1::NetworkInterface.new(
-            :network => network_name
+            network: network_name
           )
-          initParams = {
-            :source_image => pool['template'], #The source image to create this disk.
-            :labels => {'vm' => new_vmname, 'pool' => pool_name},
-            :disk_name => "#{new_vmname}-disk0"
+          init_params = {
+            source_image: pool['template'], # The source image to create this disk.
+            labels: { 'vm' => new_vmname, 'pool' => pool_name },
+            disk_name: "#{new_vmname}-disk0"
           }
           disk = Google::Apis::ComputeV1::AttachedDisk.new(
-            :auto_delete => true,
-            :boot => true,
-            :initialize_params => Google::Apis::ComputeV1::AttachedDiskInitializeParams.new(initParams)
+            auto_delete: true,
+            boot: true,
+            initialize_params: Google::Apis::ComputeV1::AttachedDiskInitializeParams.new(init_params)
           )
 
           # Assume all pool config is valid i.e. not missing
           client = ::Google::Apis::ComputeV1::Instance.new(
-            :name => new_vmname,
-            :machine_type => pool['machine_type'],
-            :disks => [disk],
-            :network_interfaces => [network_interfaces],
-            :labels => {'vm' => new_vmname, 'pool' => pool_name}
+            name: new_vmname,
+            machine_type: pool['machine_type'],
+            disks: [disk],
+            network_interfaces: [network_interfaces],
+            labels: { 'vm' => new_vmname, 'pool' => pool_name }
           )
-          debug_logger("trigger insert_instance")
+          debug_logger('trigger insert_instance')
           result = connection.insert_instance(project, zone(pool_name), client)
-          result = wait_for_operation(project, pool_name, result)
-          vm_hash = get_vm(pool_name, new_vmname)
-          vm_hash
+          wait_for_operation(project, pool_name, result)
+          get_vm(pool_name, new_vmname)
         end
 
         # create_disk creates an additional disk for an existing VM. It will name the new
@@ -200,7 +203,7 @@ module Vmpooler
         # returns
         #   [boolean] true : once the operations are finished
         def create_disk(pool_name, vm_name, disk_size)
-          debug_logger("create_disk")
+          debug_logger('create_disk')
           pool = pool_config(pool_name)
           raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil?
 
@@ -208,18 +211,19 @@ module Vmpooler
             vm_object = connection.get_instance(project, zone(pool_name), vm_name)
           rescue ::Google::Apis::ClientError => e
             raise e unless e.status_code == 404
-            #if it does not exist
+
+            # if it does not exist
             raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}")
           end
           # this number should start at 1 when there is only the boot disk,
           # eg the new disk will be named spicy-proton-disk1
-          number_disk = vm_object.disks.length()
+          number_disk = vm_object.disks.length
 
           disk_name = "#{vm_name}-disk#{number_disk}"
           disk = Google::Apis::ComputeV1::Disk.new(
-            :name => disk_name,
-            :size_gb => disk_size,
-            :labels => {"pool" => pool_name, "vm" => vm_name}
+            name: disk_name,
+            size_gb: disk_size,
+            labels: { 'pool' => pool_name, 'vm' => vm_name }
           )
           debug_logger("trigger insert_disk #{disk_name} for #{vm_name}")
           result = connection.insert_disk(project, zone(pool_name), disk)
@@ -228,9 +232,9 @@ module Vmpooler
           new_disk = connection.get_disk(project, zone(pool_name), disk_name)
 
           attached_disk = Google::Apis::ComputeV1::AttachedDisk.new(
-            :auto_delete => true,
-            :boot => false,
-            :source => new_disk.self_link
+            auto_delete: true,
+            boot: false,
+            source: new_disk.self_link
           )
           debug_logger("trigger attach_disk #{disk_name} for #{vm_name}")
           result = connection.attach_disk(project, zone(pool_name), vm_object.name, attached_disk)
@@ -254,12 +258,13 @@ module Vmpooler
         #   RuntimeError if the vm_name cannot be found
         #   RuntimeError if the snapshot_name already exists for this VM
         def create_snapshot(pool_name, vm_name, new_snapshot_name)
-          debug_logger("create_snapshot")
+          debug_logger('create_snapshot')
           begin
             vm_object = connection.get_instance(project, zone(pool_name), vm_name)
           rescue ::Google::Apis::ClientError => e
             raise e unless e.status_code == 404
-            #if it does not exist
+
+            # if it does not exist
             raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}")
           end
 
@@ -272,11 +277,11 @@ module Vmpooler
             snapshot_obj = ::Google::Apis::ComputeV1::Snapshot.new(
               name: "#{new_snapshot_name}-#{disk_name}",
               labels: {
-                "snapshot_name" => new_snapshot_name,
-                "vm" => vm_name,
-                "pool" => pool_name,
-                "diskname" => disk_name,
-                "boot" => attached_disk.boot.to_s
+                'snapshot_name' => new_snapshot_name,
+                'vm' => vm_name,
+                'pool' => pool_name,
+                'diskname' => disk_name,
+                'boot' => attached_disk.boot.to_s
               }
             )
             debug_logger("trigger async create_disk_snapshot #{vm_name}: #{new_snapshot_name}-#{disk_name}")
@@ -284,7 +289,7 @@ module Vmpooler
             # do them all async, keep a list, check later
             result_list << result
           end
-          #now check they are done
+          # now check they are done
           result_list.each do |result|
             wait_for_operation(project, pool_name, result)
           end
@@ -310,12 +315,13 @@ module Vmpooler
         #   RuntimeError if the vm_name cannot be found
         #   RuntimeError if the snapshot_name already exists for this VM
         def revert_snapshot(pool_name, vm_name, snapshot_name)
-          debug_logger("revert_snapshot")
+          debug_logger('revert_snapshot')
           begin
             vm_object = connection.get_instance(project, zone(pool_name), vm_name)
           rescue ::Google::Apis::ClientError => e
             raise e unless e.status_code == 404
-            #if it does not exist
+
+            # if it does not exist
             raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}")
           end
 
@@ -328,26 +334,24 @@ module Vmpooler
           wait_for_operation(project, pool_name, result)
 
           # Delete existing disks
-          if vm_object.disks
-            vm_object.disks.each do |attached_disk|
-              debug_logger("trigger detach_disk #{vm_name}: #{attached_disk.device_name}")
-              result = connection.detach_disk(project, zone(pool_name), vm_name, attached_disk.device_name)
-              wait_for_operation(project, pool_name, result)
-              current_disk_name = disk_name_from_source(attached_disk)
-              debug_logger("trigger delete_disk #{vm_name}: #{current_disk_name}")
-              result = connection.delete_disk(project, zone(pool_name), current_disk_name)
-              wait_for_operation(project, pool_name, result)
-            end
+          vm_object.disks&.each do |attached_disk|
+            debug_logger("trigger detach_disk #{vm_name}: #{attached_disk.device_name}")
+            result = connection.detach_disk(project, zone(pool_name), vm_name, attached_disk.device_name)
+            wait_for_operation(project, pool_name, result)
+            current_disk_name = disk_name_from_source(attached_disk)
+            debug_logger("trigger delete_disk #{vm_name}: #{current_disk_name}")
+            result = connection.delete_disk(project, zone(pool_name), current_disk_name)
+            wait_for_operation(project, pool_name, result)
           end
 
           # this block is sensitive to disruptions, for example if vmpooler is stopped while this is running
           snapshot_object.each do |snapshot|
             current_disk_name = snapshot.labels['diskname']
-            bootable = (snapshot.labels['boot'] == "true")
+            bootable = (snapshot.labels['boot'] == 'true')
             disk = Google::Apis::ComputeV1::Disk.new(
-              :name => current_disk_name,
-              :labels => {"pool" => pool_name, "vm" => vm_name},
-              :source_snapshot => snapshot.self_link
+              name: current_disk_name,
+              labels: { 'pool' => pool_name, 'vm' => vm_name },
+              source_snapshot: snapshot.self_link
             )
             # create disk in GCE as a separate resource
             debug_logger("trigger insert_disk #{vm_name}: #{current_disk_name} based on #{snapshot.self_link}")
@@ -356,9 +360,9 @@ module Vmpooler
             # read the new disk info
             new_disk_info = connection.get_disk(project, zone(pool_name), current_disk_name)
             new_attached_disk = Google::Apis::ComputeV1::AttachedDisk.new(
-              :auto_delete => true,
-              :boot => bootable,
-              :source => new_disk_info.self_link
+              auto_delete: true,
+              boot: bootable,
+              source: new_disk_info.self_link
             )
             # attach the new disk to existing instance
             debug_logger("trigger attach_disk #{vm_name}: #{current_disk_name}")
@@ -380,18 +384,19 @@ module Vmpooler
         # returns
         #   [boolean] true : once the operations are finished
         def destroy_vm(pool_name, vm_name)
-          debug_logger("destroy_vm")
+          debug_logger('destroy_vm')
           deleted = false
           begin
-            vm_object = connection.get_instance(project, zone(pool_name), vm_name)
+            connection.get_instance(project, zone(pool_name), vm_name)
           rescue ::Google::Apis::ClientError => e
             raise e unless e.status_code == 404
+
             # If a VM doesn't exist then it is effectively deleted
             deleted = true
             debug_logger("instance #{vm_name} already deleted")
           end
 
-          if(!deleted)
+          unless deleted
             debug_logger("trigger delete_instance #{vm_name}")
             result = connection.delete_instance(project, zone(pool_name), vm_name)
             wait_for_operation(project, pool_name, result, 10)
@@ -401,41 +406,37 @@ module Vmpooler
           filter = "(labels.vm = #{vm_name})"
           disk_list = connection.list_disks(project, zone(pool_name), filter: filter)
           result_list = []
-          unless disk_list.items.nil?
-            disk_list.items.each do |disk|
-              debug_logger("trigger delete_disk #{disk.name}")
-              result = connection.delete_disk(project, zone(pool_name), disk.name)
-              # do them all async, keep a list, check later
-              result_list << result
-            end
+          disk_list.items&.each do |disk|
+            debug_logger("trigger delete_disk #{disk.name}")
+            result = connection.delete_disk(project, zone(pool_name), disk.name)
+            # do them all async, keep a list, check later
+            result_list << result
           end
-          #now check they are done
-          result_list.each do |result|
-            wait_for_operation(project, pool_name, result)
+          # now check they are done
+          result_list.each do |r|
+            wait_for_operation(project, pool_name, r)
           end
 
           # list and delete leftover snapshots, this could happen if snapshots were taken,
           # as they are not removed when the original disk is deleted or the instance is detroyed
           snapshot_list = find_all_snapshots(vm_name)
           result_list = []
-          unless snapshot_list.nil?
-            snapshot_list.each do |snapshot|
-              debug_logger("trigger delete_snapshot #{snapshot.name}")
-              result = connection.delete_snapshot(project, snapshot.name)
-              # do them all async, keep a list, check later
-              result_list << result
-            end
+          snapshot_list&.each do |snapshot|
+            debug_logger("trigger delete_snapshot #{snapshot.name}")
+            result = connection.delete_snapshot(project, snapshot.name)
+            # do them all async, keep a list, check later
+            result_list << result
           end
-          #now check they are done
-          result_list.each do |result|
-            wait_for_operation(project, pool_name, result)
+          # now check they are done
+          result_list.each do |r|
+            wait_for_operation(project, pool_name, r)
           end
           true
         end
 
         def vm_ready?(_pool_name, vm_name)
           begin
-            #TODO: we could use a healthcheck resource attached to instance
+            # TODO: we could use a healthcheck resource attached to instance
             open_socket(vm_name, global_config[:config]['domain'])
           rescue StandardError => _e
             return false
@@ -446,7 +447,7 @@ module Vmpooler
         # Scans zones that are configured for list of resources (VM, disks, snapshots) that do not have the label.pool set
         # to one of the configured pools. If it is also not in the allowlist, the resource is destroyed
         def purge_unconfigured_resources(allowlist)
-          debug_logger("purge_unconfigured_resources")
+          debug_logger('purge_unconfigured_resources')
           pools_array = provided_pools
           filter = {}
           # we have to group things by zone, because the API search feature is done against a zone and not global
@@ -455,45 +456,44 @@ module Vmpooler
             filter[zone(pool)] = [] if filter[zone(pool)].nil?
             filter[zone(pool)] << "(labels.pool != #{pool})"
           end
-          filter.keys.each do |zone|
+          filter.each_key do |zone|
             # this filter should return any item that have a labels.pool that is not in the config OR
             # do not have a pool label at all
-            filter_string = filter[zone].join(" AND ") + " OR -labels.pool:*"
-            #VMs
+            filter_string = "#{filter[zone].join(' AND ')} OR -labels.pool:*"
+            # VMs
             instance_list = connection.list_instances(project, zone, filter: filter_string)
 
             result_list = []
-            unless instance_list.items.nil?
-              instance_list.items.each do |vm|
-                next if should_be_ignored(vm, allowlist)
-                debug_logger("trigger async delete_instance #{vm.name}")
-                result = connection.delete_instance(project, zone, vm.name)
-                result_list << result
-              end
+            instance_list.items&.each do |vm|
+              next if should_be_ignored(vm, allowlist)
+
+              debug_logger("trigger async delete_instance #{vm.name}")
+              result = connection.delete_instance(project, zone, vm.name)
+              result_list << result
             end
-            #now check they are done
+            # now check they are done
             result_list.each do |result|
               wait_for_zone_operation(project, zone, result)
             end
 
-            #Disks
+            # Disks
             disks_list = connection.list_disks(project, zone, filter: filter_string)
-            unless disks_list.items.nil?
-              disks_list.items.each do |disk|
-                next if should_be_ignored(disk, allowlist)
-                debug_logger("trigger async no wait delete_disk #{disk.name}")
-                result = connection.delete_disk(project, zone, disk.name)
-              end
+            disks_list.items&.each do |disk|
+              next if should_be_ignored(disk, allowlist)
+
+              debug_logger("trigger async no wait delete_disk #{disk.name}")
+              connection.delete_disk(project, zone, disk.name)
             end
 
-            #Snapshots
+            # Snapshots
             snapshot_list = connection.list_snapshots(project, filter: filter_string)
-            unless snapshot_list.items.nil?
-              snapshot_list.items.each do |sn|
-                next if should_be_ignored(sn, allowlist)
-                debug_logger("trigger async no wait delete_snapshot #{sn.name}")
-                result = connection.delete_snapshot(project, sn.name)
-              end
+            next if snapshot_list.items.nil?
+
+            snapshot_list.items.each do |sn|
+              next if should_be_ignored(sn, allowlist)
+
+              debug_logger("trigger async no wait delete_snapshot #{sn.name}")
+              connection.delete_snapshot(project, sn.name)
             end
           end
         end
@@ -506,45 +506,47 @@ module Vmpooler
         #   [String] vm_name   : Name of the VM to check if ready
         # returns
         #   [Boolean] : true if successful, false if an error occurred and it should retry
-        def tag_vm_user(pool, vm)
-          user = get_current_user(vm)
-          vm_hash = get_vm(pool, vm)
+        def tag_vm_user(pool, vm_name)
+          user = get_current_user(vm_name)
+          vm_hash = get_vm(pool, vm_name)
           return false if vm_hash.nil?
+
           new_labels = vm_hash['labels']
           # bailing in this case since labels should exist, and continuing would mean losing them
           return false if new_labels.nil?
+
           # add new label called token-user, with value as user
           new_labels['token-user'] = user
           begin
-            instances_set_labels_request_object = Google::Apis::ComputeV1::InstancesSetLabelsRequest.new(label_fingerprint:vm_hash['label_fingerprint'], labels: new_labels)
-            result = connection.set_instance_labels(project, zone(pool), vm, instances_set_labels_request_object)
+            instances_set_labels_request_object = Google::Apis::ComputeV1::InstancesSetLabelsRequest.new(label_fingerprint: vm_hash['label_fingerprint'], labels: new_labels)
+            result = connection.set_instance_labels(project, zone(pool), vm_name, instances_set_labels_request_object)
             wait_for_zone_operation(project, zone(pool), result)
           rescue StandardError => _e
             return false
           end
-          return true
+          true
         end
 
         # END BASE METHODS
 
         def should_be_ignored(item, allowlist)
           return false if allowlist.nil?
-          allowlist.map!(&:downcase)  # remove uppercase from configured values because its not valid as resource label
+
+          allowlist.map!(&:downcase) # remove uppercase from configured values because its not valid as resource label
           array_flattened_labels = []
-          unless item.labels.nil?
-            item.labels.each do |k,v|
-              array_flattened_labels << "#{k}=#{v}"
-            end
+          item.labels&.each do |k, v|
+            array_flattened_labels << "#{k}=#{v}"
           end
           (!item.labels.nil? && allowlist&.include?(item.labels['pool'])) || # the allow list specifies the value within the pool label
-            (allowlist&.include?("") && !item.labels&.keys&.include?('pool')) || # the allow list specifies "" string and the pool label is not set
+            (allowlist&.include?('') && !item.labels&.keys&.include?('pool')) || # the allow list specifies "" string and the pool label is not set
             !(allowlist & array_flattened_labels).empty? # the allow list specify a fully qualified label eg user=Bob and the item has it
         end
 
-        def get_current_user(vm)
+        def get_current_user(vm_name)
           @redis.with_metrics do |redis|
-            user = redis.hget("vmpooler__vm__#{vm}", 'token:user')
-            return "" if user.nil?
+            user = redis.hget("vmpooler__vm__#{vm_name}", 'token:user')
+            return '' if user.nil?
+
             # cleanup so it's a valid label value
             # can't have upercase
             user = user.downcase
@@ -555,13 +557,13 @@ module Vmpooler
         end
 
         # Compute resource wait for operation to be DONE (synchronous operation)
-        def wait_for_zone_operation(project, zone, result, retries=5)
+        def wait_for_zone_operation(project, zone, result, retries = 5)
           while result.status != 'DONE'
             result = connection.wait_zone_operation(project, zone, result.name)
             debug_logger("  -> wait_for_zone_operation status #{result.status} (#{result.name})")
           end
           if result.error # unsure what kind of error can be stored here
-            error_message = ""
+            error_message = ''
             # array of errors, combine them all
             result.error.each do |error|
               error_message = "#{error_message} #{error.code}:#{error.message}"
@@ -571,19 +573,20 @@ module Vmpooler
           result
         rescue Google::Apis::TransmissionError => e
           # Error returned once timeout reached, each retry typically about 1 minute.
-          if retries > 0
-            retries = retries - 1
+          if retries.positive?
+            retries -= 1
             retry
           end
           raise
         rescue Google::Apis::ClientError => e
           raise e unless e.status_code == 404
+
           # if the operation is not found, and we are 'waiting' on it, it might be because it
           # is already finished
           puts "waited on #{result.name} but was not found, so skipping"
         end
 
-        def wait_for_operation(project, pool_name, result, retries=5)
+        def wait_for_operation(project, pool_name, result, retries = 5)
           wait_for_zone_operation(project, zone(pool_name), result, retries)
         end
 
@@ -594,17 +597,17 @@ module Vmpooler
           return nil if pool_configuration.nil?
 
           {
-            'name'              => vm_object.name,
-            'hostname'          => vm_object.hostname,
-            'template'          => pool_configuration && pool_configuration.key?('template') ? pool_configuration['template'] : nil, #TODO: get it from the API, not from config, but this is what vSphere does too!
-            'poolname'          => vm_object.labels && vm_object.labels.key?('pool') ? vm_object.labels['pool'] : nil,
-            'boottime'          => vm_object.creation_timestamp,
-            'status'            => vm_object.status, # One of the following values: PROVISIONING, STAGING, RUNNING, STOPPING, SUSPENDING, SUSPENDED, REPAIRING, and TERMINATED
-            'zone'              => vm_object.zone,
-            'machine_type'      => vm_object.machine_type,
-            'labels'            => vm_object.labels,
+            'name' => vm_object.name,
+            'hostname' => vm_object.hostname,
+            'template' => pool_configuration&.key?('template') ? pool_configuration['template'] : nil, # TODO: get it from the API, not from config, but this is what vSphere does too!
+            'poolname' => vm_object.labels&.key?('pool') ? vm_object.labels['pool'] : nil,
+            'boottime' => vm_object.creation_timestamp,
+            'status' => vm_object.status, # One of the following values: PROVISIONING, STAGING, RUNNING, STOPPING, SUSPENDING, SUSPENDED, REPAIRING, and TERMINATED
+            'zone' => vm_object.zone,
+            'machine_type' => vm_object.machine_type,
+            'labels' => vm_object.labels,
             'label_fingerprint' => vm_object.label_fingerprint
-            #'powerstate' => powerstate
+            # 'powerstate' => powerstate
           }
         end
 
@@ -618,7 +621,7 @@ module Vmpooler
           retry_factor = global_config[:config]['retry_factor'] || 10
           try = 1
           begin
-            scopes = ["https://www.googleapis.com/auth/compute", "https://www.googleapis.com/auth/cloud-platform"]
+            scopes = ['https://www.googleapis.com/auth/compute', 'https://www.googleapis.com/auth/cloud-platform']
 
             authorization = Google::Auth.get_application_default(scopes)
 
@@ -627,7 +630,7 @@ module Vmpooler
 
             metrics.increment('connect.open')
             compute
-          rescue StandardError => e #is that even a thing?
+          rescue StandardError => e # is that even a thing?
             metrics.increment('connect.fail')
             raise e if try >= max_tries
 
@@ -653,18 +656,18 @@ module Vmpooler
 
         # this is used because for one vm, with the same snapshot name there could be multiple snapshots,
         # one for each disk
-        def find_snapshot(vm, snapshotname)
-          filter = "(labels.vm = #{vm}) AND (labels.snapshot_name = #{snapshotname})"
-          snapshot_list = connection.list_snapshots(project,filter: filter)
-          return snapshot_list.items #array of snapshot objects
+        def find_snapshot(vm_name, snapshotname)
+          filter = "(labels.vm = #{vm_name}) AND (labels.snapshot_name = #{snapshotname})"
+          snapshot_list = connection.list_snapshots(project, filter: filter)
+          snapshot_list.items # array of snapshot objects
         end
 
         # find all snapshots ever created for one vm,
         # regardless of snapshot name, for example when deleting it all
-        def find_all_snapshots(vm)
-          filter = "(labels.vm = #{vm})"
-          snapshot_list = connection.list_snapshots(project,filter: filter)
-          return snapshot_list.items #array of snapshot objects
+        def find_all_snapshots(vm_name)
+          filter = "(labels.vm = #{vm_name})"
+          snapshot_list = connection.list_snapshots(project, filter: filter)
+          snapshot_list.items # array of snapshot objects
         end
 
         def disk_name_from_source(attached_disk)
@@ -673,10 +676,10 @@ module Vmpooler
 
         # used in local dev environment, set DEBUG_FLAG=true
         # this way the upstream vmpooler manager does not get polluted with logs
-        def debug_logger(message, send_to_upstream=false)
-          #the default logger is simple and does not enforce debug levels (the first argument)
+        def debug_logger(message, send_to_upstream: false)
+          # the default logger is simple and does not enforce debug levels (the first argument)
           puts message if ENV['DEBUG_FLAG']
-          logger.log("[g]", message) if send_to_upstream
+          logger.log('[g]', message) if send_to_upstream
         end
       end
     end
diff --git a/spec/computeservice_helper.rb b/spec/computeservice_helper.rb
index a21363c..8f56b97 100644
--- a/spec/computeservice_helper.rb
+++ b/spec/computeservice_helper.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # this file is used to Mock the GCE objects, for example the main ComputeService object
 MockResult = Struct.new(
   # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Operation.html
@@ -7,7 +9,7 @@ MockResult = Struct.new(
   keyword_init: true
 )
 
-MockOperationError = Array.new
+MockOperationError = [].freeze
 
 MockOperationErrorError = Struct.new(
   # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Operation/Error/Error.html
@@ -27,19 +29,19 @@ MockInstance = Struct.new(
 )
 
 MockInstanceList = Struct.new(
-  #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/InstanceList.html
+  # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/InstanceList.html
   :id, :items, :kind, :next_page_token, :self_link, :warning,
   keyword_init: true
 )
 
 MockDiskList = Struct.new(
-  #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/DiskList.html
+  # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/DiskList.html
   :id, :items, :kind, :next_page_token, :self_link, :warning,
   keyword_init: true
 )
 
 MockDisk = Struct.new(
-  #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Disk.html
+  # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Disk.html
   :creation_timestamp, :description, :disk_encryption_key, :guest_os_features, :id, :kind, :label_fingerprint, :labels,
   :last_attach_timestamp, :last_detach_timestamp, :license_codes, :licenses, :name, :options,
   :physical_block_size_bytes, :region, :replica_zones, :resource_policies, :self_link, :size_gb, :source_disk,
@@ -49,13 +51,13 @@ MockDisk = Struct.new(
 )
 
 MockSnapshotList = Struct.new(
-  #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/DiskList.html
+  # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/DiskList.html
   :id, :items, :kind, :next_page_token, :self_link, :warning,
   keyword_init: true
 )
 
 MockSnapshot = Struct.new(
-  #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Snapshot.html
+  # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/Snapshot.html
   :auto_created, :chain_name, :creation_timestamp, :description, :disk_size_gb, :download_bytes, :id, :kind,
   :label_fingerprint, :labels, :license_codes, :licenses, :name, :self_link, :snapshot_encryption_key, :source_disk,
   :source_disk_encryption_key, :source_disk_id, :status, :storage_bytes, :storage_bytes_status, :storage_locations,
@@ -63,7 +65,7 @@ MockSnapshot = Struct.new(
 )
 
 MockAttachedDisk = Struct.new(
-  #https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/AttachedDisk.html
+  # https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/ComputeV1/AttachedDisk.html
   :auto_delete, :boot, :device_name, :disk_encryption_key, :disk_size_gb, :guest_os_features, :index,
   :initialize_params, :interface, :kind, :licenses, :mode, :shielded_instance_initial_state, :source, :type,
   keyword_init: true
@@ -80,6 +82,7 @@ MockComputeServiceConnection = Struct.new(
   def get_instance
     MockInstance.new
   end
+
   # Alias to serviceContent.propertyCollector
   def insert_instance
     MockResult.new
@@ -90,20 +93,18 @@ end
 # Mocking Methods
 # -------------------------------------------------------------------------------------------------------------
 
-=begin
-def mock_RbVmomi_VIM_ClusterComputeResource(options = {})
-  options[:name]  = 'Cluster' + rand(65536).to_s if options[:name].nil?
-
-  mock = MockClusterComputeResource.new()
-
-  mock.name = options[:name]
-  # All cluster compute resources have a root Resource Pool
-  mock.resourcePool = mock_RbVmomi_VIM_ResourcePool({:name => options[:name]})
-
-  allow(mock).to receive(:is_a?) do |expected_type|
-    expected_type == RbVmomi::VIM::ClusterComputeResource
-  end
-
-  mock
-end
-=end
+# def mock_RbVmomi_VIM_ClusterComputeResource(options = {})
+#   options[:name]  = 'Cluster' + rand(65536).to_s if options[:name].nil?
+#
+#   mock = MockClusterComputeResource.new()
+#
+#   mock.name = options[:name]
+#   # All cluster compute resources have a root Resource Pool
+#   mock.resourcePool = mock_RbVmomi_VIM_ResourcePool({:name => options[:name]})
+#
+#   allow(mock).to receive(:is_a?) do |expected_type|
+#     expected_type == RbVmomi::VIM::ClusterComputeResource
+#   end
+#
+#   mock
+# end
diff --git a/spec/helpers.rb b/spec/helpers.rb
index 87245db..4b2dff6 100644
--- a/spec/helpers.rb
+++ b/spec/helpers.rb
@@ -1,23 +1,22 @@
+# frozen_string_literal: true
+
 require 'mock_redis'
 
 def redis
-  unless @redis
-    @redis = MockRedis.new
-  end
+  @redis ||= MockRedis.new
   @redis
 end
 
 # Mock an object which represents a Logger.  This stops the proliferation
 # of allow(logger).to .... expectations in tests.
 class MockLogger
-  def log(_level, string)
-  end
+  def log(_level, string); end
 end
 
 def expect_json(ok = true, http = 200)
   expect(last_response.header['Content-Type']).to eq('application/json')
 
-  if (ok == true) then
+  if ok == true
     expect(JSON.parse(last_response.body)['ok']).to eq(true)
   else
     expect(JSON.parse(last_response.body)['ok']).to eq(false)
@@ -35,7 +34,7 @@ def get_token_data(token)
   redis.hgetall("vmpooler__token__#{token}")
 end
 
-def token_exists?(token)
+def token_exists?(_token)
   result = get_token_data
   result && !result.empty?
 end
@@ -43,7 +42,7 @@ end
 def create_ready_vm(template, name, redis, token = nil)
   create_vm(name, redis, token)
   redis.sadd("vmpooler__ready__#{template}", name)
-  redis.hset("vmpooler__vm__#{name}", "template", template)
+  redis.hset("vmpooler__vm__#{name}", 'template', template)
 end
 
 def create_running_vm(template, name, redis, token = nil, user = nil)
@@ -57,7 +56,7 @@ end
 def create_pending_vm(template, name, redis, token = nil)
   create_vm(name, redis, token)
   redis.sadd("vmpooler__pending__#{template}", name)
-  redis.hset("vmpooler__vm__#{name}", "template", template)
+  redis.hset("vmpooler__vm__#{name}", 'template', template)
 end
 
 def create_vm(name, redis, token = nil, user = nil)
@@ -100,12 +99,12 @@ end
 
 def snapshot_revert_vm(vm, snapshot = '12345678901234567890123456789012', redis)
   redis.sadd('vmpooler__tasks__snapshot-revert', "#{vm}:#{snapshot}")
-  redis.hset("vmpooler__vm__#{vm}", "snapshot:#{snapshot}", "1")
+  redis.hset("vmpooler__vm__#{vm}", "snapshot:#{snapshot}", '1')
 end
 
 def snapshot_vm(vm, snapshot = '12345678901234567890123456789012', redis)
   redis.sadd('vmpooler__tasks__snapshot', "#{vm}:#{snapshot}")
-  redis.hset("vmpooler__vm__#{vm}", "snapshot:#{snapshot}", "1")
+  redis.hset("vmpooler__vm__#{vm}", "snapshot:#{snapshot}", '1')
 end
 
 def disk_task_vm(vm, disk_size = '10', redis)
@@ -127,7 +126,7 @@ def vm_reverted_to_snapshot?(vm, redis, snapshot = nil)
 end
 
 def pool_has_ready_vm?(pool, vm, redis)
-  !!redis.sismember('vmpooler__ready__' + pool, vm)
+  !!redis.sismember("vmpooler__ready__#{pool}", vm)
 end
 
 def create_ondemand_request_for_test(request_id, score, platforms_string, redis, user = nil, token = nil)
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 4136646..aafca7d 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -1,9 +1,9 @@
-=begin
-require 'simplecov'
-SimpleCov.start do
-  add_filter '/spec/'
-end
-=end
+# frozen_string_literal: true
+
+# require 'simplecov'
+# SimpleCov.start do
+#   add_filter '/spec/'
+# end
 require 'helpers'
 require 'rspec'
 require 'vmpooler'
@@ -19,16 +19,16 @@ def fixtures_dir
   File.join(project_root_dir, 'spec', 'fixtures')
 end
 
-def create_google_client_error(status_code, message, reason="notFound")
-  Google::Apis::ClientError.new(Google::Apis::ClientError, status_code:status_code, body:'{
+def create_google_client_error(status_code, message, reason = 'notFound')
+  Google::Apis::ClientError.new(Google::Apis::ClientError, status_code: status_code, body: '{
   "error": {
-    "code": '+status_code.to_s+',
-    "message": "'+message+'",
+    "code": ' + status_code.to_s + ',
+    "message": "' + message + '",
     "errors": [
       {
-        "message": "'+message+'",
+        "message": "' + message + '",
         "domain": "global",
-        "reason": "'+reason+'"
+        "reason": "' + reason + '"
       }
     ]
   }
diff --git a/spec/unit/providers/gce_spec.rb b/spec/unit/providers/gce_spec.rb
index 9511064..6dc1fef 100644
--- a/spec/unit/providers/gce_spec.rb
+++ b/spec/unit/providers/gce_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 require 'mock_redis'
 require 'vmpooler/providers/gce'
@@ -12,42 +14,44 @@ describe 'Vmpooler::PoolManager::Provider::Gce' do
   let(:poolname) { 'debian-9' }
   let(:provider_options) { { 'param' => 'value' } }
   let(:project) { 'dio-samuel-dev' }
-  let(:zone){ 'us-west1-b' }
-  let(:config) { YAML.load(<<-EOT
----
-:config:
-  max_tries: 3
-  retry_factor: 10
-:providers:
-  :gce:
-    connection_pool_timeout: 1
-    project: '#{project}'
-    zone: '#{zone}'
-    network_name: 'global/networks/default'
-:pools:
-  - name: '#{poolname}'
-    alias: [ 'mockpool' ]
-    template: 'projects/debian-cloud/global/images/family/debian-9'
-    size: 5
-    timeout: 10
-    ready_ttl: 1440
-    provider: 'gce'
-    network_name: 'default'
-    machine_type: 'zones/#{zone}/machineTypes/e2-micro'
-EOT
-    )
-  }
+  let(:zone) { 'us-west1-b' }
+  let(:config) do
+    YAML.safe_load(<<~EOT
+      ---
+      :config:
+        max_tries: 3
+        retry_factor: 10
+      :providers:
+        :gce:
+          connection_pool_timeout: 1
+          project: '#{project}'
+          zone: '#{zone}'
+          network_name: 'global/networks/default'
+      :pools:
+        - name: '#{poolname}'
+          alias: [ 'mockpool' ]
+          template: 'projects/debian-cloud/global/images/family/debian-9'
+          size: 5
+          timeout: 10
+          ready_ttl: 1440
+          provider: 'gce'
+          network_name: 'default'
+          machine_type: 'zones/#{zone}/machineTypes/e2-micro'
+    EOT
+                  )
+  end
 
   let(:vmname) { 'vm16' }
   let(:connection) { MockComputeServiceConnection.new }
-  let(:redis_connection_pool) { Vmpooler::PoolManager::GenericConnectionPool.new(
-    metrics: metrics,
-    connpool_type: 'redis_connection_pool',
-    connpool_provider: 'testprovider',
-    size: 1,
-    timeout: 5
-  ) { MockRedis.new }
-  }
+  let(:redis_connection_pool) do
+    Vmpooler::PoolManager::GenericConnectionPool.new(
+      metrics: metrics,
+      connpool_type: 'redis_connection_pool',
+      connpool_provider: 'testprovider',
+      size: 1,
+      timeout: 5
+    ) { MockRedis.new }
+  end
 
   subject { Vmpooler::PoolManager::Provider::Gce.new(config, logger, metrics, redis_connection_pool, 'gce', provider_options) }
 
@@ -59,26 +63,26 @@ EOT
 
   describe '#manual tests live' do
     skip 'runs in gce' do
-      puts "creating"
+      puts 'creating'
       result = subject.create_vm(poolname, vmname)
       subject.get_vm(poolname, vmname)
       subject.vms_in_pool(poolname)
 
-      puts "create snapshot w/ one disk"
-      result = subject.create_snapshot(poolname, vmname, "sams")
-      puts "create disk"
+      puts 'create snapshot w/ one disk'
+      result = subject.create_snapshot(poolname, vmname, 'sams')
+      puts 'create disk'
       result = subject.create_disk(poolname, vmname, 10)
-      puts "create snapshot w/ 2 disks"
-      result = subject.create_snapshot(poolname, vmname, "sams2")
-      puts "revert snapshot"
-      result = subject.revert_snapshot(poolname, vmname, "sams")
+      puts 'create snapshot w/ 2 disks'
+      result = subject.create_snapshot(poolname, vmname, 'sams2')
+      puts 'revert snapshot'
+      result = subject.revert_snapshot(poolname, vmname, 'sams')
       result = subject.destroy_vm(poolname, vmname)
     end
 
     skip 'runs existing' do
-      #result = subject.create_snapshot(poolname, vmname, "sams")
-      #result = subject.revert_snapshot(poolname, vmname, "sams")
-      #puts subject.get_vm(poolname, vmname)
+      # result = subject.create_snapshot(poolname, vmname, "sams")
+      # result = subject.revert_snapshot(poolname, vmname, "sams")
+      # puts subject.get_vm(poolname, vmname)
       result = subject.destroy_vm(poolname, vmname)
     end
 
@@ -105,11 +109,13 @@ EOT
     end
 
     context 'Given a pool folder with many VMs' do
-      let(:expected_vm_list) {[
-        { 'name' => 'vm1'},
-        { 'name' => 'vm2'},
-        { 'name' => 'vm3'}
-      ]}
+      let(:expected_vm_list) do
+        [
+          { 'name' => 'vm1' },
+          { 'name' => 'vm2' },
+          { 'name' => 'vm3' }
+        ]
+      end
       before(:each) do
         instance_list = MockInstanceList.new(items: [])
         expected_vm_list.each do |vm_hash|
@@ -126,7 +132,6 @@ EOT
         expect(result).to eq(expected_vm_list)
       end
     end
-
   end
 
   describe '#get_vm' do
@@ -136,8 +141,8 @@ EOT
 
     context 'when VM does not exist' do
       it 'should return nil' do
-        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404,"The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
-        expect(subject.get_vm(poolname,vmname)).to be_nil
+        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404, "The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
+        expect(subject.get_vm(poolname, vmname)).to be_nil
       end
     end
 
@@ -147,18 +152,18 @@ EOT
       end
 
       it 'should return a hash' do
-        expect(subject.get_vm(poolname,vmname)).to be_kind_of(Hash)
+        expect(subject.get_vm(poolname, vmname)).to be_kind_of(Hash)
       end
 
       it 'should return the VM name' do
-        result = subject.get_vm(poolname,vmname)
+        result = subject.get_vm(poolname, vmname)
 
         expect(result['name']).to eq(vmname)
       end
 
-      ['hostname','boottime','zone','status'].each do |testcase|
+      %w[hostname boottime zone status].each do |testcase|
         it "should return nil for #{testcase}" do
-          result = subject.get_vm(poolname,vmname)
+          result = subject.get_vm(poolname, vmname)
 
           expect(result[testcase]).to be_nil
         end
@@ -168,52 +173,53 @@ EOT
     context 'when VM exists and contains all information' do
       let(:vm_hostname) { "#{vmname}.demo.local" }
       let(:boot_time) { Time.now }
-      let(:vm_object) { MockInstance.new(
+      let(:vm_object) do
+        MockInstance.new(
           name: vmname,
           hostname: vm_hostname,
-          labels: {'pool' => poolname},
+          labels: { 'pool' => poolname },
           creation_timestamp: boot_time,
           status: 'RUNNING',
           zone: zone,
           machine_type: "zones/#{zone}/machineTypes/e2-micro"
         )
-      }
-      let(:pool_info) { config[:pools][0]}
+      end
+      let(:pool_info) { config[:pools][0] }
 
       before(:each) do
         allow(connection).to receive(:get_instance).and_return(vm_object)
       end
 
       it 'should return a hash' do
-        expect(subject.get_vm(poolname,vmname)).to be_kind_of(Hash)
+        expect(subject.get_vm(poolname, vmname)).to be_kind_of(Hash)
       end
 
       it 'should return the VM name' do
-        result = subject.get_vm(poolname,vmname)
+        result = subject.get_vm(poolname, vmname)
 
         expect(result['name']).to eq(vmname)
       end
 
       it 'should return the VM hostname' do
-        result = subject.get_vm(poolname,vmname)
+        result = subject.get_vm(poolname, vmname)
 
         expect(result['hostname']).to eq(vm_hostname)
       end
 
       it 'should return the template name' do
-        result = subject.get_vm(poolname,vmname)
+        result = subject.get_vm(poolname, vmname)
 
         expect(result['template']).to eq(pool_info['template'])
       end
 
       it 'should return the pool name' do
-        result = subject.get_vm(poolname,vmname)
+        result = subject.get_vm(poolname, vmname)
 
         expect(result['poolname']).to eq(pool_info['name'])
       end
 
       it 'should return the boot time' do
-        result = subject.get_vm(poolname,vmname)
+        result = subject.get_vm(poolname, vmname)
 
         expect(result['boottime']).to eq(boot_time)
       end
@@ -227,33 +233,30 @@ EOT
 
     context 'Given an invalid pool name' do
       it 'should raise an error' do
-        expect{ subject.create_vm('missing_pool', vmname) }.to raise_error(/missing_pool does not exist/)
+        expect { subject.create_vm('missing_pool', vmname) }.to raise_error(/missing_pool does not exist/)
       end
     end
 
     context 'Given a template VM that does not exist' do
       before(:each) do
         config[:pools][0]['template'] = 'Templates/missing_template'
-=begin
-        result = MockResult.new
-        result.status = "PENDING"
-        errors = MockOperationError
-        errors << MockOperationErrorError.new(code: "foo", message: "it's missing")
-        result.error = errors
-=end
-        allow(connection).to receive(:insert_instance).and_raise(create_google_client_error(404,'The resource \'Templates/missing_template\' was not found'))
+        #         result = MockResult.new
+        #         result.status = "PENDING"
+        #         errors = MockOperationError
+        #         errors << MockOperationErrorError.new(code: "foo", message: "it's missing")
+        #         result.error = errors
+        allow(connection).to receive(:insert_instance).and_raise(create_google_client_error(404, 'The resource \'Templates/missing_template\' was not found'))
       end
 
       it 'should raise an error' do
-        expect{ subject.create_vm(poolname, vmname) }.to raise_error(Google::Apis::ClientError)
+        expect { subject.create_vm(poolname, vmname) }.to raise_error(Google::Apis::ClientError)
       end
     end
 
     context 'Given a successful creation' do
-
       before(:each) do
         result = MockResult.new
-        result.status = "DONE"
+        result.status = 'DONE'
         allow(connection).to receive(:insert_instance).and_return(result)
       end
 
@@ -264,7 +267,6 @@ EOT
         expect(result.is_a?(Hash)).to be true
       end
 
-
       it 'should have the new VM name' do
         instance = MockInstance.new(name: vmname)
         allow(connection).to receive(:get_instance).and_return(instance)
@@ -282,7 +284,7 @@ EOT
 
     context 'Given a missing VM name' do
       before(:each) do
-        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404,"The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
+        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404, "The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
         disk_list = MockDiskList.new(items: nil)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(subject).to receive(:find_all_snapshots).and_return(nil)
@@ -299,7 +301,7 @@ EOT
         instance = MockInstance.new(name: vmname)
         allow(connection).to receive(:get_instance).and_return(instance)
         result = MockResult.new
-        result.status = "DONE"
+        result.status = 'DONE'
         allow(subject).to receive(:wait_for_operation).and_return(result)
         allow(connection).to receive(:delete_instance).and_return(result)
       end
@@ -333,7 +335,6 @@ EOT
         subject.destroy_vm(poolname, vmname)
       end
     end
-
   end
 
   describe '#vm_ready?' do
@@ -344,17 +345,17 @@ EOT
       end
 
       it 'should return true' do
-        expect(subject.vm_ready?(poolname,vmname)).to be true
+        expect(subject.vm_ready?(poolname, vmname)).to be true
       end
     end
 
     context 'When an error occurs connecting to the VM' do
       before(:each) do
-        expect(subject).to receive(:open_socket).and_raise(RuntimeError,'MockError')
+        expect(subject).to receive(:open_socket).and_raise(RuntimeError, 'MockError')
       end
 
       it 'should return false' do
-        expect(subject.vm_ready?(poolname,vmname)).to be false
+        expect(subject.vm_ready?(poolname, vmname)).to be false
       end
     end
   end
@@ -367,17 +368,17 @@ EOT
 
     context 'Given an invalid pool name' do
       it 'should raise an error' do
-        expect{ subject.create_disk('missing_pool',vmname,disk_size) }.to raise_error(/missing_pool does not exist/)
+        expect { subject.create_disk('missing_pool', vmname, disk_size) }.to raise_error(/missing_pool does not exist/)
       end
     end
 
     context 'when VM does not exist' do
       before(:each) do
-        expect(connection).to receive(:get_instance).and_raise(create_google_client_error(404,"The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
+        expect(connection).to receive(:get_instance).and_raise(create_google_client_error(404, "The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
       end
 
       it 'should raise an error' do
-        expect{ subject.create_disk(poolname,vmname,disk_size) }.to raise_error(/VM #{vmname} .+ does not exist/)
+        expect { subject.create_disk(poolname, vmname, disk_size) }.to raise_error(/VM #{vmname} .+ does not exist/)
       end
     end
 
@@ -386,11 +387,11 @@ EOT
         disk = MockDisk.new(name: vmname)
         instance = MockInstance.new(name: vmname, disks: [disk])
         allow(connection).to receive(:get_instance).and_return(instance)
-        expect(connection).to receive(:insert_disk).and_raise(RuntimeError,'Mock Disk Error')
+        expect(connection).to receive(:insert_disk).and_raise(RuntimeError, 'Mock Disk Error')
       end
 
       it 'should raise an error' do
-        expect{ subject.create_disk(poolname,vmname,disk_size) }.to raise_error(/Mock Disk Error/)
+        expect { subject.create_disk(poolname, vmname, disk_size) }.to raise_error(/Mock Disk Error/)
       end
     end
 
@@ -400,7 +401,7 @@ EOT
         instance = MockInstance.new(name: vmname, disks: [disk])
         allow(connection).to receive(:get_instance).and_return(instance)
         result = MockResult.new
-        result.status = "DONE"
+        result.status = 'DONE'
         allow(connection).to receive(:insert_disk).and_return(result)
         allow(subject).to receive(:wait_for_operation).and_return(result)
         new_disk = MockDisk.new(name: "#{vmname}-disk1", self_link: "/foo/bar/baz/#{vmname}-disk1")
@@ -409,7 +410,7 @@ EOT
       end
 
       it 'should return true' do
-        expect(subject.create_disk(poolname,vmname,disk_size)).to be true
+        expect(subject.create_disk(poolname, vmname, disk_size)).to be true
       end
     end
   end
@@ -423,11 +424,11 @@ EOT
 
     context 'when VM does not exist' do
       before(:each) do
-        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404,"The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
+        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404, "The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
       end
 
       it 'should raise an error' do
-        expect{ subject.create_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/VM #{vmname} .+ does not exist/)
+        expect { subject.create_snapshot(poolname, vmname, snapshot_name) }.to raise_error(/VM #{vmname} .+ does not exist/)
       end
     end
 
@@ -438,7 +439,7 @@ EOT
         allow(connection).to receive(:get_instance).and_return(instance)
         snapshots = [MockSnapshot.new(name: snapshot_name)]
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
-        expect{ subject.create_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/Snapshot #{snapshot_name} .+ already exists /)
+        expect { subject.create_snapshot(poolname, vmname, snapshot_name) }.to raise_error(/Snapshot #{snapshot_name} .+ already exists /)
       end
     end
 
@@ -449,11 +450,11 @@ EOT
         allow(connection).to receive(:get_instance).and_return(instance)
         snapshots = nil
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
-        allow(connection).to receive(:create_disk_snapshot).and_raise(RuntimeError,'Mock Snapshot Error')
+        allow(connection).to receive(:create_disk_snapshot).and_raise(RuntimeError, 'Mock Snapshot Error')
       end
 
       it 'should raise an error' do
-        expect{ subject.create_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/Mock Snapshot Error/)
+        expect { subject.create_snapshot(poolname, vmname, snapshot_name) }.to raise_error(/Mock Snapshot Error/)
       end
     end
 
@@ -465,12 +466,12 @@ EOT
         snapshots = nil
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
         result = MockResult.new
-        result.status = "DONE"
+        result.status = 'DONE'
         allow(connection).to receive(:create_disk_snapshot).and_return(result)
       end
 
       it 'should return true' do
-        expect(subject.create_snapshot(poolname,vmname,snapshot_name)).to be true
+        expect(subject.create_snapshot(poolname, vmname, snapshot_name)).to be true
       end
 
       it 'should snapshot each attached disk' do
@@ -480,7 +481,7 @@ EOT
         allow(connection).to receive(:get_instance).and_return(instance)
 
         expect(connection.should_receive(:create_disk_snapshot).twice)
-        subject.create_snapshot(poolname,vmname,snapshot_name)
+        subject.create_snapshot(poolname, vmname, snapshot_name)
       end
     end
   end
@@ -494,11 +495,11 @@ EOT
 
     context 'when VM does not exist' do
       before(:each) do
-        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404,"The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
+        allow(connection).to receive(:get_instance).and_raise(create_google_client_error(404, "The resource 'projects/#{project}/zones/#{zone}/instances/#{vmname}' was not found"))
       end
 
       it 'should raise an error' do
-        expect{ subject.revert_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/VM #{vmname} .+ does not exist/)
+        expect { subject.revert_snapshot(poolname, vmname, snapshot_name) }.to raise_error(/VM #{vmname} .+ does not exist/)
       end
     end
 
@@ -509,7 +510,7 @@ EOT
         allow(connection).to receive(:get_instance).and_return(instance)
         snapshots = nil
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
-        expect{ subject.revert_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/Snapshot #{snapshot_name} .+ does not exist /)
+        expect { subject.revert_snapshot(poolname, vmname, snapshot_name) }.to raise_error(/Snapshot #{snapshot_name} .+ does not exist /)
       end
     end
 
@@ -524,7 +525,7 @@ EOT
         allow(connection).to receive(:start_instance)
         expect(subject).not_to receive(:detach_disk)
         expect(subject).not_to receive(:delete_disk)
-        subject.revert_snapshot(poolname,vmname,snapshot_name)
+        subject.revert_snapshot(poolname, vmname, snapshot_name)
       end
     end
 
@@ -537,11 +538,11 @@ EOT
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
         allow(connection).to receive(:stop_instance)
         allow(subject).to receive(:wait_for_operation)
-        expect(connection).to receive(:detach_disk).and_raise(RuntimeError,'Mock Snapshot Error')
+        expect(connection).to receive(:detach_disk).and_raise(RuntimeError, 'Mock Snapshot Error')
       end
 
       it 'should raise an error' do
-        expect{ subject.revert_snapshot(poolname,vmname,snapshot_name) }.to raise_error(/Mock Snapshot Error/)
+        expect { subject.revert_snapshot(poolname, vmname, snapshot_name) }.to raise_error(/Mock Snapshot Error/)
       end
     end
 
@@ -550,7 +551,7 @@ EOT
         attached_disk = MockAttachedDisk.new(device_name: vmname, source: "foo/bar/baz/#{vmname}")
         instance = MockInstance.new(name: vmname, disks: [attached_disk])
         allow(connection).to receive(:get_instance).and_return(instance)
-        snapshots = [MockSnapshot.new(name: snapshot_name, self_link: "foo/bar/baz/snapshot/#{snapshot_name}", labels: {"diskname" => vmname})]
+        snapshots = [MockSnapshot.new(name: snapshot_name, self_link: "foo/bar/baz/snapshot/#{snapshot_name}", labels: { 'diskname' => vmname })]
         allow(subject).to receive(:find_snapshot).and_return(snapshots)
         allow(connection).to receive(:stop_instance)
         allow(subject).to receive(:wait_for_operation)
@@ -564,7 +565,7 @@ EOT
       end
 
       it 'should return true' do
-        expect(subject.revert_snapshot(poolname,vmname,snapshot_name)).to be true
+        expect(subject.revert_snapshot(poolname, vmname, snapshot_name)).to be true
       end
     end
   end
@@ -581,7 +582,7 @@ EOT
         allow(subject).to receive(:wait_for_zone_operation)
       end
       it 'should attempt to delete unconfigured instances when they dont have a label' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo")])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo')])
         disk_list = MockDiskList.new(items: nil)
         snapshot_list = MockSnapshotList.new(items: nil)
         # the instance_list is filtered in the real code, and should only return non-configured VMs based on labels
@@ -593,7 +594,7 @@ EOT
         subject.purge_unconfigured_resources(nil)
       end
       it 'should attempt to delete unconfigured instances when they have a label that is not a configured pool' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "foobar"})])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'pool' => 'foobar' })])
         disk_list = MockDiskList.new(items: nil)
         snapshot_list = MockSnapshotList.new(items: nil)
         allow(connection).to receive(:list_instances).and_return(instance_list)
@@ -604,8 +605,8 @@ EOT
       end
       it 'should attempt to delete unconfigured disks and snapshots when they do not have a label' do
         instance_list = MockInstanceList.new(items: nil)
-        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo")])
-        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo")])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: 'diskfoo')])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: 'snapfoo')])
         allow(connection).to receive(:list_instances).and_return(instance_list)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(connection).to receive(:list_snapshots).and_return(snapshot_list)
@@ -618,10 +619,10 @@ EOT
     context 'with allowlist containing a pool name' do
       before(:each) do
         allow(subject).to receive(:wait_for_zone_operation)
-        $allowlist = ["allowed"]
+        $allowlist = ['allowed']
       end
       it 'should attempt to delete unconfigured instances when they dont have the allowlist label' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "not_this"})])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'pool' => 'not_this' })])
         disk_list = MockDiskList.new(items: nil)
         snapshot_list = MockSnapshotList.new(items: nil)
         allow(connection).to receive(:list_instances).and_return(instance_list)
@@ -631,7 +632,7 @@ EOT
         subject.purge_unconfigured_resources($allowlist)
       end
       it 'should ignore unconfigured instances when they have a label that is allowed' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "allowed"})])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'pool' => 'allowed' })])
         disk_list = MockDiskList.new(items: nil)
         snapshot_list = MockSnapshotList.new(items: nil)
         allow(connection).to receive(:list_instances).and_return(instance_list)
@@ -642,8 +643,8 @@ EOT
       end
       it 'should ignore unconfigured disks and snapshots when they have a label that is allowed' do
         instance_list = MockInstanceList.new(items: nil)
-        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"pool" => "allowed"})])
-        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo", labels: {"pool" => "allowed"})])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: 'diskfoo', labels: { 'pool' => 'allowed' })])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: 'snapfoo', labels: { 'pool' => 'allowed' })])
         allow(connection).to receive(:list_instances).and_return(instance_list)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(connection).to receive(:list_snapshots).and_return(snapshot_list)
@@ -652,10 +653,10 @@ EOT
         subject.purge_unconfigured_resources($allowlist)
       end
       it 'should ignore unconfigured item when they have the empty label that is allowed, which means we allow the pool label to not be set' do
-        $allowlist = ["allowed", ""]
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"some" => "not_important"})])
-        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"other" => "thing"})])
-        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo")])
+        $allowlist = ['allowed', '']
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'some' => 'not_important' })])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: 'diskfoo', labels: { 'other' => 'thing' })])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: 'snapfoo')])
         allow(connection).to receive(:list_instances).and_return(instance_list)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(connection).to receive(:list_snapshots).and_return(snapshot_list)
@@ -669,10 +670,10 @@ EOT
     context 'with allowlist containing a pool name and the empty string' do
       before(:each) do
         allow(subject).to receive(:wait_for_zone_operation)
-        $allowlist = ["allowed", ""]
+        $allowlist = ['allowed', '']
       end
       it 'should attempt to delete unconfigured instances when they dont have the allowlist label' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "not_this"})])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'pool' => 'not_this' })])
         disk_list = MockDiskList.new(items: nil)
         snapshot_list = MockSnapshotList.new(items: nil)
         allow(connection).to receive(:list_instances).and_return(instance_list)
@@ -683,8 +684,8 @@ EOT
       end
       it 'should ignore unconfigured disks and snapshots when they have a label that is allowed' do
         instance_list = MockInstanceList.new(items: nil)
-        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"pool" => "allowed"})])
-        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo", labels: {"pool" => "allowed"})])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: 'diskfoo', labels: { 'pool' => 'allowed' })])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: 'snapfoo', labels: { 'pool' => 'allowed' })])
         allow(connection).to receive(:list_instances).and_return(instance_list)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(connection).to receive(:list_snapshots).and_return(snapshot_list)
@@ -693,9 +694,9 @@ EOT
         subject.purge_unconfigured_resources($allowlist)
       end
       it 'should ignore unconfigured item when they have the empty label that is allowed, which means we allow the pool label to not be set' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"some" => "not_important"})])
-        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"other" => "thing"})])
-        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo")])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'some' => 'not_important' })])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: 'diskfoo', labels: { 'other' => 'thing' })])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: 'snapfoo')])
         allow(connection).to receive(:list_instances).and_return(instance_list)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(connection).to receive(:list_snapshots).and_return(snapshot_list)
@@ -709,10 +710,10 @@ EOT
     context 'with allowlist containing a a fully qualified label that is not pool' do
       before(:each) do
         allow(subject).to receive(:wait_for_zone_operation)
-        $allowlist = ["user=Bob"]
+        $allowlist = ['user=Bob']
       end
       it 'should attempt to delete unconfigured instances when they dont have the allowlist label' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"pool" => "not_this"})])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'pool' => 'not_this' })])
         disk_list = MockDiskList.new(items: nil)
         snapshot_list = MockSnapshotList.new(items: nil)
         allow(connection).to receive(:list_instances).and_return(instance_list)
@@ -722,9 +723,9 @@ EOT
         subject.purge_unconfigured_resources($allowlist)
       end
       it 'should ignore unconfigured item when they match the fully qualified label' do
-        instance_list = MockInstanceList.new(items: [MockInstance.new(name: "foo", labels: {"some" => "not_important", "user" => "bob"})])
-        disk_list = MockDiskList.new(items: [MockDisk.new(name: "diskfoo", labels: {"other" => "thing", "user" => "bob"})])
-        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: "snapfoo", labels: {"user" => "bob"})])
+        instance_list = MockInstanceList.new(items: [MockInstance.new(name: 'foo', labels: { 'some' => 'not_important', 'user' => 'bob' })])
+        disk_list = MockDiskList.new(items: [MockDisk.new(name: 'diskfoo', labels: { 'other' => 'thing', 'user' => 'bob' })])
+        snapshot_list = MockSnapshotList.new(items: [MockSnapshot.new(name: 'snapfoo', labels: { 'user' => 'bob' })])
         allow(connection).to receive(:list_instances).and_return(instance_list)
         allow(connection).to receive(:list_disks).and_return(disk_list)
         allow(connection).to receive(:list_snapshots).and_return(snapshot_list)
@@ -737,23 +738,22 @@ EOT
 
     it 'should raise any errors' do
       expect(subject).to receive(:provided_pools).and_throw('mockerror')
-      expect{ subject.purge_unconfigured_resources(nil) }.to raise_error(/mockerror/)
+      expect { subject.purge_unconfigured_resources(nil) }.to raise_error(/mockerror/)
     end
   end
 
   describe '#get_current_user' do
     it 'should downcase and replace invalid chars with dashes' do
       redis_connection_pool.with_metrics do |redis|
-        redis.hset("vmpooler__vm__#{vmname}", 'token:user', "BOBBY.PUPPET")
-        expect(subject.get_current_user(vmname)).to eq("bobby-puppet")
+        redis.hset("vmpooler__vm__#{vmname}", 'token:user', 'BOBBY.PUPPET')
+        expect(subject.get_current_user(vmname)).to eq('bobby-puppet')
       end
     end
 
     it 'returns "" for nil values' do
-      redis_connection_pool.with_metrics do |redis|
-        expect(subject.get_current_user(vmname)).to eq("")
+      redis_connection_pool.with_metrics do |_redis|
+        expect(subject.get_current_user(vmname)).to eq('')
       end
     end
   end
-  
 end