From 27fe8fd3af9c6b0110a03c691c001ba7ae1473d6 Mon Sep 17 00:00:00 2001 From: Alan Si Date: Thu, 9 Jun 2016 17:43:15 -0700 Subject: [PATCH] rubocop rails code style refactoring --- .rubocop.yml | 3 +++ .rubocop_todo.yml | 2 +- app/controllers/branches_controller.rb | 10 +++---- app/controllers/build_parts_controller.rb | 2 +- app/controllers/builds_controller.rb | 2 +- app/jobs/enforce_timeouts_job.rb | 2 +- app/models/build.rb | 3 +-- app/models/build_artifact.rb | 2 +- app/models/build_attempt.rb | 6 ++--- app/models/build_part.rb | 4 +-- app/models/repository.rb | 27 +++++++++---------- app/uploaders/log_file_uploader.rb | 8 ++++++ ...ove_on_success_script_from_repositories.rb | 19 +++++++------ ...0150717220149_assign_builds_to_branches.rb | 2 +- db/seeds.rb | 2 +- lib/partitioner.rb | 4 +-- lib/partitioner/maven.rb | 5 ++++ spec/helpers/application_helper_spec.rb | 2 +- spec/lib/partitioner_spec.rb | 4 +-- spec/models/build_part_spec.rb | 8 +++--- spec/support/factories.rb | 2 +- 21 files changed, 68 insertions(+), 51 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 9d20b3392..eedc34500 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -8,6 +8,9 @@ AllCops: - 'vendor/**/*' - 'db/schema.rb' +Rails: + Enabled: true + Lint/HandleExceptions: Exclude: - 'lib/git_repo.rb' diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index d476795ef..6af97199b 100644 --- a/.rubocop_todo.yml +++ b/.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: diff --git a/app/controllers/branches_controller.rb b/app/controllers/branches_controller.rb index 370a5f0dc..f467dca51 100644 --- a/app/controllers/branches_controller.rb +++ b/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 diff --git a/app/controllers/build_parts_controller.rb b/app/controllers/build_parts_controller.rb index f15444cd6..54c9bbebb 100644 --- a/app/controllers/build_parts_controller.rb +++ b/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 } diff --git a/app/controllers/builds_controller.rb b/app/controllers/builds_controller.rb index dd62229cf..f55d06b0a 100644 --- a/app/controllers/builds_controller.rb +++ b/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 diff --git a/app/jobs/enforce_timeouts_job.rb b/app/jobs/enforce_timeouts_job.rb index 5221c3bac..8d6fe354b 100644 --- a/app/jobs/enforce_timeouts_job.rb +++ b/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 diff --git a/app/models/build.rb b/app/models/build.rb index 83288c193..337e60dd3 100644 --- a/app/models/build.rb +++ b/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 diff --git a/app/models/build_artifact.rb b/app/models/build_artifact.rb index a97b7c888..0d3ae90f4 100644 --- a/app/models/build_artifact.rb +++ b/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') } diff --git a/app/models/build_attempt.rb b/app/models/build_attempt.rb index 62c4be18e..35fcb2506 100644 --- a/app/models/build_attempt.rb +++ b/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. diff --git a/app/models/build_part.rb b/app/models/build_part.rb index 2c3e37146..2e07c023d 100644 --- a/app/models/build_part.rb +++ b/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 diff --git a/app/models/repository.rb b/app/models/repository.rb index eec8fe612..8a5298a9c 100644 --- a/app/models/repository.rb +++ b/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,17 +19,17 @@ 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 @@ -38,7 +37,7 @@ def self.lookup(host:, namespace:, name:) # 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 diff --git a/app/uploaders/log_file_uploader.rb b/app/uploaders/log_file_uploader.rb index ec3e10097..e3450bd51 100644 --- a/app/uploaders/log_file_uploader.rb +++ b/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 diff --git a/db/migrate/20150324001246_remove_on_success_script_from_repositories.rb b/db/migrate/20150324001246_remove_on_success_script_from_repositories.rb index cd494dd43..2ea744121 100644 --- a/db/migrate/20150324001246_remove_on_success_script_from_repositories.rb +++ b/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 diff --git a/db/migrate/20150717220149_assign_builds_to_branches.rb b/db/migrate/20150717220149_assign_builds_to_branches.rb index ee1f1fe0c..7e586e61f 100644 --- a/db/migrate/20150717220149_assign_builds_to_branches.rb +++ b/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 diff --git a/db/seeds.rb b/db/seeds.rb index 7fedfd611..e439164d4 100644 --- a/db/seeds.rb +++ b/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) diff --git a/lib/partitioner.rb b/lib/partitioner.rb index 647987216..89bec794e 100644 --- a/lib/partitioner.rb +++ b/lib/partitioner.rb @@ -6,7 +6,7 @@ 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) @@ -14,7 +14,7 @@ def self.for_build(build) # 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 diff --git a/lib/partitioner/maven.rb b/lib/partitioner/maven.rb index 4631f46db..784e33d57 100644 --- a/lib/partitioner/maven.rb +++ b/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 diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 68aba84bb..fe4b0898e 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/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( %{#{timestamp}} ) diff --git a/spec/lib/partitioner_spec.rb b/spec/lib/partitioner_spec.rb index 006841ae5..8ed5c7a18 100644 --- a/spec/lib/partitioner_spec.rb +++ b/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 diff --git a/spec/models/build_part_spec.rb b/spec/models/build_part_spec.rb index ad97a6b16..abedb8e37 100644 --- a/spec/models/build_part_spec.rb +++ b/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 diff --git a/spec/support/factories.rb b/spec/support/factories.rb index ca698b9ba..e150d2d28 100644 --- a/spec/support/factories.rb +++ b/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