Merge branch 'sh-suppress-duplicate-remote-mirror-notifications' into 'master'
Only send one notification for failed remote mirror Closes #56222 See merge request gitlab-org/gitlab-ce!24381
This commit is contained in:
commit
9cd5c5f535
|
@ -61,7 +61,10 @@ class RemoteMirror < ActiveRecord::Base
|
||||||
|
|
||||||
timestamp = Time.now
|
timestamp = Time.now
|
||||||
remote_mirror.update!(
|
remote_mirror.update!(
|
||||||
last_update_at: timestamp, last_successful_update_at: timestamp, last_error: nil
|
last_update_at: timestamp,
|
||||||
|
last_successful_update_at: timestamp,
|
||||||
|
last_error: nil,
|
||||||
|
error_notification_sent: false
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -179,6 +182,10 @@ class RemoteMirror < ActiveRecord::Base
|
||||||
project.repository.add_remote(remote_name, remote_url)
|
project.repository.add_remote(remote_name, remote_url)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def after_sent_notification
|
||||||
|
update_column(:error_notification_sent, true)
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def store_credentials
|
def store_credentials
|
||||||
|
@ -221,7 +228,8 @@ class RemoteMirror < ActiveRecord::Base
|
||||||
last_error: nil,
|
last_error: nil,
|
||||||
last_update_at: nil,
|
last_update_at: nil,
|
||||||
last_successful_update_at: nil,
|
last_successful_update_at: nil,
|
||||||
update_status: 'finished'
|
update_status: 'finished',
|
||||||
|
error_notification_sent: false
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -9,7 +9,10 @@ class RemoteMirrorNotificationWorker
|
||||||
# We check again if there's an error because a newer run since this job was
|
# We check again if there's an error because a newer run since this job was
|
||||||
# fired could've completed successfully.
|
# fired could've completed successfully.
|
||||||
return unless remote_mirror && remote_mirror.last_error.present?
|
return unless remote_mirror && remote_mirror.last_error.present?
|
||||||
|
return if remote_mirror.error_notification_sent?
|
||||||
|
|
||||||
NotificationService.new.remote_mirror_update_failed(remote_mirror)
|
NotificationService.new.remote_mirror_update_failed(remote_mirror)
|
||||||
|
|
||||||
|
remote_mirror.after_sent_notification
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,11 @@
|
||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class AddErrorNotificationSentToRemoteMirrors < ActiveRecord::Migration[5.0]
|
||||||
|
include Gitlab::Database::MigrationHelpers
|
||||||
|
|
||||||
|
DOWNTIME = false
|
||||||
|
|
||||||
|
def change
|
||||||
|
add_column :remote_mirrors, :error_notification_sent, :boolean
|
||||||
|
end
|
||||||
|
end
|
|
@ -10,7 +10,7 @@
|
||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# It's strongly recommended that you check this file into your version control system.
|
||||||
|
|
||||||
ActiveRecord::Schema.define(version: 20190103140724) do
|
ActiveRecord::Schema.define(version: 20190115054216) do
|
||||||
|
|
||||||
# These are extensions that must be enabled in order to support this database
|
# These are extensions that must be enabled in order to support this database
|
||||||
enable_extension "plpgsql"
|
enable_extension "plpgsql"
|
||||||
|
@ -1849,6 +1849,7 @@ ActiveRecord::Schema.define(version: 20190103140724) do
|
||||||
t.string "encrypted_credentials_salt"
|
t.string "encrypted_credentials_salt"
|
||||||
t.datetime "created_at", null: false
|
t.datetime "created_at", null: false
|
||||||
t.datetime "updated_at", null: false
|
t.datetime "updated_at", null: false
|
||||||
|
t.boolean "error_notification_sent"
|
||||||
t.index ["last_successful_update_at"], name: "index_remote_mirrors_on_last_successful_update_at", using: :btree
|
t.index ["last_successful_update_at"], name: "index_remote_mirrors_on_last_successful_update_at", using: :btree
|
||||||
t.index ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree
|
t.index ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree
|
||||||
end
|
end
|
||||||
|
|
|
@ -303,6 +303,25 @@ describe RemoteMirror, :mailer do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context '#url=' do
|
||||||
|
let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
|
||||||
|
|
||||||
|
it 'resets all the columns when URL changes' do
|
||||||
|
remote_mirror.update(last_error: Time.now,
|
||||||
|
last_update_at: Time.now,
|
||||||
|
last_successful_update_at: Time.now,
|
||||||
|
update_status: 'started',
|
||||||
|
error_notification_sent: true)
|
||||||
|
|
||||||
|
expect { remote_mirror.update_attribute(:url, 'http://new.example.com') }
|
||||||
|
.to change { remote_mirror.last_error }.to(nil)
|
||||||
|
.and change { remote_mirror.last_update_at }.to(nil)
|
||||||
|
.and change { remote_mirror.last_successful_update_at }.to(nil)
|
||||||
|
.and change { remote_mirror.update_status }.to('finished')
|
||||||
|
.and change { remote_mirror.error_notification_sent }.to(false)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context '#updated_since?' do
|
context '#updated_since?' do
|
||||||
let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
|
let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
|
||||||
let(:timestamp) { Time.now - 5.minutes }
|
let(:timestamp) { Time.now - 5.minutes }
|
||||||
|
|
|
@ -0,0 +1,39 @@
|
||||||
|
require 'spec_helper'
|
||||||
|
|
||||||
|
describe RemoteMirrorNotificationWorker, :mailer do
|
||||||
|
set(:project) { create(:project, :repository, :remote_mirror) }
|
||||||
|
set(:mirror) { project.remote_mirrors.first }
|
||||||
|
|
||||||
|
describe '#execute' do
|
||||||
|
it 'calls NotificationService#remote_mirror_update_failed when the mirror exists' do
|
||||||
|
mirror.update_column(:last_error, "There was a problem fetching")
|
||||||
|
|
||||||
|
expect(NotificationService).to receive_message_chain(:new, :remote_mirror_update_failed)
|
||||||
|
|
||||||
|
subject.perform(mirror.id)
|
||||||
|
|
||||||
|
expect(mirror.reload.error_notification_sent?).to be_truthy
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does nothing when the mirror has no errors' do
|
||||||
|
expect(NotificationService).not_to receive(:new)
|
||||||
|
|
||||||
|
subject.perform(mirror.id)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does nothing when the mirror does not exist' do
|
||||||
|
expect(NotificationService).not_to receive(:new)
|
||||||
|
|
||||||
|
subject.perform(RemoteMirror.maximum(:id).to_i.succ)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does nothing when a notification has already been sent' do
|
||||||
|
mirror.update_columns(last_error: "There was a problem fetching",
|
||||||
|
error_notification_sent: true)
|
||||||
|
|
||||||
|
expect(NotificationService).not_to receive(:new)
|
||||||
|
|
||||||
|
subject.perform(mirror.id)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -22,6 +22,13 @@ describe RepositoryUpdateRemoteMirrorWorker do
|
||||||
expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.update_status }.to('finished')
|
expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.update_status }.to('finished')
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'resets the notification flag upon success' do
|
||||||
|
expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success)
|
||||||
|
remote_mirror.update_column(:error_notification_sent, true)
|
||||||
|
|
||||||
|
expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.error_notification_sent }.to(false)
|
||||||
|
end
|
||||||
|
|
||||||
it 'sets status as failed when update remote mirror service executes with errors' do
|
it 'sets status as failed when update remote mirror service executes with errors' do
|
||||||
error_message = 'fail!'
|
error_message = 'fail!'
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue