From 4374e1ce48850ea5a602123b3dce29c4a1e758e5 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Tue, 30 May 2023 22:10:15 -0300 Subject: [PATCH 1/2] Prevent GitHub Import to progress worker when import fails Check if the import state is no longer in progress and abort the worker to prevent workers running indefinitely Changelog: fixed --- .../gitlab/github_import/object_importer.rb | 8 +++++-- .../gitlab/github_import/stage_methods.rb | 8 +++++-- .../github_import/object_importer_spec.rb | 17 ++++++++------- .../github_import/stage_methods_spec.rb | 21 +++++++++++-------- .../import_collaborator_worker_spec.rb | 2 ++ .../import_protected_branch_worker_spec.rb | 2 ++ .../import_review_request_worker_spec.rb | 2 ++ 7 files changed, 39 insertions(+), 21 deletions(-) diff --git a/app/workers/concerns/gitlab/github_import/object_importer.rb b/app/workers/concerns/gitlab/github_import/object_importer.rb index 408354d5caa2b4..6cb9bd34969005 100644 --- a/app/workers/concerns/gitlab/github_import/object_importer.rb +++ b/app/workers/concerns/gitlab/github_import/object_importer.rb @@ -38,8 +38,12 @@ module ObjectImporter # client - An instance of `Gitlab::GithubImport::Client` # hash - A Hash containing the details of the object to import. def import(project, client, hash) - if project.import_state&.canceled? - info(project.id, message: 'project import canceled') + unless project.import_state&.in_progress? + info( + project.id, + message: 'Project import is no longer running. Stopping worker.', + import_status: project.import_state.status + ) return end diff --git a/app/workers/concerns/gitlab/github_import/stage_methods.rb b/app/workers/concerns/gitlab/github_import/stage_methods.rb index 1feaaf917b2a86..a5287fcfbe25c4 100644 --- a/app/workers/concerns/gitlab/github_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/github_import/stage_methods.rb @@ -9,8 +9,12 @@ def perform(project_id) return unless (project = find_project(project_id)) - if project.import_state&.canceled? - info(project_id, message: 'project import canceled') + unless project.import_state&.in_progress? + info( + project_id, + message: 'Project import is no longer running. Stopping worker.', + import_status: project.import_state.status + ) return end diff --git a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb index 18a3e3c2c5bf3a..2abbd29fd43ae7 100644 --- a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb @@ -21,8 +21,7 @@ def representation_class end.new end - let_it_be(:project) { create(:project, :import_started) } - let_it_be(:project2) { create(:project, :import_canceled) } + let_it_be(:project) { create(:project, :import_started, import_url: 'https://github.com/foo/baz.git') } let(:importer_class) { double(:importer_class, name: 'klass_name') } let(:importer_instance) { double(:importer_instance) } @@ -113,8 +112,9 @@ def github_identifiers }) end - it 'logs info if the import state is canceled' do - expect(project2.import_state.status).to eq('canceled') + it 'do not execute importer if import state is not in progress' do + allow(project.import_state).to receive(:status).and_return('failed') + allow(project.import_state).to receive(:in_progress?).and_return(false) expect(importer_class).not_to receive(:new) @@ -125,13 +125,14 @@ def github_identifiers .with( { github_identifiers: nil, - message: 'project import canceled', - project_id: project2.id, - importer: 'klass_name' + message: 'Project import is no longer running. Stopping worker.', + project_id: project.id, + importer: 'klass_name', + import_status: 'failed' } ) - worker.import(project2, client, { 'number' => 11, 'github_id' => 2 } ) + worker.import(project, client, { 'number' => 11, 'github_id' => 2 } ) end it 'logs error when the import fails' do diff --git a/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb b/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb index ce9a9db5dd9163..d054421ca14b59 100644 --- a/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb @@ -4,7 +4,6 @@ RSpec.describe Gitlab::GithubImport::StageMethods, feature_category: :importers do let_it_be(:project) { create(:project, :import_started, import_url: 'https://t0ken@github.com/repo/repo.git') } - let_it_be(:project2) { create(:project, :import_canceled) } let(:worker) do Class.new do @@ -23,11 +22,14 @@ def self.name worker.perform(-1) end - it 'returns if the import state is canceled' do + it 'returns if the import state is no longer in progress' do + allow(project.import_state).to receive(:status).and_return('failed') + allow(project.import_state).to receive(:in_progress?).and_return(false) + allow(worker) .to receive(:find_project) - .with(project2.id) - .and_return(project2) + .with(project.id) + .and_return(project) expect(worker).not_to receive(:try_import) @@ -36,7 +38,7 @@ def self.name .with( { message: 'starting stage', - project_id: project2.id, + project_id: project.id, import_stage: 'DummyStage' } ) @@ -45,13 +47,14 @@ def self.name .to receive(:info) .with( { - message: 'project import canceled', - project_id: project2.id, - import_stage: 'DummyStage' + message: 'Project import is no longer running. Stopping worker.', + project_id: project.id, + import_stage: 'DummyStage', + import_status: 'failed' } ) - worker.perform(project2.id) + worker.perform(project.id) end it 'imports the data when the project exists' do diff --git a/spec/workers/gitlab/github_import/import_collaborator_worker_spec.rb b/spec/workers/gitlab/github_import/import_collaborator_worker_spec.rb index b9463fb9a2da1c..c8d3b66734ca1a 100644 --- a/spec/workers/gitlab/github_import/import_collaborator_worker_spec.rb +++ b/spec/workers/gitlab/github_import/import_collaborator_worker_spec.rb @@ -15,6 +15,8 @@ let(:importer) { instance_double('Gitlab::GithubImport::Importer::NoteAttachmentsImporter') } it 'imports a collaborator' do + allow(import_state).to receive(:in_progress?).and_return(true) + expect(Gitlab::GithubImport::Importer::CollaboratorImporter) .to receive(:new) .with( diff --git a/spec/workers/gitlab/github_import/import_protected_branch_worker_spec.rb b/spec/workers/gitlab/github_import/import_protected_branch_worker_spec.rb index d6e8f7600334d4..8097f9d840a6f4 100644 --- a/spec/workers/gitlab/github_import/import_protected_branch_worker_spec.rb +++ b/spec/workers/gitlab/github_import/import_protected_branch_worker_spec.rb @@ -20,6 +20,8 @@ end it 'imports protected branch rule' do + allow(import_state).to receive(:in_progress?).and_return(true) + expect(Gitlab::GithubImport::Importer::ProtectedBranchImporter) .to receive(:new) .with( diff --git a/spec/workers/gitlab/github_import/pull_requests/import_review_request_worker_spec.rb b/spec/workers/gitlab/github_import/pull_requests/import_review_request_worker_spec.rb index 99ed83ae2db2a1..4c9e0fd618e192 100644 --- a/spec/workers/gitlab/github_import/pull_requests/import_review_request_worker_spec.rb +++ b/spec/workers/gitlab/github_import/pull_requests/import_review_request_worker_spec.rb @@ -26,6 +26,8 @@ end it 'imports an pull request review requests' do + allow(import_state).to receive(:in_progress?).and_return(true) + expect(Gitlab::GithubImport::Importer::PullRequests::ReviewRequestImporter) .to receive(:new) .with( -- GitLab From c31da00f5aa88f3d85cb6e1f5c4936bbedfa189f Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Fri, 9 Jun 2023 12:23:03 -0300 Subject: [PATCH 2/2] Fix typo and method stub --- .../concerns/gitlab/github_import/object_importer_spec.rb | 3 +-- .../concerns/gitlab/github_import/stage_methods_spec.rb | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb index 2abbd29fd43ae7..3b7bbfc8a7b029 100644 --- a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb @@ -112,9 +112,8 @@ def github_identifiers }) end - it 'do not execute importer if import state is not in progress' do + it 'does not execute importer if import state is not in progress' do allow(project.import_state).to receive(:status).and_return('failed') - allow(project.import_state).to receive(:in_progress?).and_return(false) expect(importer_class).not_to receive(:new) diff --git a/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb b/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb index d054421ca14b59..f65a8cd0d3c7eb 100644 --- a/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb @@ -24,7 +24,6 @@ def self.name it 'returns if the import state is no longer in progress' do allow(project.import_state).to receive(:status).and_return('failed') - allow(project.import_state).to receive(:in_progress?).and_return(false) allow(worker) .to receive(:find_project) -- GitLab