Update rubocop config + update bosh template spec tests (#394)

* Update routing's rubocop configuration I tried to run rubocop the other day and it was an absolute mess. This PR updates the rubocop config in routing-release and updates all of the bosh template spec tests appropriately. The update strategy is based on what exists in CAPI and this PR specifically: - cloudfoundry/cloud_controller_ng#3448 * Enable `rubocop-rspec` and regen rubocop_todo.yml We'll get this these... eventually... maybe.
cloudfoundry · Mar 28, 2024 · 7601d8c · 7601d8c
1 parent 7df92e6
commit 7601d8c
Show file tree

Hide file tree

Showing 11 changed files with 244 additions and 90 deletions.
diff --git a/.gitignore b/.gitignore
@@ -3,7 +3,6 @@
 blobs/
 config/dev.yml
 config/private.yml
-/*.yml
 stub.yml
 .final_builds/jobs/**/*.tgz
 .final_builds/packages/**/*.tgz

diff --git a/.rubocop.yml b/.rubocop.yml
@@ -0,0 +1,16 @@
+# We put our own configuration into .rubocop_routing.yml to let it be
+# overwritten buy .rubocop_todo.yml.
+# The .rubocop_todo.yml is autogenerated with `rubocop --regenerate-todo` and
+# ignores old errors for new cops
+# Then one can, one by one, remove to ignore from the .rubocop_todo.yml file
+# and work on applying new cops also to old codebase.
+#
+# This was adapted from [cloudfoundry/cloud_controller_ng](https://github.com/cloudfoundry/cloud_controller_ng/blob/a6febf66cf9cf7c86b27b917df9b111b874b6972/.rubocop.yml)
+
+inherit_from:
+  - .rubocop_routing.yml
+  - .rubocop_todo.yml
+inherit_mode:
+  merge:
+    - Exclude
+    - Include
diff --git a/.rubocop_routing.yml b/.rubocop_routing.yml
@@ -0,0 +1,36 @@
+# Routing Release rubocop config for bosh template tests
+
+require:
+  - rubocop-rspec
+
+AllCops:
+  TargetRubyVersion: 3.2
+  NewCops: enable
+
+Layout/LineLength:
+  Max: 200
+
+Metrics/BlockLength:
+  Exclude:
+    - 'spec/**/*'
+  Max: 50
+
+Metrics/MethodLength:
+  Max: 60
+
+Style/MutableConstant:
+  EnforcedStyle: literals
+
+Style/FrozenStringLiteralComment:
+  EnforcedStyle: always_true
+  Exclude:
+    - 'Gemfile'
+    - 'scripts/staged_shortlog'
+
+Style/Documentation:
+  Exclude:
+    - spec/spec_helper.rb
+
+Style/SelectByRegexp:
+  Exclude:
+    - 'scripts/staged_shortlog'
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -0,0 +1,133 @@
+# This configuration was generated by
+# `rubocop --auto-gen-config`
+# on 2024-03-06 23:41:06 UTC using RuboCop version 1.57.1.
+# The point is for the user to remove these configuration records
+# one by one as the offenses are removed from the code base.
+# Note that changes in the inspected code, or installation of new
+# versions of RuboCop, may require this file to be generated again.
+
+# Offense count: 36
+# This cop supports unsafe autocorrection (--autocorrect-all).
+RSpec/BeEq:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/routing_api_templates_spec.rb'
+
+# Offense count: 50
+# Configuration parameters: Prefixes, AllowedPatterns.
+# Prefixes: when, with, without
+RSpec/ContextWording:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/haproxy_config_spec.rb'
+    - 'spec/routing_api_templates_spec.rb'
+
+# Offense count: 5
+# Configuration parameters: IgnoredMetadata.
+RSpec/DescribeClass:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/haproxy_config_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+    - 'spec/routing_api_templates_spec.rb'
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 14
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: AllowConsecutiveOneLiners.
+RSpec/EmptyLineAfterExample:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+
+# Offense count: 38
+# This cop supports safe autocorrection (--autocorrect).
+RSpec/EmptyLineAfterExampleGroup:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 12
+# This cop supports safe autocorrection (--autocorrect).
+RSpec/EmptyLineAfterFinalLet:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+    - 'spec/routing_api_templates_spec.rb'
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 56
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: AllowConsecutiveOneLiners.
+RSpec/EmptyLineAfterHook:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 13
+# Configuration parameters: CountAsOne.
+RSpec/ExampleLength:
+  Max: 47
+
+# Offense count: 108
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: CustomTransform, IgnoredWords, DisallowedExamples.
+# DisallowedExamples: works
+RSpec/ExampleWording:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+    - 'spec/routing_api_templates_spec.rb'
+    - 'spec/spec_helper.rb'
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 7
+# This cop supports safe autocorrection (--autocorrect).
+RSpec/LeadingSubject:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/routing_api_templates_spec.rb'
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 1
+# This cop supports safe autocorrection (--autocorrect).
+RSpec/MatchArray:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+
+# Offense count: 34
+RSpec/MultipleExpectations:
+  Max: 5
+
+# Offense count: 254
+# Configuration parameters: AllowSubject.
+RSpec/MultipleMemoizedHelpers:
+  Max: 10
+
+# Offense count: 179
+# Configuration parameters: AllowedGroups.
+RSpec/NestedGroups:
+  Max: 7
+
+# Offense count: 4
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: EnforcedStyle.
+# SupportedStyles: not_to, to_not
+RSpec/NotToNot:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/haproxy_config_spec.rb'
+
+# Offense count: 4
+RSpec/RepeatedExampleGroupBody:
+  Exclude:
+    - 'spec/tcp_router_templates_spec.rb'
+
+# Offense count: 6
+RSpec/RepeatedExampleGroupDescription:
+  Exclude:
+    - 'spec/gorouter_templates_spec.rb'
+    - 'spec/route_registar_templates_spec.rb'
+    - 'spec/tcp_router_templates_spec.rb'
diff --git a/scripts/staged_shortlog b/scripts/staged_shortlog
@@ -4,7 +4,7 @@ diffs = `git diff --cached`.split('diff --git').select { |log| log =~ /Subprojec
 
 locations = {}
 diffs.each do |diff|
-  /a\/(?<loc>.+?)\sb.*index\s(?<shas>\S+)/m.match diff do |matches|
+  %r{a/(?<loc>.+?)\sb.*index\s(?<shas>\S+)}m.match diff do |matches|
     locations[matches[:loc]] = matches[:shas]
   end
 end
@@ -14,7 +14,7 @@ puts # Second line of git commit message should always be empty
 
 locations.each do |location, shas|
   Dir.chdir(location) do
-    remote = `git remote -v`.match(/github.com[\/:]([^\s]+?).git/)
+    remote = `git remote -v`.match(%r{github.com[/:]([^\s]+?).git})
     repo = remote && remote[1]
 
     puts "Bump #{repo}"

diff --git a/spec/gorouter_templates_spec.rb b/spec/gorouter_templates_spec.rb
@@ -1,8 +1,5 @@
 # frozen_string_literal: true
 
-# rubocop:disable Layout/LineLength
-# rubocop:disable Metrics/BlockLength
-
 require 'rspec'
 require 'yaml'
 require 'json'
@@ -119,7 +116,7 @@
             'password' => 'pass',
             'tls' => {
               'certificate' => TEST_CERT,
-              'key' => TEST_KEY,
+              'key' => TEST_KEY
             }
           },
           'enable_ssl' => true,
@@ -856,7 +853,7 @@
           context 'not enabled but rules provided' do
             before do
               deployment_manifest_fragment['router']['verify_client_certificate_metadata'] = [
-                  { "issuer_in_chain" => { "common_name" => "test.com" }}
+                { 'issuer_in_chain' => { 'common_name' => 'test.com' } }
               ]
             end
             it 'does not populate the property' do
@@ -870,7 +867,6 @@
             before do
               deployment_manifest_fragment['router']['enable_verify_client_certificate_metadata'] = false
               deployment_manifest_fragment['router']['verify_client_certificate_metadata'] = []
-
             end
             it 'does not populate the property' do
               expect { parsed_yaml }.not_to raise_error
@@ -883,16 +879,15 @@
             before do
               deployment_manifest_fragment['router']['enable_verify_client_certificate_metadata'] = true
               deployment_manifest_fragment['router']['verify_client_certificate_metadata'] = [
-                { "issuer_in_chain" => { "common_name" => "test-with-san.com" },
-                  "valid_cert_subjects" => [
-                    {"issuer_in_chain" => { "common_name" => "test.com client cert1" }},
-                    {"issuer_in_chain" => { "common_name" => "test.com client cert2", "locality" => ["US"] }}
-                  ]
-                }
+                { 'issuer_in_chain' => { 'common_name' => 'test-with-san.com' },
+                  'valid_cert_subjects' => [
+                    { 'issuer_in_chain' => { 'common_name' => 'test.com client cert1' } },
+                    { 'issuer_in_chain' => { 'common_name' => 'test.com client cert2', 'locality' => ['US'] } }
+                  ] }
               ]
             end
             it 'fails generating the template as there are metadata verification rules but no client ca certs' do
-              expect { parsed_yaml }.to raise_error RuntimeError, "client certificate rules defined, but no client CA defined in `client_ca_certs`"
+              expect { parsed_yaml }.to raise_error RuntimeError, 'client certificate rules defined, but no client CA defined in `client_ca_certs`'
             end
           end
           context 'enabled with configured client_ca_certs' do
@@ -903,12 +898,11 @@
               before do
                 deployment_manifest_fragment['router']['enable_verify_client_certificate_metadata'] = true
                 deployment_manifest_fragment['router']['verify_client_certificate_metadata'] = [
-                  { "issuer_in_chain" => { "common_name" => "test-with-san.com" },
-                    "valid_cert_subjects" => [
-                      {"issuer_in_chain" => { "common_name" => "test.com client cert1" }},
-                      {"issuer_in_chain" => { "common_name" => "test.com client cert2", "locality" => ["US"] }}
-                    ]
-                  }
+                  { 'issuer_in_chain' => { 'common_name' => 'test-with-san.com' },
+                    'valid_cert_subjects' => [
+                      { 'issuer_in_chain' => { 'common_name' => 'test.com client cert1' } },
+                      { 'issuer_in_chain' => { 'common_name' => 'test.com client cert2', 'locality' => ['US'] } }
+                    ] }
                 ]
               end
               it 'populates the properties after a successful check' do
@@ -921,12 +915,11 @@
               before do
                 deployment_manifest_fragment['router']['enable_verify_client_certificate_metadata'] = true
                 deployment_manifest_fragment['router']['verify_client_certificate_metadata'] = [
-                  { "issuer_in_chain" => { "common_name" => "test-with-san.com", "country" => ["US"] },
-                    "valid_cert_subjects" => [
-                      {"issuer_in_chain" => { "common_name" => "test.com client cert1" }},
-                      {"issuer_in_chain" => { "common_name" => "test.com client cert2", "locality" => ["US"] }}
-                    ]
-                  }
+                  { 'issuer_in_chain' => { 'common_name' => 'test-with-san.com', 'country' => ['US'] },
+                    'valid_cert_subjects' => [
+                      { 'issuer_in_chain' => { 'common_name' => 'test.com client cert1' } },
+                      { 'issuer_in_chain' => { 'common_name' => 'test.com client cert2', 'locality' => ['US'] } }
+                    ] }
                 ]
               end
               it 'fails and explains the valid cert subjects in the message' do
@@ -1230,7 +1223,9 @@
           end
 
           it 'raises error' do
-            expect { parsed_yaml }.to raise_error(RuntimeError, "'meow' is not a valid timestamp format for the property 'router.logging.format.timestamp'. Valid options are: 'rfc3339', 'deprecated', and 'unix-epoch'.")
+            err_msg = "'meow' is not a valid timestamp format for the property 'router.logging.format.timestamp'. Valid options are: 'rfc3339', 'deprecated', and 'unix-epoch'."
+
+            expect { parsed_yaml }.to raise_error(RuntimeError, err_msg)
           end
         end
 
@@ -1395,7 +1390,7 @@
 
     context 'when router.status.routes.port is specified' do
       before do
-        deployment_manifest_fragment['router']['status']['routes'] = {'port' => 8888}
+        deployment_manifest_fragment['router']['status']['routes'] = { 'port' => 8888 }
       end
 
       it 'overrides the default value' do
@@ -1442,7 +1437,7 @@
           deployment_manifest_fragment['router']['status']['tls']['port'] = 1234
         end
         it 'overrides the default' do
-        expect(parsed_yaml['status']['tls']['port']).to eq 1234
+          expect(parsed_yaml['status']['tls']['port']).to eq 1234
         end
       end
 
@@ -1451,19 +1446,18 @@
           deployment_manifest_fragment['router']['status']['tls'].delete('certificate')
         end
         it 'raises an error about missing certificate values' do
-          expect {parsed_yaml}.to raise_error(/router.status.tls.certificate must be provided when router.status.tls.port is set/)
+          expect { parsed_yaml }.to raise_error(/router.status.tls.certificate must be provided when router.status.tls.port is set/)
         end
       end
       context 'but the key is not specified' do
         before do
           deployment_manifest_fragment['router']['status']['tls'].delete('key')
         end
         it 'raises an error about missing key values' do
-          expect {parsed_yaml}.to raise_error(/router.status.tls.key must be provided when router.status.tls.port is set/)
+          expect { parsed_yaml }.to raise_error(/router.status.tls.key must be provided when router.status.tls.port is set/)
         end
       end
     end
-
   end
 
   describe 'healthchecker.yml' do
@@ -1498,7 +1492,7 @@
           'user' => 'some-user',
           'password' => 'some-password',
           'path' => '/is-process-alive-do-not-use-for-loadbalancing',
-          'scheme' => 'https',
+          'scheme' => 'https'
         }
       )
     end
@@ -1578,7 +1572,7 @@
     let(:properties) do
       { 'router' => {
         'port' => 81,
-        'status' => { 'port' => 8081, 'tls' => {'port' => 8443}, },
+        'status' => { 'port' => 8081, 'tls' => { 'port' => 8443 } },
         'prometheus' => { 'port' => 7777 },
         'tls_port' => 442,
         'debug_address' => '127.0.0.1:17003'
@@ -1654,6 +1648,3 @@
     end
   end
 end
-
-# rubocop:enable Layout/LineLength
-# rubocop:enable Metrics/BlockLength