Merge pull request #2945 from zendesk/grosser/cop

modernize rubocop a bit

.rubocop.yml

@@ -6,21 +6,15 @@ AllCops:
     - tmp/**/*
     - node_modules/**/*
 
-LineLength:
+Metrics/LineLength:
   Max: 120
 
-ClassLength:
-  Enabled: false
-
-Layout/CommentIndentation:
+Metrics/ClassLength:
   Enabled: false
 
 Style/StringLiterals:
   Enabled: false
 
-HashSyntax:
-   EnforcedStyle: ruby19
-
 Style/SignalException:
   Enabled: false
 
@@ -31,8 +25,9 @@ Layout/SpaceInsideHashLiteralBraces:
 Lint/AmbiguousOperator:
   Enabled: false
 
+# Always use `->`
 Style/Lambda:
-  Enabled: false
+  EnforcedStyle: literal
 
 Style/SpecialGlobalVars:
   Enabled: false
@@ -55,14 +50,15 @@ Style/StringLiteralsInInterpolation:
 Style/NumericLiterals:
   Enabled: false
 
+# prefer simpler `a == 0` over `a.zero?`
 Style/NumericPredicate:
-  Enabled: false
+  EnforcedStyle: comparison
 
 Layout/FirstParameterIndentation:
   Enabled: false
 
 Layout/IndentHash:
-  Enabled: false
+  EnforcedStyle: consistent
 
 Layout/AlignParameters:
   EnforcedStyle: with_fixed_indentation
@@ -92,7 +88,7 @@ Metrics/CyclomaticComplexity:
   Enabled: false
 
 Layout/MultilineMethodCallIndentation:
-  Enabled: false
+  Enabled: false # none of the EnforcedStyle options allow for consistent 2-space indent
 
 Lint/AmbiguousRegexpLiteral:
   Enabled: false
@@ -103,14 +99,7 @@ Layout/DotPosition:
 Style/ClassAndModuleChildren:
   Enabled: false
 
-# %w[] shows that it will return an array
-Style/PercentLiteralDelimiters:
-  PreferredDelimiters:
-    '%i': '[]'
-    '%w': '[]'
-    '%W': '[]'
-
-# for single `/` more readable
+# often single `/` is more readable as `/a\/b/`
 Style/RegexpLiteral:
   Enabled: false
 
@@ -138,6 +127,7 @@ Metrics/ParameterLists:
 Metrics/BlockLength:
   Enabled: false
 
+# Allow use of `if a = b(1, 2)` for simplified control flow
 Lint/AssignmentInCondition:
   Enabled: false
 

app/controllers/csv_exports_controller.rb

@@ -73,8 +73,8 @@ def user_filter
     options = {}
     options[:inherited] = params[:inherited] == "true"
     options[:deleted] = params[:deleted] == "true"
-    options[:project_id] = params[:project_id].to_i unless params[:project_id].to_i.zero?
-    options[:user_id] = params[:user_id].to_i unless params[:user_id].to_i.zero?
+    options[:project_id] = params[:project_id].to_i unless params[:project_id].to_i == 0
+    options[:user_id] = params[:user_id].to_i unless params[:user_id].to_i == 0
     options[:datetime] = Time.now.strftime "%Y%m%d_%H%M"
     options
   end
@@ -118,7 +118,7 @@ def deploy_filter
     end
 
     if project = params[:project].try(:to_i)
-      if project.positive?
+      if project > 0
         filter['stages.project_id'] = project
       elsif project.to_s != params[:project]
         raise "Invalid project id #{params[:project]}"

app/controllers/mass_rollouts_controller.rb

@@ -107,7 +107,7 @@ def merge_stage(stage)
 
     return "has no template stage to merge into" unless template_stage
     return "is a template stage" if stage.is_template
-    return "has no deploy groups" if stage.deploy_groups.count.zero?
+    return "has no deploy groups" if stage.deploy_groups.count == 0
     return "has more than one deploy group" if stage.deploy_groups.count > 1
     return "commands in template stage differ" if stage.script != template_stage.script
 

app/helpers/application_helper.rb

@@ -311,7 +311,7 @@ def link_to_chart(name, values)
 
     max = values.max.round
     y_axis = [0, max / 4, max / 2, (max / 1.333333).to_i, max].map { |t| duration_text(t) }.join("|")
-    y_values = values.reverse.map { |v| max.zero? ? max : (v * 100.0 / max).round }.join(",") # values as % of max
+    y_values = values.reverse.map { |v| max == 0 ? max : (v * 100.0 / max).round }.join(",") # values as % of max
     params = {
       cht: "lc", # chart type
       chtt: name,

app/helpers/dashboards_helper.rb

@@ -6,7 +6,7 @@ def project_has_different_deploys?(deploy_group_versions)
 
   def dashboard_project_row_style(project_id)
     project_versions = @versions[project_id].values.map(&:reference).uniq.count
-    if project_versions.zero?
+    if project_versions == 0
       'class=no-deploys'
     elsif project_versions == 1
       ''

app/helpers/deploys_helper.rb

@@ -41,7 +41,7 @@ def file_status_label(status)
   end
 
   def file_changes_label(count, type)
-    content_tag :span, count.to_s, class: "label #{type}" unless count.zero?
+    content_tag :span, count.to_s, class: "label #{type}" unless count == 0
   end
 
   def github_users(github_users)

app/models/csv_export.rb

@@ -9,7 +9,7 @@ class CsvExport < ActiveRecord::Base
   validates :status, inclusion: {in: STATUS_VALUES}
   delegate :email, to: :user, allow_nil: true
 
-  scope :old, lambda {
+  scope :old, -> {
     end_date = Rails.application.config.samson.export_job.downloaded_age.seconds.ago
     timeout_date = Rails.application.config.samson.export_job.max_age.seconds.ago
     where("(status = 'downloaded' AND updated_at <= :end_date) OR created_at <= :timeout_date",

app/models/git_repository.rb

@@ -106,7 +106,7 @@ def ensure_mirror_current
   # @returns [block result, false on lock timeout]
   def exclusive(timeout: 10.minutes)
     log_wait = proc do |owner|
-      if Rails.env.test? || (Time.now.to_i % 10).zero?
+      if Rails.env.test? || (Time.now.to_i % 10) == 0
         executor.output.write("Waiting for repository lock for #{owner}\n")
       end
     end

app/models/image_builder.rb

@@ -92,7 +92,7 @@ def push_image_to_registries(image_id, build, executor, tag:, override_tag:)
       digest = nil
 
       DockerRegistry.all.each_with_index do |registry, i|
-        primary = i.zero?
+        primary = i == 0
         repo = build.project.docker_repo(registry, build.dockerfile)
 
         if override_tag

app/models/job_queue.rb

@@ -139,7 +139,7 @@ def debug_hash_from_queue(queue)
   end
 
   def staggering_enabled?
-    ENV['SERVER_MODE'] && !stagger_interval.zero?
+    ENV['SERVER_MODE'] && stagger_interval != 0
   end
 
   def stagger_interval

lib/samson/boot_check.rb

@@ -7,7 +7,7 @@ def check
           # make sure nobody uses connections on the main thread since they will block reloading in dev
           error = "Do not use AR on the main thread, use ActiveRecord::Base.connection_pool.with_connection"
           Samson::Retry.until_result tries: 10, wait_time: 0.5, error: error do
-            ActiveRecord::Base.connection_pool.stat.fetch(:busy).zero?
+            ActiveRecord::Base.connection_pool.stat.fetch(:busy) == 0
           end
         else
           # make sure we do not regress into slow startup time by preloading too much

lib/samson/logging.rb

@@ -10,7 +10,7 @@
   # log 1 message per request to syslog in json format
   config.lograge.enabled = true
 
-  config.lograge.custom_options = lambda do |event|
+  config.lograge.custom_options = ->(event) do
     # show params for every request
     unwanted_keys = %w[format action controller]
     params = event.payload[:params].reject { |key, _| unwanted_keys.include? key }

lib/samson/retry.rb

@@ -22,7 +22,7 @@ def self.until_result(tries:, wait_time:, error:)
         tries -= 1
         result = yield
         return result if result
-        raise error if tries.zero?
+        raise error if tries == 0
         sleep(wait_time)
       end
     end

plugins/docker_binary_builder/app/models/binary_builder.rb

@@ -109,7 +109,7 @@ def create_container_options
 
     # Mount a cache directory for sharing .m2, .ivy2, .bundler directories between build containers.
     api_version_major, api_version_minor = docker_api_version.scan(/(\d+)\.(\d+)/).flatten.map(&:to_i)
-    if api_version_major.zero? || (api_version_major == 1 && api_version_minor <= 14)
+    if api_version_major == 0 || (api_version_major == 1 && api_version_minor <= 14)
       fail "Unsupported Docker api version '#{docker_api_version}', use at least v1.15"
     elsif api_version_major == 1 && api_version_minor <= 22
       options.merge!(

plugins/jenkins/app/models/samson/jenkins.rb

@@ -251,7 +251,7 @@ def notify_emails
       emails.select! { |e| e.include?('@') }
       emails.map! { |x| Mail::Address.new(x) }
       if restricted_domain = ENV["EMAIL_DOMAIN"]
-        emails.select! { |x| x.domain.casecmp(restricted_domain).zero? }
+        emails.select! { |x| x.domain.casecmp(restricted_domain) == 0 }
       end
       emails.map(&:address).uniq.join(",")
     end

plugins/kubernetes/app/decorators/deploy_group_decorator.rb

@@ -15,7 +15,7 @@
     :cluster_deploy_group,
     allow_destroy: true,
     update_only: true,
-    reject_if: lambda { |h| h[:kubernetes_cluster_id].blank? }
+    reject_if: ->(h) { h[:kubernetes_cluster_id].blank? }
   )
 
   def kubernetes_namespace

plugins/kubernetes/app/models/kubernetes/api/pod.rb

@@ -31,7 +31,7 @@ def failed?
       end
 
       def restarted?
-        @pod.dig(:status, :containerStatuses)&.any? { |s| s.fetch(:restartCount).positive? }
+        @pod.dig(:status, :containerStatuses)&.any? { |s| s.fetch(:restartCount) > 0 }
       end
 
       def phase

plugins/kubernetes/app/models/kubernetes/resource.rb

@@ -280,7 +280,7 @@ def request_delete
           # $ kubectl scale deployment {DEPLOYMENT_NAME} --replicas 0
           # "replicas" key is actually removed from "status" map
           # $ {"status":{"conditions":[...],"observedGeneration":2}}
-          break if fetch_resource.dig(:status, :replicas).to_i.zero?
+          break if fetch_resource.dig(:status, :replicas).to_i == 0
         end
 
         # delete the actual deployment

plugins/rollbar_dashboards/test/decorators/project_decorator_test.rb

@@ -12,7 +12,7 @@
     it "assigns rollbar dashboard attributes" do
       project.attributes = {
         rollbar_dashboards_settings_attributes: {0 => {read_token: '123', base_url: 'https://foobar.org'}}
-        }
+      }
       project.rollbar_dashboards_settings.size.must_equal 1
     end
 

script/create_many_projects_stages_deploys.rb

@@ -26,7 +26,7 @@
 
 NUM_PROJECTS.times do |i|
   project_name = "Project#{i}"
-  next unless Project.unscoped.where(name: project_name).count.zero?
+  next unless Project.unscoped.where(name: project_name).count == 0
   project = Project.create!(name: project_name, repository_url: "git@github.com:samson-test-org/example-project.git")
   project.stages.create!(name: "Production", deploy_groups: [pod1, pod2, pod3, pod4, pod5, pod6])
   project.stages.create!(name: "Staging", deploy_groups: [pod100, pod101])

test/controllers/integrations/buildkite_controller_test.rb

@@ -51,7 +51,7 @@
   end
 
   context 'when the buildkite_release_params hook gets trigger' do
-    let(:buildkite_build_number) { lambda { |_, _| [[:number, 9]] } }
+    let(:buildkite_build_number) { ->(_, _) { [[:number, 9]] } }
     before do
       project.releases.destroy_all
       project.builds.destroy_all

test/models/changeset_test.rb

@@ -87,9 +87,7 @@
     let(:pr_from_coolcommitter) do
       Sawyer::Resource.new(
         sawyer_agent,
-        user: {
-        login: 'coolcommitter'
-        },
+        user: {login: 'coolcommitter'},
         additions: 10
       )
     end

test/models/multi_lock_test.rb

@@ -21,7 +21,7 @@ def assert_time(operand, number)
     end
 
     it "locks" do
-      result = MultiLock.lock(1, "a", timeout: 1, failed_to_lock: lambda { |owner| calls << owner }) { calls << 1 }
+      result = MultiLock.lock(1, "a", timeout: 1, failed_to_lock: ->(owner) { calls << owner }) { calls << 1 }
       result.must_equal true
       assert_equal [1], calls
     end
@@ -30,7 +30,7 @@ def assert_time(operand, number)
       MultiLock.send(:try_lock, 1, "a")
 
       assert_time :>, 1 do
-        result = MultiLock.lock(1, "a", timeout: 1, failed_to_lock: lambda { |owner| calls << owner }) { calls << 1 }
+        result = MultiLock.lock(1, "a", timeout: 1, failed_to_lock: ->(owner) { calls << owner }) { calls << 1 }
         result.must_equal false
       end
       assert_equal ["a"], calls

test/models/release_service_test.rb

@@ -68,7 +68,7 @@ def assert_failed_ref_find(count)
       let!(:stage) { project.stages.create!(name: "production", deploy_on_release: true) }
 
       it 'does not deploy if the release_deploy_condition check is false' do
-        deployable_condition_check = lambda { |_, _| false }
+        deployable_condition_check = ->(_, _) { false }
 
         Samson::Hooks.with_callback(:release_deploy_conditions, deployable_condition_check) do |_|
           service.release(commit: commit, author: author)
@@ -78,7 +78,7 @@ def assert_failed_ref_find(count)
       end
 
       it 'does deploy if the release_deploy_condition check is true' do
-        deployable_condition_check = lambda { |_, _| true }
+        deployable_condition_check = ->(_, _) { true }
 
         Samson::Hooks.with_callback(:release_deploy_conditions, deployable_condition_check) do |_|
           release = service.release(commit: commit, author: author)

test/test_helper.rb

@@ -82,7 +82,7 @@ def freeze_time
   # record hook and their arguments called during a given block
   def record_hooks(callback, &block)
     called = []
-    Samson::Hooks.with_callback(callback, lambda { |*args| called << args }, &block)
+    Samson::Hooks.with_callback(callback, ->(*args) { called << args }, &block)
     called
   end