diff --git a/.rubocop.yml b/.rubocop.yml index 037e8c4e00f..2944b3555e8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -355,7 +355,6 @@ RSpec/LeakyConstantDeclaration: - 'spec/lib/gitlab/quick_actions/dsl_spec.rb' - 'spec/lib/marginalia_spec.rb' - 'spec/mailers/notify_spec.rb' - - 'spec/migrations/20191125114345_add_admin_mode_protected_path_spec.rb' - 'spec/models/concerns/batch_destroy_dependent_associations_spec.rb' - 'spec/models/concerns/bulk_insert_safe_spec.rb' - 'spec/models/concerns/bulk_insertable_associations_spec.rb' diff --git a/app/models/chat_team.rb b/app/models/chat_team.rb index 28aab279545..6e39d7e2204 100644 --- a/app/models/chat_team.rb +++ b/app/models/chat_team.rb @@ -12,6 +12,6 @@ class ChatTeam < ApplicationRecord # Either the group is not found, or the user doesn't have the proper # access on the mattermost instance. In the first case, we're done either way # in the latter case, we can't recover by retrying, so we just log what happened - Rails.logger.error("Mattermost team deletion failed: #{e}") # rubocop:disable Gitlab/RailsLogger + Gitlab::AppLogger.error("Mattermost team deletion failed: #{e}") end end diff --git a/app/models/project.rb b/app/models/project.rb index c657e76c86f..314f24f3f6d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -925,17 +925,15 @@ class Project < ApplicationRecord job_id end - # rubocop:disable Gitlab/RailsLogger def log_import_activity(job_id, type: :import) job_type = type.to_s.capitalize if job_id - Rails.logger.info("#{job_type} job scheduled for #{full_path} with job ID #{job_id}.") + Gitlab::AppLogger.info("#{job_type} job scheduled for #{full_path} with job ID #{job_id}.") else - Rails.logger.error("#{job_type} job failed to create for #{full_path}.") + Gitlab::AppLogger.error("#{job_type} job failed to create for #{full_path}.") end end - # rubocop:enable Gitlab/RailsLogger def reset_cache_and_import_attrs run_after_commit do @@ -1776,17 +1774,15 @@ class Project < ApplicationRecord ensure_pages_metadatum.update!(deployed: false) end - # rubocop:disable Gitlab/RailsLogger def write_repository_config(gl_full_path: full_path) # We'd need to keep track of project full path otherwise directory tree # created with hashed storage enabled cannot be usefully imported using # the import rake task. repository.raw_repository.write_config(full_path: gl_full_path) rescue Gitlab::Git::Repository::NoRepository => e - Rails.logger.error("Error writing to .git/config for project #{full_path} (#{id}): #{e.message}.") + Gitlab::AppLogger.error("Error writing to .git/config for project #{full_path} (#{id}): #{e.message}.") nil end - # rubocop:enable Gitlab/RailsLogger def after_import repository.expire_content_cache @@ -1829,17 +1825,15 @@ class Project < ApplicationRecord @pipeline_status ||= Gitlab::Cache::Ci::ProjectPipelineStatus.load_for_project(self) end - # rubocop:disable Gitlab/RailsLogger def add_export_job(current_user:, after_export_strategy: nil, params: {}) job_id = ProjectExportWorker.perform_async(current_user.id, self.id, after_export_strategy, params) if job_id - Rails.logger.info "Export job started for project ID #{self.id} with job ID #{job_id}" + Gitlab::AppLogger.info "Export job started for project ID #{self.id} with job ID #{job_id}" else - Rails.logger.error "Export job failed to start for project ID #{self.id}" + Gitlab::AppLogger.error "Export job failed to start for project ID #{self.id}" end end - # rubocop:enable Gitlab/RailsLogger def import_export_shared @import_export_shared ||= Gitlab::ImportExport::Shared.new(self) diff --git a/app/models/project_import_state.rb b/app/models/project_import_state.rb index e434ea58729..dfe660ac780 100644 --- a/app/models/project_import_state.rb +++ b/app/models/project_import_state.rb @@ -84,7 +84,7 @@ class ProjectImportState < ApplicationRecord update_column(:last_error, sanitized_message) rescue ActiveRecord::ActiveRecordError => e - Rails.logger.error("Error setting import status to failed: #{e.message}. Original error: #{sanitized_message}") # rubocop:disable Gitlab/RailsLogger + Gitlab::AppLogger.error("Error setting import status to failed: #{e.message}. Original error: #{sanitized_message}") ensure @errors = original_errors end diff --git a/changelogs/unreleased/leaky-constant-fix-22.yml b/changelogs/unreleased/leaky-constant-fix-22.yml new file mode 100644 index 00000000000..ebeaceee09f --- /dev/null +++ b/changelogs/unreleased/leaky-constant-fix-22.yml @@ -0,0 +1,5 @@ +--- +title: Fix leaky constant issue in admin mode migration spec +merge_request: 32074 +author: Rajendra Kadam +type: fixed diff --git a/changelogs/unreleased/rails-logger-cop-1.yml b/changelogs/unreleased/rails-logger-cop-1.yml new file mode 100644 index 00000000000..c9b37eb9244 --- /dev/null +++ b/changelogs/unreleased/rails-logger-cop-1.yml @@ -0,0 +1,5 @@ +--- +title: Use applogger in project import state file +merge_request: 32182 +author: Rajendra Kadam +type: fixed diff --git a/changelogs/unreleased/rails-logger-cop-2.yml b/changelogs/unreleased/rails-logger-cop-2.yml new file mode 100644 index 00000000000..0685f503e44 --- /dev/null +++ b/changelogs/unreleased/rails-logger-cop-2.yml @@ -0,0 +1,5 @@ +--- +title: Use applogger in project.rb +merge_request: 32183 +author: Rajendra Kadam +type: fixed diff --git a/changelogs/unreleased/rails-logger-cop-3.yml b/changelogs/unreleased/rails-logger-cop-3.yml new file mode 100644 index 00000000000..b7de674cdb6 --- /dev/null +++ b/changelogs/unreleased/rails-logger-cop-3.yml @@ -0,0 +1,5 @@ +--- +title: Use applogger in chat_team.rb +merge_request: 32184 +author: Rajendra Kadam +type: fixed diff --git a/spec/migrations/20191125114345_add_admin_mode_protected_path_spec.rb b/spec/migrations/20191125114345_add_admin_mode_protected_path_spec.rb index 669e31618a3..77d8dd002e3 100644 --- a/spec/migrations/20191125114345_add_admin_mode_protected_path_spec.rb +++ b/spec/migrations/20191125114345_add_admin_mode_protected_path_spec.rb @@ -4,10 +4,9 @@ require 'spec_helper' require Rails.root.join('db', 'migrate', '20191125114345_add_admin_mode_protected_path.rb') describe AddAdminModeProtectedPath do - ADMIN_MODE_ENDPOINT = '/admin/session' - subject(:migration) { described_class.new } + let(:admin_mode_endpoint) { '/admin/session' } let(:application_settings) { table(:application_settings) } context 'no settings available' do @@ -30,7 +29,7 @@ describe AddAdminModeProtectedPath do application_settings.create!(protected_paths: '{a,b,c}') protected_paths_before = %w[a b c] - protected_paths_after = protected_paths_before.dup << ADMIN_MODE_ENDPOINT + protected_paths_after = protected_paths_before.dup << admin_mode_endpoint expect { migrate! }.to change { application_settings.first.protected_paths }.from(protected_paths_before).to(protected_paths_after) end @@ -38,13 +37,13 @@ describe AddAdminModeProtectedPath do it 'new default includes admin mode endpoint' do settings_before = application_settings.create! - expect(settings_before.protected_paths).not_to include(ADMIN_MODE_ENDPOINT) + expect(settings_before.protected_paths).not_to include(admin_mode_endpoint) migrate! application_settings.reset_column_information settings_after = application_settings.create! - expect(settings_after.protected_paths).to include(ADMIN_MODE_ENDPOINT) + expect(settings_after.protected_paths).to include(admin_mode_endpoint) end end diff --git a/spec/models/project_import_state_spec.rb b/spec/models/project_import_state_spec.rb index cb34d898a6e..0b6b858daa8 100644 --- a/spec/models/project_import_state_spec.rb +++ b/spec/models/project_import_state_spec.rb @@ -57,6 +57,25 @@ describe ProjectImportState, type: :model do end end + describe '#mark_as_failed' do + let(:error_message) { 'some message' } + + it 'logs error when update column fails' do + allow(import_state).to receive(:update_column).and_raise(ActiveRecord::ActiveRecordError) + allow(Gitlab::AppLogger).to receive(:error) + + import_state.mark_as_failed(error_message) + + expect(Gitlab::AppLogger).to have_received(:error) + end + + it 'updates last_error with error message' do + import_state.mark_as_failed(error_message) + + expect(import_state.last_error).to eq(error_message) + end + end + describe '#human_status_name' do context 'when import_state exists' do it 'returns the humanized status name' do