Fix rubocop errors

and update RuboCop config to fail on error

Signed-off-by: Joseph Palermo <[email protected]>
Co-authored-by: Joseph Palermo <[email protected]>

src/bosh_openstack_cpi/Rakefile

@@ -7,6 +7,4 @@ namespace :spec do
   end
 end
 
-RuboCop::RakeTask.new do |t|
-  t.options = ['--fail-level=E'] # only fail on `Error`
-end
+RuboCop::RakeTask.new

src/bosh_openstack_cpi/lib/cloud/openstack.rb

@@ -5,11 +5,9 @@ module OpenStackCloud; end
 require 'fog/openstack'
 require 'httpclient'
 require 'json'
-require 'pp'
 require 'set'
 require 'tmpdir'
 require 'securerandom'
-require 'json'
 require 'membrane'
 require 'netaddr'
 

src/bosh_openstack_cpi/lib/cloud/openstack/cloud.rb

@@ -23,6 +23,8 @@ class Cloud < Bosh::Cloud
     # @option options [Hash] registry agent options
     # @option cpi_api_version CPI API Version as requested by BOSH director
     def initialize(options, cpi_api_version)
+      super()
+
       @cpi_api_version = cpi_api_version || 1
       @options = normalize_options(options)
 
@@ -634,7 +636,7 @@ def soft_reboot(server)
     # @return [void]
     def hard_reboot(server)
       @logger.info("Hard rebooting server `#{server.id}'...")
-      @openstack.with_openstack { server.reboot(type = 'HARD') }
+      @openstack.with_openstack { server.reboot('HARD') }
       @openstack.wait_resource(server, :active, :state)
     end
 

src/bosh_openstack_cpi/lib/cloud/openstack/dynamic_network.rb

@@ -2,15 +2,6 @@ module Bosh::OpenStackCloud
   ##
   # Represents OpenStack dynamic network: where IaaS sets VM's IP
   class DynamicNetwork < PrivateNetwork
-    ##
-    # Creates a new dynamic network
-    #
-    # @param [String] name Network name
-    # @param [Hash] spec Raw network spec
-    def initialize(name, spec)
-      super
-    end
-
     def prepare(_openstack, _security_group_ids)
       cloud_error("Network with id '#{net_id}' is a dynamic network. VRRP is not supported for dynamic networks") if @allowed_address_pairs
     end

src/bosh_openstack_cpi/lib/cloud/openstack/floating_ip.rb

@@ -20,7 +20,6 @@ def self.reassociate(openstack, ip, server, network_id)
       end
     end
 
-    private
 
     def self.port_attached?(floating_ip)
       return false if floating_ip['port_id'].nil? || floating_ip['port_id'].empty?

src/bosh_openstack_cpi/lib/cloud/openstack/loadbalancer_configurator.rb

@@ -35,7 +35,7 @@ def create_membership(pool_id, ip, port, subnet_id)
           membership_id = retry_on_conflict_pending_update(pool_id) {
             @openstack.network.create_lbaas_pool_member(pool_id, ip, port, subnet_id:).body['member']['id']
           }
-        rescue Excon::Error::Conflict => e
+        rescue Excon::Error::Conflict
           lbaas_pool_members = @openstack.with_openstack(retryable: true) { @openstack.network.list_lbaas_pool_members(pool_id) }
           membership_id =
             lbaas_pool_members
@@ -89,19 +89,18 @@ def remove_vm_from_pool(pool_id, membership_id)
     end
 
     def retry_on_conflict_pending_update(pool_id)
-      update_complete = false
       action_result = nil
       start_time = Time.now
       attempts = 0
 
       loadbalancer_id = loadbalancer_id(pool_id)
       resource = LoadBalancerResource.new(loadbalancer_id, @openstack)
 
-      begin
+      loop do
         @openstack.wait_resource(resource, :active, :provisioning_status)
         attempts += 1
         action_result = yield
-        update_complete = true
+        break
       rescue Excon::Error::Conflict => e
         neutron_error = @openstack.parse_openstack_response(e.response, 'NeutronError')
         if neutron_error&.fetch('message', '')&.include? 'PENDING_UPDATE'
@@ -112,7 +111,7 @@ def retry_on_conflict_pending_update(pool_id)
         else
           raise e
         end
-      end until update_complete
+      end
       @openstack.wait_resource(resource, :active, :provisioning_status)
 
       action_result

src/bosh_openstack_cpi/lib/cloud/openstack/manual_network.rb

@@ -79,7 +79,7 @@ def create_port_for_manual_network(openstack, net_id, security_group_ids)
 
     def delete_conflicting_unused_ports(openstack, net_id)
       ports = openstack.with_openstack(retryable: true) do
-        openstack.network.ports.all("fixed_ips": ["ip_address=#{@ip}", "network_id": net_id])
+        openstack.network.ports.all(fixed_ips: ["ip_address=#{@ip}", network_id: net_id])
       end
       detached_port_ids = ports.select { |p| p.status == 'DOWN' && p.device_id.empty? && p.device_owner.empty? }.map(&:id)
       @logger.warn("IP #{@ip} already allocated: Deleting conflicting unused ports with ids=#{detached_port_ids}")

src/bosh_openstack_cpi/lib/cloud/openstack/network_configurator.rb

@@ -74,9 +74,8 @@ def cleanup(openstack)
     ##
     # Setup network configuration for one network spec.
     #
-    # @param [String] network spec name
-    # @param [Hash] network spec
-    #   configure
+    # @param [String] name
+    # @param [Hash] network_spec
     def initialize_network(name, network_spec)
       network_type = NetworkConfigurator.network_type(network_spec)
 
@@ -153,6 +152,28 @@ def self.gateway_ip(network_spec, openstack, server)
       end
     end
 
+    def self.network_type(network)
+      # in case of a manual network bosh doesn't provide a type.
+      network.fetch('type', 'manual')
+    end
+
+    def self.private_network_specs(network_spec)
+      network_spec.values.reject { |spec| spec['type'] == 'vip' }
+    end
+
+    ##
+    # Extracts the network ID from the network configuration
+    #
+    # @param [Hash] network_spec Network specification
+    # @return [Hash] network ID
+    def self.extract_net_id(network_spec)
+      if network_spec && network_spec['cloud_properties']
+        cloud_properties = network_spec['cloud_properties']
+        return cloud_properties['net_id'] if cloud_properties&.key?('net_id')
+      end
+      nil
+    end
+
     ##
     # Applies network configuration to the vm
     #
@@ -218,15 +239,6 @@ def default_network?(network)
       network.spec == default_network_spec
     end
 
-    def self.network_type(network)
-      # in case of a manual network bosh doesn't provide a type.
-      network.fetch('type', 'manual')
-    end
-
-    def self.private_network_specs(network_spec)
-      network_spec.values.reject { |spec| spec['type'] == 'vip' }
-    end
-
     def multiple_private_networks?
       @networks.length > 1
     end
@@ -247,18 +259,5 @@ def extract_security_groups(network_spec)
       end
       []
     end
-
-    ##
-    # Extracts the network ID from the network configuration
-    #
-    # @param [Hash] network_spec Network specification
-    # @return [Hash] network ID
-    def self.extract_net_id(network_spec)
-      if network_spec && network_spec['cloud_properties']
-        cloud_properties = network_spec['cloud_properties']
-        return cloud_properties['net_id'] if cloud_properties&.key?('net_id')
-      end
-      nil
-    end
   end
 end

src/bosh_openstack_cpi/lib/cloud/openstack/noop_registry.rb

@@ -12,6 +12,7 @@ def read_settings(_instance_id)
 
     def delete_settings(instance_id) end
 
-    def endpoint() end
+    def endpoint
+    end
   end
 end

src/bosh_openstack_cpi/lib/cloud/openstack/openstack.rb

@@ -66,7 +66,7 @@ def with_openstack(retryable: false, ignore_not_found: false)
         raise_openstack_error(e, format_error_body(e))
       rescue *retryable_errors => e
         raise_openstack_error(e) unless retries < MAX_RETRIES &&
-                                        (dns_problem?(e) || connectivity_problem?(e) && retryable || server_problem?(e))
+                                        (dns_problem?(e) || (connectivity_problem?(e) && retryable) || server_problem?(e))
 
         retries += 1
         log_retry_error(e, retries)
@@ -323,7 +323,7 @@ def determine_message(body)
       hash_with_msg_property = proc { |_k, v| (v.is_a? Hash) && v['message'] }
 
       body ||= {}
-      _, value = body.find &hash_with_msg_property
+      _, value = body.find(&hash_with_msg_property)
       value ? "(#{value['message']})" : ''
     end
 

src/bosh_openstack_cpi/lib/cloud/openstack/server.rb

@@ -222,7 +222,6 @@ def with_dns(network_spec)
       network_spec.each_value do |properties|
         if properties.key?('dns') && !properties['dns'].nil?
           yield properties['dns']
-          return
         end
       end
     end

src/bosh_openstack_cpi/lib/cloud/openstack/vip_network.rb

@@ -3,15 +3,6 @@ module Bosh::OpenStackCloud
   # Represents OpenStack vip network: where users sets VM's IP (floating IP's
   # in OpenStack)
   class VipNetwork < Network
-    ##
-    # Creates a new vip network
-    #
-    # @param [String] name Network name
-    # @param [Hash] spec Raw network spec
-    def initialize(name, spec)
-      super
-    end
-
     ##
     # Configures OpenStack vip network
     #

src/bosh_openstack_cpi/spec/integration/lifecycle_spec.rb

@@ -156,7 +156,7 @@
         @multiple_nics_vm_id = create_vm(@stemcell_id, multiple_network_spec, [])
 
         vm = openstack.compute.servers.get(@multiple_nics_vm_id)
-        network_interfaces = vm.addresses.map { |_, network_interfaces| network_interfaces }.flatten
+        network_interfaces = vm.addresses.map { |_, interfaces| interfaces }.flatten
         network_interface_1 = network_interfaces.find(&where_ip_address_is(@config.no_dhcp_manual_ip_1))
         network_interface_2 = network_interfaces.find(&where_ip_address_is(@config.no_dhcp_manual_ip_2))
 
@@ -199,7 +199,7 @@ def where_ip_address_is(ip)
 
       def clean_up_port(ip, net_id)
         ports = openstack.with_openstack(retryable: true) do
-          openstack.network.ports.all("fixed_ips": ["ip_address=#{ip}", "network_id": net_id])
+          openstack.network.ports.all(fixed_ips: ["ip_address=#{ip}", network_id: net_id])
         end
 
         port_ids = ports.select { |p| p.status == 'DOWN' && p.device_id.empty? && p.device_owner.empty? }.map(&:id)
@@ -295,7 +295,6 @@ def test_boot_volume(resource_pool = {})
   end
 
   context 'when using cloud_properties and specifying security groups' do
-    let(:security_group) {}
     let(:network_spec) do
       {
         'default' => {

src/bosh_openstack_cpi/spec/integration/support/integration_config.rb

@@ -86,7 +86,7 @@ def initialize(identity_version = :v3)
     Bosh::Clouds::Config.configure(OpenStruct.new(logger: @logger, cpi_task_log: nil))
   end
 
-  def create_cpi(boot_from_volume: false, config_drive: nil, human_readable_vm_names: false, use_nova_networking: false, use_dhcp: true, default_volume_type: nil, request_id: nil)
+  def create_cpi(boot_from_volume: false, config_drive: nil, human_readable_vm_names: false, use_nova_networking: false, use_dhcp: true, default_volume_type: nil)
     properties = {
       'openstack' => openstack_properties(boot_from_volume, config_drive, human_readable_vm_names, use_nova_networking, use_dhcp, default_volume_type),
     }

src/bosh_openstack_cpi/spec/integration/support/lifecycle_helpers.rb

@@ -25,7 +25,8 @@ def vm_lifecycle(stemcell_id, network_spec, disk_id = nil, resource_pool = {})
       cpi.detach_disk(vm_id, disk_id)
 
       disk_snapshot_id = create_disk_snapshot(disk_id) unless @config.disable_snapshots
-    rescue Exception => create_error
+    rescue StandardError => create_error
+      # intentionally blank
     ensure
       funcs = [
         -> { clean_up_disk(disk_id) },
@@ -112,7 +113,7 @@ def run_all_and_raise_any_errors(existing_errors, funcs)
       funcs.each do |f|
         begin
           f.call
-        rescue Exception => e
+        rescue StandardError => e
           exceptions << e
         end
       end

src/bosh_openstack_cpi/spec/unit/cloud_spec.rb

@@ -297,7 +297,9 @@
         expect(cpi.registry).to receive(:read_settings).with('name')
         expect(cpi.registry).to receive(:update_settings).with('name', anything)
 
-        cpi.update_agent_settings(server) {}
+        cpi.update_agent_settings(server) {
+          # intentionally empty
+        }
       end
     end
 
@@ -311,7 +313,9 @@
         expect(cpi.registry).to receive(:read_settings).with('registry-tag-value')
         expect(cpi.registry).to receive(:update_settings).with('registry-tag-value', anything)
 
-        cpi.update_agent_settings(server) {}
+        cpi.update_agent_settings(server) {
+          # intentionally empty
+        }
       end
     end
 
@@ -325,7 +329,9 @@
 
         expect(cpi.logger).to receive(:info).with("Updating settings for server 'id' with registry key 'name'...")
 
-        cpi.update_agent_settings(server) {}
+        cpi.update_agent_settings(server) {
+          # intentionally empty
+        }
       end
     end
 
@@ -345,7 +351,9 @@
         allow(server.metadata).to receive(:get).with(:registry_key).and_return(nil)
         expect(cpi.logger).to_not receive(:info)
 
-        cpi.update_agent_settings(server) {}
+        cpi.update_agent_settings(server) {
+          # intentionally empty
+        }
       end
     end
   end

src/bosh_openstack_cpi/spec/unit/cpi_lambda_spec.rb

@@ -31,7 +31,7 @@
       it 'raises an error' do
         expect {
           subject.call({}, 1)
-        }.to raise_error /Could not find cloud properties in the configuration/
+        }.to raise_error(/Could not find cloud properties in the configuration/)
       end
     end
 

src/bosh_openstack_cpi/spec/unit/create_disk_spec.rb

@@ -160,7 +160,7 @@
       description: '',
       size: 1,
     }
-    server = double('server', id: 'i-test',
+    double('server', id: 'i-test',
                               availability_zone: 'foobar-land')
     volume = double('volume', id: 'v-foobar')
 

src/bosh_openstack_cpi/spec/unit/create_vm_spec.rb

@@ -557,10 +557,10 @@ def user_data(unique_name, network_spec, nameserver = nil, openssh = false)
 
       expect {
         cloud.create_vm('agent-id', 'sc-id', resource_pool_spec, { 'network_a' => dynamic_network_spec }, nil, environment)
-      }.to raise_error { |error|
+      }.to(raise_error { |error|
              expect(error).to be_a(Bosh::Clouds::VMCreationFailed)
              expect(error.ok_to_retry).to eq(false)
-           }
+           })
       expect(cloud.openstack).to have_received(:wait_resource).with(server, %i[terminated deleted], :state, true)
     end
 
@@ -569,10 +569,10 @@ def user_data(unique_name, network_spec, nameserver = nil, openssh = false)
 
       expect {
         cloud.create_vm('agent-id', 'sc-id', resource_pool_spec, { 'network_a' => dynamic_network_spec }, nil, environment)
-      }.to raise_error { |error|
+      }.to(raise_error { |error|
              expect(error).to be_a(Bosh::Clouds::VMCreationFailed)
              expect(error.ok_to_retry).to eq(false)
-           }
+           })
       expect(cloud.openstack).to have_received(:wait_resource).with(server, %i[terminated deleted], :state, true)
     end
 
@@ -714,7 +714,7 @@ def user_data(unique_name, network_spec, nameserver = nil, openssh = false)
 
     context 'when "human_readable_vm_names" is disabled' do
       let(:options) do
-        options = mock_cloud_options['properties']
+        mock_cloud_options['properties']
       end
 
       it 'does not tag server with registry tag' do