From 963ef398cac294eda1b491a6c3f70f755d45c40f Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Thu, 27 Oct 2016 13:35:53 +0100 Subject: [PATCH 1/3] WIP --- .../repository_update_remote_mirror_worker.rb | 6 ++--- spec/factories/remote_mirrors.rb | 6 +++++ spec/models/remote_mirror_spec.rb | 11 ++++++++ ...sitory_update_remote_mirror_worker_spec.rb | 13 ++++++++++ .../update_all_remote_mirrors_worker_spec.rb | 25 +++++++++++++++++++ 5 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 spec/factories/remote_mirrors.rb create mode 100644 spec/workers/repository_update_remote_mirror_worker_spec.rb create mode 100644 spec/workers/update_all_remote_mirrors_worker_spec.rb diff --git a/app/workers/repository_update_remote_mirror_worker.rb b/app/workers/repository_update_remote_mirror_worker.rb index fdcae2895ef2f7..6aa8520e8e15b3 100644 --- a/app/workers/repository_update_remote_mirror_worker.rb +++ b/app/workers/repository_update_remote_mirror_worker.rb @@ -18,10 +18,10 @@ def perform(remote_mirror_id) else remote_mirror.update_finish end - rescue => ex - remote_mirror.mark_as_failed("We're sorry, a temporary error occurred, please try again.") + # rescue => ex + # remote_mirror.mark_as_failed("We're sorry, a temporary error occurred, please try again.") - raise UpdateRemoteMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}" + # raise UpdateRemoteMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}" end end end diff --git a/spec/factories/remote_mirrors.rb b/spec/factories/remote_mirrors.rb new file mode 100644 index 00000000000000..f8133bfbba66ea --- /dev/null +++ b/spec/factories/remote_mirrors.rb @@ -0,0 +1,6 @@ +FactoryGirl.define do + factory :remote_mirror do + url 'http://cantbeblank' + project + end +end \ No newline at end of file diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 46bc8affe6498c..84f53dbb6eeaef 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -84,6 +84,17 @@ end end + context 'with missing project' do + it 'can be marked as failed' do + mirror = create(:remote_mirror) + mirror.update!(enabled: true, project_id: nil) + + mirror.reload.mark_as_failed('missing project') + + expect(mirror.reload.last_error).to eq 'missing project' + end + end + def create_mirror(params) project = FactoryGirl.create(:project) project.remote_mirrors.create!(params) diff --git a/spec/workers/repository_update_remote_mirror_worker_spec.rb b/spec/workers/repository_update_remote_mirror_worker_spec.rb new file mode 100644 index 00000000000000..46d1a2c96cc29b --- /dev/null +++ b/spec/workers/repository_update_remote_mirror_worker_spec.rb @@ -0,0 +1,13 @@ +require 'rails_helper' + +describe RepositoryUpdateRemoteMirrorWorker do + let(:mirror) { create(:remote_mirror) } + + describe '#perform' do + it 'does nothing if project is missing' do + mirror.update!(project_id: nil) + + expect(described_class.new.perform(mirror.id)).to be_nil + end + end +end diff --git a/spec/workers/update_all_remote_mirrors_worker_spec.rb b/spec/workers/update_all_remote_mirrors_worker_spec.rb new file mode 100644 index 00000000000000..377f35c3081658 --- /dev/null +++ b/spec/workers/update_all_remote_mirrors_worker_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' + +describe UpdateAllRemoteMirrorsWorker do + let(:worker) { described_class.new } + + describe '#perform' do + context 'stuck mirrors' do + let!(:mirror) { create(:remote_mirror, update_status: :started, last_update_at: 1.week.ago) } + + it 'are transitioned to failure state' do + worker.perform + + expect(mirror.reload.update_status).to eq 'failed' + end + + it 'handles enabled mirrors with missing project' do + mirror.update!(project: nil, enabled: true) + + worker.perform + + expect(mirror.reload.update_status).to eq 'failed' + end + end + end +end -- GitLab From cad63649e0abe66f12adf3130f4cd7c0790b465e Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 11 Nov 2016 12:21:23 +0100 Subject: [PATCH 2/3] fix remote mirror issue to do with missing project and updated specs --- app/models/remote_mirror.rb | 17 ++++++++++++----- .../repository_update_remote_mirror_worker.rb | 9 ++++++--- spec/models/remote_mirror_spec.rb | 15 +++++++++++---- .../update_all_remote_mirrors_worker_spec.rb | 6 +++--- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index 2fd6ea877e3342..a4f7113519ebb4 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -39,9 +39,15 @@ class RemoteMirror < ActiveRecord::Base transition failed: :started end - state :started + state :started do + validates_presence_of :project + end + state :finished - state :failed + + state :failed do + validates_presence_of :project + end after_transition any => :started, do: :schedule_update_job @@ -80,8 +86,9 @@ def mark_for_delete_if_blank_url end def mark_as_failed(error_message) - update_fail - update_column(:last_error, Gitlab::UrlSanitizer.sanitize(error_message)) + error_message = errors.full_messages.first unless update_fail + + update_columns(last_error: Gitlab::UrlSanitizer.sanitize(error_message), update_status: 'failed') end def url=(value) @@ -109,7 +116,7 @@ def safe_url private def url_availability - if project.import_url == url && project.mirror? + if project && project.import_url == url && project.mirror? errors.add(:url, 'is already in use') end end diff --git a/app/workers/repository_update_remote_mirror_worker.rb b/app/workers/repository_update_remote_mirror_worker.rb index 6aa8520e8e15b3..3993788fb698ac 100644 --- a/app/workers/repository_update_remote_mirror_worker.rb +++ b/app/workers/repository_update_remote_mirror_worker.rb @@ -10,6 +10,9 @@ def perform(remote_mirror_id) begin remote_mirror = RemoteMirror.find(remote_mirror_id) project = remote_mirror.project + + return unless project + current_user = project.creator result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror) @@ -18,10 +21,10 @@ def perform(remote_mirror_id) else remote_mirror.update_finish end - # rescue => ex - # remote_mirror.mark_as_failed("We're sorry, a temporary error occurred, please try again.") + rescue => ex + remote_mirror.mark_as_failed("We're sorry, a temporary error occurred, please try again.") - # raise UpdateRemoteMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}" + raise UpdateRemoteMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}" end end end diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 84f53dbb6eeaef..63241aa111780d 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -85,13 +85,20 @@ end context 'with missing project' do - it 'can be marked as failed' do - mirror = create(:remote_mirror) - mirror.update!(enabled: true, project_id: nil) + let(:mirror) { create(:remote_mirror, update_status: 'started') } + + before do + mirror.update_columns(enabled: true, project_id: nil) mirror.reload.mark_as_failed('missing project') + end + + it 'sets the last error' do + expect(mirror.reload.last_error).to include("Project can't be blank") + end - expect(mirror.reload.last_error).to eq 'missing project' + it 'is marked as failed' do + expect(mirror.reload.update_status).to eq('failed') end end diff --git a/spec/workers/update_all_remote_mirrors_worker_spec.rb b/spec/workers/update_all_remote_mirrors_worker_spec.rb index 377f35c3081658..3266ee93b568ce 100644 --- a/spec/workers/update_all_remote_mirrors_worker_spec.rb +++ b/spec/workers/update_all_remote_mirrors_worker_spec.rb @@ -10,15 +10,15 @@ it 'are transitioned to failure state' do worker.perform - expect(mirror.reload.update_status).to eq 'failed' + expect(mirror.reload.update_status).to eq('failed') end it 'handles enabled mirrors with missing project' do - mirror.update!(project: nil, enabled: true) + mirror.update_attributes(project_id: nil, enabled: true) worker.perform - expect(mirror.reload.update_status).to eq 'failed' + expect(mirror.reload.update_status).to eq('failed') end end end -- GitLab From 1eaa546f41cf9ca5acb6e0ccbf3cc5c685ddcbe3 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 11 Nov 2016 12:55:07 +0100 Subject: [PATCH 3/3] add changelog and fix rubocop warning --- .../1165-jej-remote-mirror-validation-failures.yml | 4 ++++ spec/factories/remote_mirrors.rb | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased-ee/1165-jej-remote-mirror-validation-failures.yml diff --git a/changelogs/unreleased-ee/1165-jej-remote-mirror-validation-failures.yml b/changelogs/unreleased-ee/1165-jej-remote-mirror-validation-failures.yml new file mode 100644 index 00000000000000..9d9302ca1a0b5e --- /dev/null +++ b/changelogs/unreleased-ee/1165-jej-remote-mirror-validation-failures.yml @@ -0,0 +1,4 @@ +--- +title: Fix UpdateAllRemoteMirrorsWorker failing when remote mirror has no project +merge_request: +author: diff --git a/spec/factories/remote_mirrors.rb b/spec/factories/remote_mirrors.rb index f8133bfbba66ea..1e534325d8d074 100644 --- a/spec/factories/remote_mirrors.rb +++ b/spec/factories/remote_mirrors.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :remote_mirror do - url 'http://cantbeblank' - project + url 'http://cantbeblank' + project end -end \ No newline at end of file +end -- GitLab