rubocop rails code style refactoring

.rubocop.yml

@@ -8,6 +8,9 @@ AllCops:
     - 'vendor/**/*'
     - 'db/schema.rb'
 
+Rails:
+  Enabled: true
+
 Lint/HandleExceptions:
   Exclude:
     - 'lib/git_repo.rb'

.rubocop_todo.yml

@@ -45,7 +45,7 @@ Metrics/AbcSize:
 # Offense count: 7
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 256
+  Max: 300
 
 # Offense count: 8
 Metrics/CyclomaticComplexity:

app/controllers/branches_controller.rb

@@ -159,20 +159,20 @@ def load_build_stats
     @first_built_date = @branch.builds.first.try(:created_at)
     return if @first_built_date.nil?
 
-    @days_since_first_build = (Date.today - @first_built_date.to_date).to_i
+    @days_since_first_build = (Time.zone.today - @first_built_date.to_date).to_i
 
     @total_build_count = @branch.builds.count
     @total_failure_count = @branch.builds.where.not(state: 'succeeded').count
     @total_pass_rate = (@total_build_count - @total_failure_count) * 100 / @total_build_count
 
-    @last30_build_count = @branch.builds.where('created_at >= ?', Date.today - 30.days).count
+    @last30_build_count = @branch.builds.where('created_at >= ?', Time.zone.today - 30.days).count
     return if @last30_build_count.zero?
-    @last30_failure_count = @last30_build_count - @branch.builds.where('state = "succeeded" AND created_at >= ?', Date.today - 30.days).count
+    @last30_failure_count = @last30_build_count - @branch.builds.where('state = "succeeded" AND created_at >= ?', Time.zone.today - 30.days).count
     @last30_pass_rate = (@last30_build_count - @last30_failure_count) * 100 / @last30_build_count
 
-    @last7_build_count = @branch.builds.where('created_at >= ?', Date.today - 7.days).count
+    @last7_build_count = @branch.builds.where('created_at >= ?', Time.zone.today - 7.days).count
     return if @last7_build_count.zero?
-    @last7_failure_count = @last7_build_count - @branch.builds.where('state = "succeeded" AND created_at >= ?', Date.today - 7.days).count
+    @last7_failure_count = @last7_build_count - @branch.builds.where('state = "succeeded" AND created_at >= ?', Time.zone.today - 7.days).count
     @last7_pass_rate = (@last7_build_count - @last7_failure_count) * 100 / @last7_build_count
   end
 end

app/controllers/build_parts_controller.rb

@@ -1,5 +1,5 @@
 class BuildPartsController < ApplicationController
-  before_filter :load_repository_build_and_part, :only => [:rebuild, :show, :modified_time]
+  before_action :load_repository_build_and_part, :only => [:rebuild, :show, :modified_time]
 
   caches_action :show, cache_path: proc {
     { :modified => @build_part.updated_at.to_i }

app/controllers/builds_controller.rb

@@ -1,7 +1,7 @@
 require 'git_repo'
 
 class BuildsController < ApplicationController
-  before_filter :load_repository, :only => [:show, :retry_partitioning, :rebuild_failed_parts, :request_build, :abort, :toggle_merge_on_success, :build_status, :modified_time]
+  before_action :load_repository, :only => [:show, :retry_partitioning, :rebuild_failed_parts, :request_build, :abort, :toggle_merge_on_success, :build_status, :modified_time]
 
   caches_action :show, cache_path: proc {
     updated_at = Build.select(:updated_at).find(params[:id]).updated_at

app/jobs/enforce_timeouts_job.rb

@@ -19,7 +19,7 @@ def message.path
         end
 
         BuildArtifact.create(:build_attempt_id => attempt.id, :log_file => message)
-        attempt.update!(state: :errored, finished_at: Time.now)
+        attempt.update!(state: :errored, finished_at: Time.current)
         Rails.logger.error "Errored BuildAttempt:#{ attempt.id } due to timeout"
 
         # Enqueue another BuildAttempt if this is the most recent attempt for the BuildPart

app/models/build.rb

@@ -114,7 +114,6 @@ def update_state_from_parts!
     errored = build_parts.errored
     passed = build_parts.passed
     failed = build_parts.failed
-
     next_state = case
                  when (build_parts - passed).empty?
                    :succeeded
@@ -234,7 +233,7 @@ def abort!
     BuildAttempt
       .joins(:build_part)
       .where(:state => :runnable, 'build_parts.build_id' => self.id)
-      .update_all(state: :aborted, updated_at: Time.now)
+      .update_all(state: :aborted, updated_at: Time.current)
   end
 
   def to_color

app/models/build_artifact.rb

@@ -4,7 +4,7 @@ class BuildArtifact < ActiveRecord::Base
   belongs_to :build_attempt, :inverse_of => :build_artifacts, :touch => true
   mount_uploader :log_file, LogFileUploader
   skip_callback :commit, :after, :remove_log_file!
-  validates_presence_of :log_file
+  validates :log_file, presence: true
 
   scope :stdout_log, -> { where(:log_file => ['stdout.log.gz', 'stdout.log']) }
   scope :error_txt, -> { where(:log_file => 'error.txt') }

app/models/build_attempt.rb

@@ -16,12 +16,12 @@ def elapsed_time
     if finished_at && started_at
       finished_at - started_at
     elsif started_at
-      Time.now - started_at
+      Time.current - started_at
     end
   end
 
   def start!(builder)
-    return false unless update_attributes(:state => :running, :started_at => Time.now, :builder => builder)
+    return false unless update_attributes(:state => :running, :started_at => Time.current, :builder => builder)
 
     build = build_part.build_instance
     previous_state, new_state = build.update_state_from_parts!
@@ -40,7 +40,7 @@ def start!(builder)
   end
 
   def finish!(state)
-    return false unless update_attributes(:state => state, :finished_at => Time.now)
+    return false unless update_attributes(:state => state, :finished_at => Time.current)
 
     if should_reattempt?
       # Will only send email if email_on_first_failure is enabled.

app/models/build_part.rb

@@ -3,7 +3,7 @@ class BuildPart < ActiveRecord::Base
   belongs_to :build_instance, :class_name => "Build", :foreign_key => "build_id", :inverse_of => :build_parts
   has_many :build_attempts, :dependent => :destroy, :inverse_of => :build_part
   symbolize :queue
-  validates_presence_of :kind, :paths, :queue
+  validates :kind, :paths, :queue, presence: true
 
   serialize :paths, Array
   serialize :options, Hash
@@ -92,7 +92,7 @@ def elapsed_time
     if finished_at && started_at
       finished_at - started_at
     elsif started_at
-      Time.now - started_at
+      Time.current - started_at
     end
   end
 

app/models/repository.rb

@@ -6,12 +6,11 @@
 class Repository < ActiveRecord::Base
   has_many :branches, :dependent => :destroy
   has_many :convergence_branches, -> { where(convergence: true) }, class_name: "Branch"
-  validates_presence_of :host, :name, :url
-  validates_uniqueness_of :name
-  validates_numericality_of :timeout, :only_integer => true
-  validates_inclusion_of :timeout, :in => 0..1440, :message => 'The maximum timeout allowed is 1440 minutes'
-
-  validates_uniqueness_of :url, :allow_blank => true
+  validates :host, :name, :url, presence: true
+  validates :name, uniqueness: true
+  validates :timeout, numericality: { :only_integer => true }
+  validates :timeout, inclusion: { in: 0..1440, message: 'The maximum timeout allowed is 1440 minutes' }
+  validates :url, uniqueness: true, allow_blank: true
   validate :validate_url_against_remote_servers
 
   def self.lookup_by_url(url)
@@ -20,25 +19,25 @@ def self.lookup_by_url(url)
     repository_name = remote_server.attributes.fetch(:repository_name)
     repository_host_and_aliases = remote_server.attributes.fetch(:possible_hosts)
 
-    Repository.where(host: repository_host_and_aliases,
-                     namespace: repository_namespace,
-                     name: repository_name).first
+    Repository.find_by(host: repository_host_and_aliases,
+                       namespace: repository_namespace,
+                       name: repository_name)
   end
 
   def self.lookup(host:, namespace:, name:)
     git_server_settings = Settings.git_server(host)
 
-    Repository.where(host: [git_server_settings.host, *git_server_settings.aliases].compact,
-                     namespace: namespace,
-                     name: name).first
+    Repository.find_by(host: [git_server_settings.host, *git_server_settings.aliases].compact,
+                       namespace: namespace,
+                       name: name)
   end
 
   # Setting a URL will extract values for host, namespace, and name. This
   # should not overwrite values for those attributes that were set in the same
   # session.
   def url=(value)
     # this column is deprecated; eventually url will just be a virtual attribute
-    write_attribute(:url, value)
+    self[:url] = value
 
     return unless RemoteServer.parseable_url?(value)
     return unless RemoteServer.valid_git_host?(value)
@@ -71,7 +70,7 @@ def interested_github_events
   #
   # Returns: Build AR object or nil
   def build_for_commit(sha)
-    Build.joins(:branch_record).where(:ref => sha, 'branches.repository_id' => self.id).first
+    Build.joins(:branch_record).find_by(:ref => sha, 'branches.repository_id' => self.id)
   end
 
   # Public: looks across all of a repository's builds for one with the given

app/uploaders/log_file_uploader.rb

@@ -5,6 +5,14 @@ def store_dir
     build_attempt_id = model.build_attempt_id
     build_part_id = model.build_attempt.build_part_id
     build_id = model.build_attempt.build_part.build_id
+
+    # temporary backwards compatibility for old build artifacts created before the deploy on 08/25/2015
+    if model.build_attempt.created_at < Time.parse("2015-08-25 04:12:46 UTC").utc &&
+       (project_id = model.build_attempt.build_part.build_instance.project_id)
+      project_param = ActiveRecord::Base.connection.select_value("select name from projects where id = #{project_id}")
+      return File.join(project_param, "build_#{build_id}", "part_#{build_part_id}", "attempt_#{build_attempt_id}")
+    end
+
     repository_param = model.build_attempt.build_part.build_instance.repository.to_param
     Rails.public_path.join("log_files", repository_param, "build_#{build_id}", "part_#{build_part_id}", "attempt_#{build_attempt_id}")
   end

db/migrate/20150324001246_remove_on_success_script_from_repositories.rb

@@ -4,14 +4,17 @@ def up
     rows_with_old_data = select_value("select count(*) from repositories where on_success_script IS NOT NULL AND on_success_script != ''")
 
     if rows_with_old_data > 0
-      puts
-      puts "Found #{rows_with_old_data} rows in the Repositories table with non-empty values"
-      puts "for `on_success_script`."
-      puts
-      puts "Kochiku no longer supports on_success_script inside of the repository table."
-      puts "The new location is inside of each project's kochiku.yml file."
-      puts
-      puts "Please remove the data from the on_success_script column and re-run this migration."
+      err_message = <<-ERR_MESSAGE
+        "Found #{rows_with_old_data} rows in the Repositories table with non-empty values"
+        "for `on_success_script`."
+
+        "Kochiku no longer supports on_success_script inside of the repository table."
+        "The new location is inside of each project's kochiku.yml file."
+
+        "Please remove the data from the on_success_script column and re-run this migration."
+      ERR_MESSAGE
+
+      Rails.logger.error(err_message)
       exit(1)
     end
 

db/migrate/20150717220149_assign_builds_to_branches.rb

@@ -8,7 +8,7 @@ def up
       repository_id = connection.select_value("SELECT repository_id FROM projects WHERE id = #{build.project_id}")
 
       if repository_id.nil?
-        puts "skipping Build #{build.id} because its project or repository not longer exists"
+        Rails.logger.error "skipping Build #{build.id} because its project or repository not longer exists"
         next
       end
 

db/seeds.rb

@@ -48,7 +48,7 @@ def create_build_part(build, kind, paths, build_attempt_state)
     :build_part => bp,
     :builder => @builders.sample,
     :state => build_attempt_state,
-    :started_at => Time.now,
+    :started_at => Time.current,
     :finished_at => finished
   )
   create_build_artifact(attempt)

lib/partitioner.rb

@@ -6,15 +6,15 @@ module Partitioner
   def self.for_build(build)
     kochiku_yml = build.kochiku_yml
     if kochiku_yml
-      start = Time.now
+      start = Time.current
       res = case kochiku_yml['partitioner']
             when 'maven'
               Partitioner::Maven.new(build, kochiku_yml)
             else
               # Default behavior
               Partitioner::Default.new(build, kochiku_yml)
             end
-      finish = Time.now
+      finish = Time.current
       diff = finish - start
       Rails.logger.info("Partition finished: [#{kochiku_yml['partitioner'] || 'DEFAULT'}] #{diff} #{build.ref}")
       res

lib/partitioner/maven.rb

@@ -21,6 +21,8 @@ def initialize(build, kochiku_yml)
     end
 
     def partitions
+      Rails.logger.info("Partition started: [maven] #{@build.ref}")
+      start = Time.current
       modules_to_build = Set.new
 
       GitRepo.inside_copy(@build.repository, @build.ref) do
@@ -49,6 +51,9 @@ def partitions
 
         add_options(group_modules(sort_modules(modules_to_build)))
       end
+    ensure
+      # TODO: log this information to event stream
+      Rails.logger.info("Partition finished: [maven] #{Time.current - start} #{@build.ref}")
     end
 
     def emails_for_commits_causing_failures

spec/helpers/application_helper_spec.rb

@@ -100,7 +100,7 @@
 
   describe "timeago" do
     it "should generate the correct abbr tag" do
-      timestamp = Time.at(0)
+      timestamp = Time.at(0).utc
       expect(timeago(timestamp)).to eq(
         %{<abbr class="timeago" title="1970-01-01T00:00:00Z">#{timestamp}</abbr>}
       )

spec/lib/partitioner_spec.rb

@@ -40,7 +40,7 @@
         end
 
         it "parses options from kochiku yml" do
-          allow(Time).to receive(:now).and_return(Time.new(1977, 3, 10, 5, 30, 0))
+          allow(Time).to receive(:now).and_return(Time.new(1977, 3, 10, 5, 30, 0).utc)
           expect(Rails.logger).to receive(:info).with("Partition finished: [DEFAULT] 0.0 1111111111111111111111111111111111111111")
           partitions = subject.partitions
           expect(partitions.first["options"]["ruby"]).to eq("ree-1.8.7-2011.12")
@@ -55,7 +55,7 @@
         let(:kochiku_yml) { {'partitioner' => 'maven'} }
 
         it "should call the maven partitioner" do
-          allow(Time).to receive(:now).and_return(Time.new(1977, 3, 10, 5, 30, 0))
+          allow(Time).to receive(:now).and_return(Time.new(1977, 3, 10, 5, 30, 0).utc)
           expect(Rails.logger).to receive(:info).with("Partition finished: [maven] 0.0 1111111111111111111111111111111111111111")
           expect(subject).to be_a(Partitioner::Maven)
         end

spec/models/build_part_spec.rb

@@ -155,7 +155,7 @@
       it { should be true }
     end
     context "when finished" do
-      before { FactoryGirl.create(:build_attempt, :build_part => build_part, :state => :passed, :finished_at => Time.now) }
+      before { FactoryGirl.create(:build_attempt, :build_part => build_part, :state => :passed, :finished_at => Time.current) }
       it { should be false }
     end
   end
@@ -192,7 +192,7 @@
       let(:build_part) { FactoryGirl.create(:build_part, retry_count: 0, build_instance: build) }
 
       before do
-        FactoryGirl.create(:build_attempt, build_part: build_part, state: :errored, started_at: 10.seconds.ago, finished_at: Time.now)
+        FactoryGirl.create(:build_attempt, build_part: build_part, state: :errored, started_at: 10.seconds.ago, finished_at: Time.current)
       end
 
       it "will reattempt" do
@@ -205,7 +205,7 @@
 
       before do
         5.times do
-          FactoryGirl.create(:build_attempt, build_part: build_part, state: :errored, started_at: 10.seconds.ago, finished_at: Time.now)
+          FactoryGirl.create(:build_attempt, build_part: build_part, state: :errored, started_at: 10.seconds.ago, finished_at: Time.current)
         end
       end
 
@@ -218,7 +218,7 @@
       let(:build_part) { FactoryGirl.create(:build_part, retry_count: 0, build_instance: build) }
 
       before do
-        FactoryGirl.create(:build_attempt, build_part: build_part, state: :errored, started_at: 70.seconds.ago, finished_at: Time.now)
+        FactoryGirl.create(:build_attempt, build_part: build_part, state: :errored, started_at: 70.seconds.ago, finished_at: Time.current)
       end
 
       it "shouldn't reattempt" do

spec/support/factories.rb

@@ -56,7 +56,7 @@
 
     factory :completed_build_attempt do
       state { build_part.build_instance.state == :succeeded ? :passed : :failed }
-      finished_at { Time.now }
+      finished_at { Time.current }
     end
   end